1040199 - Adding a new rule for a node whose id contains invalid character throws

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

with this exception :

DOMException [SyntaxError: "An invalid or illegal string was specified"
code: 12
nsresult: 0x8053000c
location: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/styles.js:590]

And then nothing happens on the UI.

I think if the id is invalid, we should go ahead and use class next, or maybe use the unqiue selector

Comment 1

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

Any examples on how to reproduce this?

Comment 2

[Girish Sharma [:Optimizer]]

One easy way: Go to facebook, open inspect, type # in search box and you will see some id with ":" in them and likewise. Select those nodes (no idea how, manual search only) and then add rule.

Or else, have a dummy web page with a node with an invalid id like "asd@asd:asd"

Comment 3

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

Attachment: 1040199.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1688c4be362d

Comment 4

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

We introduce a try catch similar to what we did in modifySelector because there is a chance for a failure in insertRule when you insert a rule with syntax error (invalid selector name). We try to handle such error by finding an unique css name.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8613282 [details] [diff] [review]
1040199.patch

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

::: toolkit/devtools/server/actors/styles.js
@@ +883,5 @@
> +    let index;
> +    try {
> +      index = sheet.insertRule(selector + " {}", cssRules.length);
> +    } catch(e) {
> +      index = sheet.insertRule(CssLogic.findCssSelector(rawNode) + " {}",

Using findCssSelector here is not such a bad idea after all, we could use this to replace the whole if/elseif/else above that checks for ids and then classes and then tagnames, because I believe that's what CssLogic.findCssSelector does, and it does it better probably.

My problem with this patch though is that I think you should either fix the if/elseif/else block so it never throws, or using findCssSelector instead of it, but not use both with a try catch.

Option 1, fix the existing code:

if (raNode.id) {
  selector = "#" + CSS.escape(rawNode.id);
} else if (rawNode.className) {
  selector = "." + rawNode.className.split(" ").map(c => CSS.escape(c)).join(".");
} else {
  selector = rawNode.tagName.toLowerCase();
}

Or something like the above anyway, indeed the problem is simply that the id and classes aren't being escaped properly.

Option 2, use findCssSelector instead of the existing code:

let selector = CssLogic.findCssSelector(rawNode);

and remove the whole if/elseif/else and no need for a try/catch.

Up to you what you choose, I like findCssSelector, but it may have side effects I didn't see coming. Using CSS.escape is such a small and quick fix that I'd R+ a patch with just this + the test.

Comment 6

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

Attachment: 1040199.patch [1.0]

findCssSelector does have some unexpected side effects since it tries to find an unique CSS selector. I stuck with option 1.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=419300086a46

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8613643 [details] [diff] [review]
1040199.patch [1.0]

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

Looks good to me now.

Comment 8

[Pulsebot]

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

Comment 9

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

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