Closed Bug 1298230 Opened 6 years ago Closed 6 years ago

SyntaxError thrown during querySelector doesn't provide additional information about why it was thrown (selector and node)

Categories

(DevTools :: Console, defect, P3)

45 Branch
defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: mcomella, Assigned: bgrins)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 2 obsolete files)

I don't know exactly what is going on here so please rename once it's investigated.

STR:
* Clone my fathom REPL test: https://github.com/mcomella/fathom-repl
* `git checkout 2786cf70bb8cd2afb986ac00478b6ced34e25d31` (this probably works with all revisions, but just to be safe)
* `npm install && npm run build && open public/repl.html` (`open` assumes OS X)
* Open the browser console
* Enter the following into the console:

var titleFinder = ruleset(
    // Give any title tag a score of 1, and tag it as title-ish:
    rule(dom("title"), node => [{score: 1, flavor: 'titley'}]),

    // Give any OpenGraph meta tag a score of 2, and tag it as title-ish as well:
    rule(dom("meta[og:title]"), node => [{score: 2, flavor: 'titley'}]),

    // Take all title-ish things, and punish them if they contain
    // navigational claptrap like colons or dashes:
    rule(flavor("titley"), node => [{score: containsColonsOrDashes(node.element) ? .5 : 1}])
);

* Enter the following into the console: `titleFinder.score(document)`.

---
On Firefox: "SyntaxError: An invalid or illegal string was specified" (note: no line number)

On Chrome: "out.js:235 Uncaught DOMException: Failed to execute 'querySelectorAll' on 'Document': 'meta[og:title]' is not a valid selector."
I can see the error message with this test case: `data:text/html,<script>var foo = document.querySelector("meta[og:title]");</script>`.  Although I *do* see a line number so that might be another bug.

We should be more useful here and chrome's message seems good.  Here's the current logging (uses NS_ERROR_DOM_SYNTAX_ERR): https://dxr.mozilla.org/mozilla-central/source/dom/base/nsINode.cpp?#2709-2710.  Might also be a good candidate for the 'learn more' MDN link.
Oh, the missing line number in this test case is because titleFinder was declared in a console evaluation
Hi Boris.  There's a suggestion here to change our error message when passing an invalid string into query selector from something like:

  "SyntaxError: An invalid or illegal string was specified"

to:

  "SyntaxError: Failed to execute 'querySelectorAll' on 'Document': 'meta[og:title]' is not a valid selector."

or maybe to this since it appears it'd be easier to plug into ParseSelectorList:

  "SyntaxError: 'meta[og:title]' is not a valid selector."

First of all, does that seem reasonable and are you OK with a change like that?

Secondly, any ideas how to implement this?  In particular, I don't see any example of passing a variable into any of the DOM4_MSG_DEF error messages.  And also even though this is still an NS_ERROR_DOM_SYNTAX_ERR, that's used for a lot of different errors so I guess we'd need to come up with a new error type if we wanted an alternate string.
Flags: needinfo?(bzbarsky)
Priority: -- → P3
Summary: Not useful SyntaxError in FF but useful DOMException in Chrome → SyntaxError thrown during querySelector doesn't provide additional information about why it was thrown (selector and node)
> First of all, does that seem reasonable and are you OK with a change like that?

Generally speaking, yes.  The one caveat is that I'd want to see performance numbers before/after the change, because the throwing case of querySelectorAll is somewhat performance-sensitive because of the way jQuery uses querySelectorAll.  I expect it should be fine, since throwing exceptions is not that cheap to start with, but would like to see measurements.

We could certainly include the strings "querySelectorAll" and "Document" in there if we just moved the throwing to the callers of ParseSelectorList.  Or we could try to change bindings to always output that sort of thing; that would be out of scope for this bug, and involve some tradeoffs in terms of lots more codesize and whatnot that might be better avoided.  We used to have something sort of like that and ripped it out...

> Secondly, any ideas how to implement this?

Instead of doing this:

  aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

do this:

  aRv.ThrowDOMException(
    NS_ERROR_DOM_SYNTAX_ERR, 
    nsPrintfCString("'%s' is not a valid selector",
                    NS_ConvertUTF16toUTF8(aSelectorString).get()));

