Closed Bug 1263439 Opened 8 years ago Closed 8 years ago

CSS rules inspector not showing rules when stylesheet has a data-uri sourcemap and link href is generated with `URL.createObjectURL`

Categories

(DevTools :: Inspector: Rules, defect, P1)

46 Branch
defect

Tracking

(firefox45 unaffected, firefox46 wontfix, firefox47+ fixed, firefox48+ fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox45 --- unaffected
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file blob.html
When a stylesheet is inserted in a page using a blob and `URL.createObjectURL` , and has a data-uri sourcemap, the rule inspector don't show any rules.

STR:

1. Open attached file
2. Right-click -> inspect "Test" element ( which will open the inspector )

Expected result :
The Rules panel shows the rule for the h1 tag

Actual result :
The Rules is empty. Error is shown in Browser Toolbox : 
`
Full Message: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]
Full Stack: JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: dirname :: line 1202
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: StyleSheetActor<._setSourceMapRoot :: line 806
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: StyleSheetActor<._fetchSourceMap/</map< :: line 779
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816
`
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Priority: -- → P1
Assignee: nobody → chevobbe.nicolas
Status: NEW → ASSIGNED
Has STR: --- → yes
Keywords: regression
Summary: CSS rules inspector not showing rules when stylesheet has a data-uri sourcemap and link href is generated with `URL.createObjectURL`` → CSS rules inspector not showing rules when stylesheet has a data-uri sourcemap and link href is generated with `URL.createObjectURL`
An error was thrown : `
Full Message: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]
Full Stack: JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: dirname :: line 1202
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: StyleSheetActor<._setSourceMapRoot :: line 806
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: StyleSheetActor<._fetchSourceMap/</map< :: line 779
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816
`
I stripped out the "blob:" part of the safeUrl before calling the `dirname` function and it resolves the bug.
However, I don't know if it is a proper fix ( do we want the sourceMapRoot url without the "blob:" part ? ).
If it is, I don't know if we can move it to the safeHref getter instead.

Review commit: https://reviewboard.mozilla.org/r/45369/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45369/
Attachment #8739773 - Flags: review?(pbrosset)
Another regression of bug 1243736.
Blocks: 1243736
Version: 47 Branch → 46 Branch
Bug 1243736 has introduced 3 regressions: bug 1249888 (fixed in 46+), bug 1255787 (only fixed in 48) and the current bug 1263439 (waiting for fix). Could we backport the last 2 patches in 47 at least?
Flags: needinfo?(lhenry)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

https://reviewboard.mozilla.org/r/45369/#review42287

This seems to work fine. Thanks.
I'd like to see a test for it though. It should be an easy one. Create a new doc_blob_stylesheet.html file in devtools/client/inspector/rules/test and a new test browser_rules_blob_stylesheet.js that goes with it. The doc page should craft a blob URL like the attachment you added to the bug, and the test should just open it and assert that there are rules displayed in the sidebar, nothing more.
Attachment #8739773 - Flags: review?(pbrosset)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45369/diff/1-2/
Attachment #8739773 - Flags: review?(pbrosset)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45369/diff/2-3/
Attachment #8739773 - Flags: review?(pbrosset) → review+
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

https://reviewboard.mozilla.org/r/45369/#review42299

Looks good, thanks!

Just a minor comment about the test html file, which you can address without asking for a new review.

Also, the first line of the commit message should try and say what you changed, rather than explain the problem only. Maybe something like this:

Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

And then go into more details on the following lines (although I'm not sure the error message should be in there).

::: devtools/client/inspector/rules/test/doc_blob_stylesheet.html:1
(Diff revision 2)
> +<head>



::: devtools/client/inspector/rules/test/doc_blob_stylesheet.html:1
(Diff revision 2)
> +<head>

Please add the license header that we have in other html files and while you're at it, maybe make it a complete HTML document with doctype, html, and meta utf-8.
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45369/diff/3-4/
Attachment #8739773 - Attachment description: MozReview Request: Bug 1263439 - Fix empty rules panel when stylesheet href is created with 'URL.createObjectURL' and has a data-uri sourcemap. r=pbro → MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro
Thanks loic, patrick, nicolas, for digging into this. 
We could still take a patch in 47 but it is getting late for 46 uplift. 
If you disagree and think it is important (and safe for beta 11) please let me know.
Flags: needinfo?(lhenry)
This was backed out from fx-team for eslint failures.
Here are the failures:


TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/browser_rules_blob_stylesheet.js:10:18 | Missing space after *. (generator-star-spacing)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:13:1 | Use the global form of 'use strict'. (strict)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:19:1 | Line 19 exceeds the maximum line length of 80. (max-len)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:20:45 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:23:11 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:23:36 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:24:11 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:24:36 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:25:11 | Multiple spaces found before '='. (no-multi-spaces)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:25:13 | Strings must use doublequote. (quotes)
TEST-UNEXPECTED-ERROR | devtools/client/inspector/rules/test/doc_blob_stylesheet.html:26:13 | Strings must use doublequote. (quotes)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45369/diff/4-5/
(In reply to Nicolas Chevobbe from comment #14)
> Comment on attachment 8739773 [details]
> MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created
> with URL.createObjectURL; r=pbro
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/45369/diff/4-5/

I fixed the eslint errors and now both `./mach eslint devtools/client/inspector/rules/test/browser_rules_blob_stylesheet.js`and `/mach eslint devtools/client/inspector/rules/test/doc_blob_stylesheet.html` output `No errors encountered.`.

As discussed with pbro on IRC, I put this as checkin-needed again
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a83cf78360d1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Big thank you to everyone who worked on this. Highly appreciated.
Hi Patrick, Nicolas, this is marked as a P1 regression. Should we consider uplifting this fix to Beta47?
Flags: needinfo?(pbrosset)
Flags: needinfo?(chevobbe.nicolas)
Sorry I don't know what's the protocol to uplift a patch. Patrick would know better.
Flags: needinfo?(chevobbe.nicolas)
(In reply to Ritu Kothari (:ritu) from comment #19)
> Hi Patrick, Nicolas, this is marked as a P1 regression. Should we consider
> uplifting this fix to Beta47?
Hard to tell. I have no idea how many developers can be affected by this. The fix is short and safe though, so I'll request uplift to beta.
Flags: needinfo?(pbrosset)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Approval Request Comment
[Feature/regressing bug #]: 1243736
[User impact if declined]: Low impact. This only impacts devtools users who open the inspector on a site that loads a stylesheet with a blob URL, and when this stylesheet defines a source map with a data-uri.
When that happens, the inspector's rule-view is empty (no css rules displayed).
[Describe test coverage new/current, TreeHerder]: A new automated test was added. And this fix has landed a while back and has not caused any regressions in nightly and aurora so far.
[Risks and why]: Minimal risk for the reason stated above. Should there be a problem anyway, then it would only affect the rule-view in the inspector of devtools.
[String/UUID change made/needed]: None
Attachment #8739773 - Flags: approval-mozilla-beta?
Hello Nicolas, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(chevobbe.nicolas)
Comment on attachment 8739773 [details]
MozReview Request: Bug 1263439 - Remove "blob:" from stylesheet URLs created with URL.createObjectURL; r=pbro

Recent regression in devtools inspector, baked in Nightly for ~2 weeks, Beta47+
Attachment #8739773 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Ritu Kothari (:ritu) from comment #23)
> Hello Nicolas, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

Following steps from Comment 0 in Nightly 49.0a1 (2016-05-02), I do see the expected rules, so this issue is actually fixed.
Flags: needinfo?(chevobbe.nicolas)
I have reproduced this bug with nightly 48.0a1 (2016-04-10) on Windows 10, 64 bit!

The Bug's fix is now verified on Release 47.0 and Beta 48.0b6.

Firefox 47.0
Build ID 	20160604131506
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0

Beta 48.0b6
Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

 [bugday-20160713]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.