1249888 - Inspector doesn't show style rules in developer edition

Status: RESOLVED FIXED

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

Description

[cvetan.simsic]

Attachment: firefox-inspector.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160209234642

Steps to reproduce:

Inspect some element like text field, text area etc... Inspector should show style rules but it doesn't.

Comment 1

[Loic]

Could you attach a testcase to reproduce the issue when opening Inspector, please.

Comment 2

[cvetan.simsic]

I've put attachment in description. 

There is screenshot of inspector window. If something else is needed, please instruct me.

Comment 3

[Loic]

It doesn't really help, because the code is partially interpreted. In addition, we need the CSS sheet with the HTML source.

Comment 4

[cvetan.simsic]

It's a bootstrap min css, and some of my cystom css. 

But in regular firefox it shows all normal. 


It looks like it doesn't display rules from minimized css files although i have some custom css also in the element i am trying to inspect.

Comment 5

[Pouyan]

Hi want to second this problem on
FF Ubuntu 46.0a2 (2016-02-21).
Rules are not shown when bootstrap is attached to the webpage.

Comment 6

[Pouyan]

Attachment: Rules are not shown on FF46Dev

Comment 7

[Tom Tromey :tromey]

I could reproduce by visiting http://uni-wuerzburg.de/scripts/ (seen in the attached png)
and then opening the inspector.

I see this in the browser console:

GET 
http://uni-wuerzburg.de/scripts/static/bootstrap.css.map [HTTP/1.0 404 Not Found 206ms]

... which is suggestive.

If I then open the <header> in the inspector and click the <nav>, I get an error:

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: Mon Feb 22 2016 09:03:28 GMT-0700 (MST)
Full Message: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data
Full Stack: SourceMapConsumer@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/sourcemap/source-map.js:1281:20
StyleSheetActor<._fetchSourceMap/</map<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/stylesheets.js:753:21
Handler.prototype.process@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/Promise-backend.js:714:7
mainThreadFetch/onResponse@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:442:7
NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17
 source-map.js:1281:0
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

Comment 8

[Loic]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5f9ba76eb3b1fd9377bbdb4cc2f98a7e75eabdfb&tochange=ce6b2fdabc3ecd709abf5de6dbe963edd44d18fb

Bug 1243749 - Enable 2 rule-view test with e10s and fix unhandled rejected promises; r=miker
Bug 1243736 - Enable browser_rules_pseudo-element_02.js with e10s and make rule-view wait for updateSourceLink; r=bgrins

Comment 9

[Patrick Brosset <:pbro>]

I was able to see the errors mentioned in comment 7 with FF46 but couldn't get an empty rules-view.
However with FF47, I see both the errors and the rules-view is empty.

It indeed looks like a problem with a source map not being found.
And because the server returns a 404 HTML page:

"<html><head><title>404 Not found</title><meta http-equiv="refresh" CONTENT="0;URL=http://uni-wuerzburg.de/not_found/"></head><body><h1>HTTP/1.0 404 Not Found</h1><!-- Redirect URL is http://uni-wuerzburg.de/not_found/ --></body></html>"

then the source-map code fails when calling JSON.parse on that string, which it does here:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/sourcemap/source-map.js#1281
Which, in fact, comes from an upstream repo on github:
https://github.com/mozilla/source-map/blob/master/lib/source-map-consumer.js

The code responsible for using the source-map-consumer is _fetchSourceMap in the StyleSheetActor in 
\devtools\server\actors\stylesheets.js

It seems to me like we should:
- try/catch JSON.parse calls in the source-map library
- and listen for 404 or errors when fetching the source map and bailing out

Comment 10

[Patrick Brosset <:pbro>]

Filed this issue: https://github.com/mozilla/source-map/issues/225
But we still need to fix the issue in stylesheets.js.
This can be done pretty easily in _fetchSourceMap, the promise returned by fetch actually also provides the contentType of the response.
So we could do something like:

  // Fetching the source map might have failed with a 404 or other. Make
  // sure the source map is somethig that SourceMapConsumer can parse.
  if (contentType !== "text/plain") {
    deferred.reject(new Error("Source map was not found"));
    return;
  }

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems like a pretty serious regression we should fix soon.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1249888 - try/catch SourceMapConsumer to avoid empty rule-view when source map is invalid; r=gl

Review commit: https://reviewboard.mozilla.org/r/36579/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36579/

Comment 14

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d9cf7798f9a&group_state=expanded

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8723478 [details]
MozReview Request: Bug 1249888 - try/catch SourceMapConsumer to avoid empty rule-view when source map is invalid; r=gl