or so.  You will also want to change the code on the codepath that asserts that ParseSelectorString returned a NS_ERROR_DOM_SYNTAX_ERR to make a ThrowDOMException call like that as well.  That's OK to do for an ErrorResult that just has an nsresult in it.
Flags: needinfo?(bzbarsky)
Assignee: nobody → bgrinstead
Attached file qs-benchmark.html (obsolete) —
Here's a first attempt at a perf test.  What it does is attempt 1000 querySelector calls with a bad selector inside a try / catch.  It does that for 100 cycles then reports the average, median, and stddev.

Locally with my patch applied it does look like it's slower although it's hard to make a confident statement about how much since the numbers jump around between reloads (maybe typically 3.2 seconds median per cycle with the patch applied and 2.5 seconds median per cycle without).  I don't know how much variability just has to do with needing to try/catch.  If you know of any of the talos tests that will hit this I can do a perfherder comparison.

For comparison, Chrome Canary 55 is showing a median around 6 seconds and a much higher stddev.
Attached file qs-benchmark.html (obsolete) —
Accidentally uploaded a version that used a valid selector string
Attachment #8786529 - Attachment is obsolete: true
Chrome's exception-handling code is all sorts of bad; it can't JIT try/catch at its highest jit level, and it's actual throwing is slower than ours too.

I don't think we have talos tests for this, but we _have_ run into jQuery benchmarks that involve this exact exception.  If you want to post the patch you were testing, I'll take a look at the test and the patch (and their interaction) tomorrow....
Oh, you did post the patch.  I'll take a look tomorrow!
Comment on attachment 8786530 [details]
Bug 1298230 - Use a more descriptive Syntax Error when querySelector / querySelectorAll fails;

https://reviewboard.mozilla.org/r/75448/#review73680

