Closed Bug 1255787 Opened 8 years ago Closed 8 years ago

"Rules" tab is empty when inspecting elements on https://davidwalsh.name/

Categories

(DevTools :: Inspector, defect, P1)

46 Branch
defect

Tracking

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

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

People

(Reporter: marco, Assigned: pbro)

References

()

Details

(Keywords: regression, Whiteboard: [btpp-fix-now])

Attachments

(2 files)

Attached video asd2.webm
Sometimes some content appears and rapidly disappears.
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]
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: nobody → pbrosset
Status: NEW → ASSIGNED
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.
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)
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 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 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+
https://hg.mozilla.org/mozilla-central/rev/dbb64aae2a23
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Blocks: 1243736
Keywords: regression
Version: Trunk → 46 Branch
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.
Flags: needinfo?(pbrosset)
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)
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.
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.