Closed Bug 1154356 Opened 5 years ago Closed 5 years ago

inspector throws exception and does not display valid CSS

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [polish-backlog])

Attachments

(2 files, 3 obsolete files)

Attached file css-t5.html
Open the attached HTML and then open the inspector.
The inspector throws an exception and does not display the
CSS in the rule view.

A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: computedProp is undefined
Full stack: ElementStyle.prototype.markOverridden@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:352:11
ElementStyle.prototype.markOverriddenAll@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:301:5
ElementStyle.prototype.populate/populated</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/styleinspector/rule-view.js:225:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1217:7
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:953:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:561:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
So far what appears to be going on is that the server returns malformed CSS.
Specifically, in styles.js:

      case Ci.nsIDOMCSSRule.STYLE_RULE:
        form.selectors = CssLogic.getSelectors(this.rawRule);
        form.cssText = this.rawStyle.cssText || "";

Here, form.cssText winds up as 

    --unusual;ident:  red; background:  var(--unusual\;ident);

... but this is missing a "\" in the first property.
It seems to be Declaration::AppendVariableAndValueToString:

  aResult.AppendLiteral("--");
  aResult.Append(aName);

... which should probably do some quoting when needed.
Using nsStyleUtil::AppendEscapedCSSIdent there worked;
working on a test case now.
This fixes the quoting of variable names.
Attachment #8595574 - Flags: review?(cam)
Comment on attachment 8595574 [details] [diff] [review]
quote variable name in Declaration::AppendVariableAndValueToString

Review of attachment 8595574 [details] [diff] [review]:
-----------------------------------------------------------------

The bug isn't inspector specific, so I think it would be better under layout/style/tests/.  In fact, can you add it as a subtest within test_variables.html?  That is where we have a grab-bag of miscellaneous CSS Variables related tests.

::: layout/inspector/tests/test_bug1154356.html
@@ +24,5 @@
> +var domUtils =
> +  CC["@mozilla.org/inspector/dom-utils;1"].getService(CI.inIDOMUtils);
> +var rules = domUtils.getCSSStyleRules(document.getElementById("display"));
> +var rule =
> +  rules.GetElementAt(rules.Count() - 1).QueryInterface(CI.nsIDOMCSSStyleRule);

There's no need to use SpecialPowers to get at the rule.  You can use this:

  <style>
    p { --weird\;name: green ; }
  </style>
  ...
  <script>
    var rule = document.querySelector("style").sheet.cssRules[0];
    is(...);
  </script>

::: layout/style/Declaration.cpp
@@ +1171,5 @@
>  Declaration::AppendVariableAndValueToString(const nsAString& aName,
>                                              nsAString& aResult) const
>  {
>    aResult.AppendLiteral("--");
> +  nsStyleUtil::AppendEscapedCSSIdent(aName, aResult);

AppendEscapedCSSIdent assumes that it is given the entire identifier, so here because we've got a leading "--", while it won't make the output incorrect, it won't necessarily be minimal.  For example of the custom property name is "--1" then we'll get "--\\31 : ..." as output (as it thinks it has a leading digit) but it would be fine as "--1: ...".  So can you construct the entire custom property name to pass in to AppendEscapedCSSIdent?
Attachment #8595574 - Flags: review?(cam)
Thanks for the review.  Here's a new patch that I think addresses all
your comments.  I also added a test for the "leading" digit case.
Attachment #8595574 - Attachment is obsolete: true
Attachment #8595609 - Flags: review?(cam)
Comment on attachment 8595609 [details] [diff] [review]
quote variable name in Declaration::AppendVariableAndValueToString

Review of attachment 8595609 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.  One possible issue:

::: layout/style/test/test_variables.html
@@ +100,5 @@
> +    var test7 = document.getElementById("test7");
> +    test7.textContent = "p { --weird\\;name:  green; }";
> +    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name: green;");
> +    test7.textContent = "p { --0: green; }";
> +    is(test7.sheet.cssRules[0].style.cssText, "--0:  green;");

Is the white space after the colons in the two is() calls correct?  It looks like the first should have two spaces and the second should have one.
Attachment #8595609 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #7)

> ::: layout/style/test/test_variables.html
> @@ +100,5 @@
> > +    var test7 = document.getElementById("test7");
> > +    test7.textContent = "p { --weird\\;name:  green; }";
> > +    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name: green;");
> > +    test7.textContent = "p { --0: green; }";
> > +    is(test7.sheet.cssRules[0].style.cssText, "--0:  green;");
> 
> Is the white space after the colons in the two is() calls correct?  It looks
> like the first should have two spaces and the second should have one.

So sorry -- I don't know what happened, since I ran this test a zillion
times, but when I tried it again just now it failed.  How embarrassing.

Here's what works:

    test7.textContent = "p { --weird\\;name: green; }";
    is(test7.sheet.cssRules[0].style.cssText, "--weird\\;name:  green;");
    test7.textContent = "p { --0: green; }";
    is(test7.sheet.cssRules[0].style.cssText, "--0:  green;");

The inputs here have one space but the expected results have two.
This is some artifact of the cssText code that I didn't investigate, as it
didn't seem important.

I'll attach a new patch momentarily and carry over the r+.
I'm going to send it through "try" tomorrow.
This one really does work.
Attachment #8595609 - Attachment is obsolete: true
(In reply to Tom Tromey :tromey from comment #8)
> The inputs here have one space but the expected results have two.
> This is some artifact of the cssText code that I didn't investigate, as it
> didn't seem important.

It's because we unconditionally output a space after the colon, but the stored token stream value of the variable includes the white space from the declaration; so the value of the custom property is actually
" green" in this case.  I believe this is the expected behaviour.
Attachment #8595621 - Flags: review+
Keywords: checkin-needed
Status: NEW → ASSIGNED
Component: Developer Tools: Inspector → Layout
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Comment on attachment 8595621 [details] [diff] [review]
quote variable name in Declaration::AppendVariableAndValueToString

>Bug 1154356 - quote variable name in Declaration::AppendVariableAndValueToString; r=heycam

As a commit message, I think "escape" makes more sense than "quote" here.
Updated commit message.
Attachment #8595621 - Attachment is obsolete: true
Attachment #8597428 - Flags: review+
Blocks: 1154809
I had to back this out because something in the push was making the recursion.js jsreftest fail frequently on Android debug with the errors shown in the log below. It wasn't clear what was at fault, so I backed out both possible candidates for the sake of expediency.
https://treeherder.mozilla.org/logviewer.html#?job_id=9333091&repo=mozilla-inbound

https://hg.mozilla.org/mozilla-central/rev/caf25344f73e

I've also started Try runs of each patch individually applied to hopefully determine which was really at fault. I will re-land whatever patch comes out innocent :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=807bf45de9bb
Relanded along with a CLOBBER touch for good measure after the try run came back green.
https://hg.mozilla.org/mozilla-central/rev/2dba54f538d0
https://hg.mozilla.org/mozilla-central/rev/479891760c0c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [polish-backlog]
You need to log in before you can comment on or make changes to this bug.