::: devtools/client/webconsole/test/browser_webconsole_output_04.js:62
(Diff revision 1)
>  
>    // 4
>    {
>      input: "testDOMException()",
> -    output: 'DOMException [SyntaxError: "An invalid or illegal string was ' +
> -            'specified"',
> +    output: "DOMException [SyntaxError: \"'foo;()bar!' is not a valid selector\"",
> +    printOutput: "SyntaxError: 'foo;()bar!' is not a valid selector",

This part confuses me a bit.  The old string had double quotes in the actual string.  The new one does not.  Why is that a reasonable change here?

::: dom/base/nsINode.cpp:2711
(Diff revision 1)
>    nsCSSSelectorList* selectorList = nullptr;
>    bool haveCachedList = cache.GetList(aSelectorString, &selectorList);
>    if (haveCachedList) {
>      if (!selectorList) {
>        // Invalid selector.
> -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      aRv.ThrowDOMException(

OK, I did some measurements and I'm seeing the same thing as you: this patch gives a 30% slowdown in querySelector performance for invalid selectors.

Profiling suggests the problem is actually just that nsPrintfCString is rather slow.  I think we have two options:

1. Replace the nsPrintfCString bit with something like: NS_LITERAL_CSTRING("'") + NS_ConvertUTF16toUTF8(aSelectorString) + NS_LITERAL_CSTRING("' is not a valid selector").  This seems to be only a 10% or less slowdown in my testing, but I'd appreciate it if you could double-check that.
2. Cache the exception message in the SelectorCache.  That adds a bunch more complication, though, and doesn't seem _that_ much faster than just building the message directly, so I'm not sure it's worth it.

r=me with the test bit explained and the performance thing addressed.

Oh, and as far as the testcase goes, please replace the `sort()` calls with `sort((a,b) => a-b)` because otherwise you're sorting by alphabetical order of the strings, which can do weird things near powers-of-10 boundaries.
Attachment #8786530 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8786530 [details]
> Bug 1298230 - Use a more descriptive Syntax Error when querySelector /
> querySelectorAll fails;
> 
> https://reviewboard.mozilla.org/r/75448/#review73680
> 
> ::: devtools/client/webconsole/test/browser_webconsole_output_04.js:62
> (Diff revision 1)
> >  
> >    // 4
> >    {
> >      input: "testDOMException()",
> > -    output: 'DOMException [SyntaxError: "An invalid or illegal string was ' +
> > -            'specified"',
> > +    output: "DOMException [SyntaxError: \"'foo;()bar!' is not a valid selector\"",
> > +    printOutput: "SyntaxError: 'foo;()bar!' is not a valid selector",
> 
> This part confuses me a bit.  The old string had double quotes in the actual
> string.  The new one does not.  Why is that a reasonable change here?

The new one does have double quotes, but they are escaped because I switched the outer quotes from ' to ".  I think I'll switch the outer quotes to ` so that neither single nor double quotes need to be escaped and the diff is easier to read.
(In reply to Boris Zbarsky [:bz] from comment #10)
> Oh, and as far as the testcase goes, please replace the `sort()` calls with
> `sort((a,b) => a-b)` because otherwise you're sorting by alphabetical order
> of the strings, which can do weird things near powers-of-10 boundaries.

Interesting, I grabbed this code from talos: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/pageloader/chrome/report.js#97.  Joel, does this median calculation get used for tracking / reporting times?  Because it is error prone in the case of [1,2,3,4,5,6,7,8,9,10,11].sort().
Flags: needinfo?(jmaher)
Attached file qs-benchmark.html
Fixed median calculation on perf test
Attachment #8786531 - Attachment is obsolete: true
> The new one does have double quotes

The new "output" string does.  I'm talking about the "printOutput" string.  Yay mozreview not making it very clear what's being quoted....
(In reply to Boris Zbarsky [:bz] from comment #14)
> > The new one does have double quotes
> 
> The new "output" string does.  I'm talking about the "printOutput" string. 
> Yay mozreview not making it very clear what's being quoted....

Oh I see, that change was unintentional so I'll re-add them.  Test passes both with and without the quotes bc it uses a helper `waitForMessages` that accepts partial matches.
(In reply to Boris Zbarsky [:bz] from comment #10)
> ::: dom/base/nsINode.cpp:2711
> (Diff revision 1)
> >    nsCSSSelectorList* selectorList = nullptr;
> >    bool haveCachedList = cache.GetList(aSelectorString, &selectorList);
> >    if (haveCachedList) {
> >      if (!selectorList) {
> >        // Invalid selector.
> > -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> > +      aRv.ThrowDOMException(
> 
> OK, I did some measurements and I'm seeing the same thing as you: this patch
> gives a 30% slowdown in querySelector performance for invalid selectors.
> 
> Profiling suggests the problem is actually just that nsPrintfCString is
> rather slow.  I think we have two options:
> 
> 1. Replace the nsPrintfCString bit with something like:
> NS_LITERAL_CSTRING("'") + NS_ConvertUTF16toUTF8(aSelectorString) +
> NS_LITERAL_CSTRING("' is not a valid selector").  This seems to be only a
> 10% or less slowdown in my testing, but I'd appreciate it if you could
> double-check that.

I see maybe a 15-20% improvement with that change.  Should I switch to that instead of nsPrintfCString?

> 2. Cache the exception message in the SelectorCache.  That adds a bunch more
> complication, though, and doesn't seem _that_ much faster than just building
> the message directly, so I'm not sure it's worth it.

Yeah, and it only helps if the same selector is passed in - if it's frequently changing then it won't hit that case.
Flags: needinfo?(bzbarsky)
> Should I switch to that instead of nsPrintfCString?

Yes, please.
Flags: needinfo?(bzbarsky)
I'm not sure on formatting guidelines, tried to follow what the rest of the file was doing in https://reviewboard.mozilla.org/r/75448/diff/2#3 but let me know if it should be changed
The formatting looks fine, thank you!
bgrins, thanks for the heads up- regarding official talos calculations, we do it outside of the pageloader addon, but the code in pageloader addon is used for dumping a human friendly summary- we should fix that and I filed bug 1299758 to track that.
Flags: needinfo?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/62bde80d230d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Looks like this should be documented at a new page under https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors.

Sebastian
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.