https://reviewboard.mozilla.org/r/36579/#review33491

::: devtools/client/inspector/rules/test/browser_rules_invalid-source-map.js:15
(Diff revision 1)
> +  info("Setting the " + PREF + " pref to true");

I personally prefer to not repeat what can be inferred from the function.

I think we should remove:
L15 - info("Setting the " + PREF + " pref to true");
L21 - info("Selecting the test node");
since we can infer what is being done from reading the code.

::: devtools/client/inspector/rules/test/browser_rules_invalid-source-map.js:39
(Diff revision 1)
> +function verifyLinkText(text, view) {

I prefer to see view as the first argument. That is consistent in some functions we have in the rules view.

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8723478 [details]
MozReview Request: Bug 1249888 - try/catch SourceMapConsumer to avoid empty rule-view when source map is invalid; r=gl

https://reviewboard.mozilla.org/r/36579/#review33497

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #15)
> I personally prefer to not repeat what can be inferred from the function.
> 
> I think we should remove:
> L15 - info("Setting the " + PREF + " pref to true");
> L21 - info("Selecting the test node");
> since we can infer what is being done from reading the code.
I completely agree with you for production code: way better to make the code readable and avoid the extra comment. Comments are hard to keep in sync and tend to become obsolete.

I only agree with you for test code to some extent though: It's also very important to make the code readable, it's code that will have to be maintained too, but the big difference is that with tests, we spend hours reading through logs either locally or on treeherder. So you may have the perfect code, very easily readable, but if you don't have any info() statement in it, then the logs are useless when the test starts failing.
By all means, if the code is easy to read, no need for line/block comments (// or /**/), but a healthy does of info("doing this now") sprinkled at the right places in the test is a huge help!

Having said this, the 2 places you mention don't require info() statements, so I will remove them.
The pref is set before adding the tab, and when adding a tab, there's an info() added already, so we're fine. 
And selectNode already has an info() too, so no need for another one here.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/059d7b0fa764

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/059d7b0fa764

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8723478 [details]
MozReview Request: Bug 1249888 - try/catch SourceMapConsumer to avoid empty rule-view when source map is invalid; r=gl

Approval Request Comment
[Feature/regressing bug #]: This regression was introduced by the fix in bug 1243736. That fix made the Rules panel wait for the resolution of source maps. Which means that if a source map is missing, the panel will never load.
[User impact if declined]: Pages that load a CSS file referencing an invalid or inexistant source map will cause the Rules panel in the inspector to be empty.
[Describe test coverage new/current, TreeHerder]: A new devtools mochitest was added for this fix. The patch has landed on m-c.
[Risks and why]: The fix itself is a simple try/catch block around the code that consumes the source map, to fail silently if it isn't found or invalid. So this is low risk.
[String/UUID change made/needed]: None

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression from 46.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723478 [details]
MozReview Request: Bug 1249888 - try/catch SourceMapConsumer to avoid empty rule-view when source map is invalid; r=gl

Minor regression from 46, needed for dev tools inspector to work correctly with e10s.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/abd4499c985d

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/abd4499c985d

Comment 27

[Hiram J. Perez]

Still fails on 47.0a2 with Error: Connection closed, pending request to server1.conn5.child1/53, type getOriginalLocation failed

Comment 28

[Hiram J. Perez]

Should I need to provide a testcase or sth in order to reopen this bug?, I've tried Aurora & Nightly and I can still reproduce it

Comment 29

[Loic]

(In reply to Hiram J. Perez from comment #28)
> Should I need to provide a testcase or sth in order to reopen this bug?,
> I've tried Aurora & Nightly and I can still reproduce it

Open a new bug report and attach your testcase, then copy here the bug link as follow-up.

Comment 30

[Hiram J. Perez]

Follow-up: https://bugzilla.mozilla.org/show_bug.cgi?id=1260510

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Hiram J. Perez from comment #30)
> Follow-up: https://bugzilla.mozilla.org/show_bug.cgi?id=1260510
Thanks for filing a new bug, this is a different issue. I will comment in this other bug.

Comment 32

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Yes

Actual Results: 
As expected

Comment 33

[Kazi Nuzhat Tasnem]

I have reproduced this bug with Firefox aurora 46.0a2(build ID:20160220004011)on 
windows 7(64 bit)

I have verified this bug as fixed with Firefox nightly 48.0a1(build ID:20160421030302)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0