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

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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

Comment 1

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Brian Grinstead [:bgrins]]

Oh, the missing line number in this test case is because titleFinder was declared in a console evaluation

Comment 3

[Brian Grinstead [:bgrins]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: qs-benchmark.html

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.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: Bug 1298230 - Use a more descriptive Syntax Error when querySelector / querySelectorAll fails;

Review commit: https://reviewboard.mozilla.org/r/75448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75448/

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: qs-benchmark.html

Accidentally uploaded a version that used a valid selector string

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, you did post the patch.  I'll take a look tomorrow!

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Brian Grinstead [:bgrins]]

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

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: qs-benchmark.html

Fixed median calculation on perf test

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

Comment 15

[Brian Grinstead [:bgrins]]

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

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Should I switch to that instead of nsPrintfCString?

Yes, please.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8786530 [details]
Bug 1298230 - Use a more descriptive Syntax Error when querySelector / querySelectorAll fails;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75448/diff/1-2/

Comment 19

[Brian Grinstead [:bgrins]]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

The formatting looks fine, thank you!

Comment 21

[Joel Maher ( :jmaher ) (UTC -0800)]

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.

Comment 22

[Brian Grinstead [:bgrins]]

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

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62bde80d230d

Comment 24

[Sebastian Zartner [:sebo]]

Looks like this should be documented at a new page under https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors.

Sebastian