Closed
Bug 1255787
Opened 9 years ago
Closed 9 years ago
"Rules" tab is empty when inspecting elements on https://davidwalsh.name/
Categories
(DevTools :: Inspector, defect, P1)
Tracking
(firefox45 unaffected, firefox46+ wontfix, firefox47+ fixed, firefox48+ verified)
VERIFIED
FIXED
Firefox 48
People
(Reporter: marco, Assigned: pbro)
References
()
Details
(Keywords: regression, Whiteboard: [btpp-fix-now])
Attachments
(2 files)
(deleted),
video/webm
|
Details | |
(deleted),
patch
|
gl
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Sometimes some content appears and rapidly disappears.
Updated•9 years ago
|
status-firefox45:
--- → unaffected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Assignee | ||
Comment 1•9 years ago
|
||
Looks like a css sourcemap issue to me.
This appears in the browser console shortly after the inspector is opened.
A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Date: Fri Mar 11 2016 15:12:25 GMT+0100 (CET)
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 :: normalize :: line 1177
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js :: StyleSheetActor<._fetchSourceMap/< :: line 745
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 937
JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 816
Priority: -- → P1
Whiteboard: [btpp-fix-now]
Assignee | ||
Comment 2•9 years ago
|
||
Services.io.newURI throws an exception on David's site.
See https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/stylesheets.js#1177
This results in
NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]
The URI in this case is "maps/above-fold.css.map", and I think nsIIOService.netURI doesn't work with relative URLs.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
More investigation:
The site has an inline stylesheet (a <style> tag), which contains one or more stylesheets that come from a PHP include() instruction on the server side.
One of the included stylesheets has a sourcemap instruction:
/*# sourceMappingURL=maps/above-fold.css.map */
Because of this, devtools tries to fetch this source map.
But it turns out that to do this, it tries to resolve this relative URL to an absolute URL. To do this, it attempts to use the stylesheet URL as a base.
The problem is that we assume that all stylesheets have an href attribute, which they don't, in the case of inline stylesheets.
So the fix here is to check the type of stylesheet, and if it is an inline stylesheet, then get the href form the document instead.
Now, with this fixed, the rule-view appears normally again.
Now, the relative URL of the source map in the <style> tag is wrong. David told me that the actual location for the source map is: https://davidwalsh.name/wp-content/themes/punky/maps/above-fold.css.map
but that's not a problem that causes any devtools bug.
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
This seems ready for review.
Attachment #8729562 -
Flags: review?(ttromey)
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Comment 7•9 years ago
|
||
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
Tom is on PTO this week, and this is a rather important bug, so transferring the review request to Gabriel instead.
Attachment #8729562 -
Flags: review?(ttromey) → review?(gl)
Comment 8•9 years ago
|
||
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
Review of attachment 8729562 [details] [diff] [review]:
-----------------------------------------------------------------
Not my review any more but I wanted to register my approval.
::: devtools/client/inspector/rules/test/doc_inline_sourcemap.html
@@ +11,5 @@
> +
> +span {
> + background-color: #EEE; }
> +
> +/*# sourceMappingURL=doc_sourcemaps.css.map */
I think this line could use a comment explaining that the URL has to be relative.
Comment 9•9 years ago
|
||
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
Review of attachment 8729562 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/server/actors/stylesheets.js
@@ +448,5 @@
> return this.rawSheet.href;
> },
>
> /**
> + * This returns either the stylesheet href or the document href if the sheet
Let's rephrase the comment to "Returns the stylesheet href or the document href ..."
Attachment #8729562 -
Flags: review?(gl) → review+
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Patrick, want to uplift this fix at least to 47, and maybe to 46 beta 11? Since it seems to be a regression in 46.
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
Approval Request Comment
[Feature/regressing bug #]: 1243736
[User impact if declined]: If declined, then users visiting web pages that have inline stylesheets (<style> tags) which contain sourcemap URLs are going to have an empty CSS Rules view in the devtools inspector.
This use case should happen fairly rarely. Source maps are usually only used in development, and in external stylesheets. But still, this may happen.
[Describe test coverage new/current, TreeHerder]: There's a new test for this in the tree, and the fix has been in m-c for 2 weeks.
[Risks and why]: The risk is very limited and the fix has been tested on m-c for 2 weeks. So we should be fine.
Any bug introduced in this patch would cause the CSS Rule View to be impacted again, so there's no risk to firefox users not using devtools.
[String/UUID change made/needed]: None
Flags: needinfo?(pbrosset)
Attachment #8729562 -
Flags: approval-mozilla-aurora?
Comment on attachment 8729562 [details] [diff] [review]
WIP - Bug_1255787_-_Do_not_assume_sourceMap_appears_only.diff
Fix a regression in Devtools CSS Rules view, Aurora47+
Attachment #8729562 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marco, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(mcastelluccio)
Reporter | ||
Comment 17•9 years ago
|
||
The rules take some time to appear, but the issue is fixed.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mcastelluccio)
Too late for 46 as I don't think the issue is widespread (Sorry David...) but this will be fixed in 47.
Comment 19•9 years ago
|
||
bugherder uplift |
Comment 20•9 years ago
|
||
I have reproduced this bug according to (2016-03-11)
It's fixed on Latest Developer Edition -- Build ID (20160423004022) ;User Agent: Mozilla/5.0 (Windows NT 6.3; rv:47.0) Gecko/20100101 Firefox/47.0
Tested OS-- Windows8.1 32bit
QA Whiteboard: [bugday-20160420]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•