1154356 - inspector throws exception and does not display valid CSS

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tom Tromey :tromey]

Attachment: 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

Comment 1

[Tom Tromey :tromey]

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.

Comment 2

[Tom Tromey :tromey]

It seems to be Declaration::AppendVariableAndValueToString:

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

... which should probably do some quoting when needed.

Comment 3

[Tom Tromey :tromey]

Using nsStyleUtil::AppendEscapedCSSIdent there worked;
working on a test case now.

Comment 4

[Tom Tromey :tromey]

Attachment: quote variable name in Declaration::AppendVariableAndValueToString

This fixes the quoting of variable names.

Comment 5

[Cameron McCormack (:heycam)]

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?

Comment 6

[Tom Tromey :tromey]

Attachment: quote variable name in Declaration::AppendVariableAndValueToString

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.

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Tom Tromey :tromey]

(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.

Comment 9

[Tom Tromey :tromey]

Attachment: quote variable name in Declaration::AppendVariableAndValueToString

This one really does work.

Comment 10

[Cameron McCormack (:heycam)]

(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.

Comment 11

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a944723581d

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[Tom Tromey :tromey]

Attachment: escape variable name in Declaration::AppendVariableAndValueToString

Updated commit message.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc645744a11

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dba54f538d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/479891760c0c

Comment 17

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

Relanded along with a CLOBBER touch for good measure after the try run came back green.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dba54f538d0
https://hg.mozilla.org/mozilla-central/rev/479891760c0c