'Add rule' fails on bugzilla <body> elements

RESOLVED FIXED in Firefox 41

Status

Product:
DevTools
Component:
Inspector
Type:
defect
Status:
RESOLVED FIXED
Opened:
4 years ago
Updated:
Last year

People

(Reporter: bgrins, Assigned: gl)

Tracking

Version:
Trunk
Target:
Firefox 41
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

1175661.patch
2.30 KB, patch
pbro
: review+
Details | Diff | Splinter Review 
STR:
Select the body element on a bug, like: https://bugzilla.mozilla.org/show_bug.cgi?id=1166959
Right click -> Add Rule

Expected:

A new rule is created with the class name

Actual:

There is an error:

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:904] protocol.js:907


It looks like maybe it's caused by some whitespace in the class name:

 <body onload=""
        class="bugzilla-mozilla-org
               skin-Mozilla bz_bug bz_status_ASSIGNED bz_product_Firefox bz_component_Developer_Tools&#X3A;_Inspector bz_bug_1166959 bz_gravatar yui-skin-sam">
I think we have try/catches around almost all instances of el.querySelector, el.matches, sheet.insertRule, ... in the actor. Basically, anywhere we evaluate a selector that comes from user input.
It looks like we don't have one in addNewRule. It might be all it takes to fix this issue.
But of course, we don't really have a good way to report problems to the user.
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #1)
> I think we have try/catches around almost all instances of el.querySelector,
> el.matches, sheet.insertRule, ... in the actor. Basically, anywhere we
> evaluate a selector that comes from user input.
> It looks like we don't have one in addNewRule. It might be all it takes to
> fix this issue.
> But of course, we don't really have a good way to report problems to the
> user.

We originally thought escaping all the selectors will avoid all syntax error. We can add in a try catch and default to using the node tag as the selector.
I didn't check the code closely enough, the problems seems to come from this part of the AddNewRule method:

selector = "." + rawNode.className.split(" ").map(c => CSS.escape(c)).join(".");

This assumes that className contains a list of classes separated by exactly 1 space.
We should replace this with:

selector = "." + [...rawNode.classList].map(c => CSS.escape(c)).join(".");

(but we should check if classList works in XUL and SVG documents first too).
Assignee: nobody → gabriel.luong
Posted patch 1175661.patch — — Splinter Review
Attachment #8624086 - Flags: review?(pbrosset)
Comment on attachment 8624086 [details] [diff] [review]
1175661.patch

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

r=me if try is green *and* if this works with SVG nodes and in XUL documents (I have a vague memory of classList being weird in SVG, or maybe it was the className property, or class attribute, I can't remember).
Attachment #8624086 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #7)
> Comment on attachment 8624086 [details] [diff] [review]
> 1175661.patch
> 
> Review of attachment 8624086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if try is green *and* if this works with SVG nodes and in XUL documents
> (I have a vague memory of classList being weird in SVG, or maybe it was the
> className property, or class attribute, I can't remember).

I did test classList with SVG and XUL and it works!
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/a36b8e56b6f7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox41: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.