631615 - Suppress CSS parser diagnostics in ParseSelectorString

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: test case

matchesSelector, querySelectorAll, etc are specified to throw a JS exception if their CSS selector argument triggers a parse error.  At present, the CSS parser reports the parse error directly to the error console before the exception is thrown.  If the exception is deliberately triggered and caught (as part of a feature probe, for instance) people get annoyed with the console spew -- the URL points to a jQuery bug report over this, for instance.

Per the discussion in m.d.t.layout (http://groups.google.com/group/mozilla.dev.tech.layout/browse_thread/thread/42e3b6d9e383b6a1 ), the location information in the uncaught-exception notification is actually _more_ accurate than what we put in the CSS parser's diagnostics in this case, and since selector strings are usually pretty short, I think we can do without the parser's diagnostic.  Accordingly, the patch I will shortly attach just turns them off altogether.

A superior alternative would be to have a way to delay the parse error report till after we know the exception is uncaught, but we can't do that now.  I'll file a separate bug for that.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: patch

Here's the patch.  The actual change is one line, but I felt an explanatory comment was in order.

There is no testsuite addition because I don't know how to check for "no new messages were added to the error console since time X".  If I could get a little help with that, that would be great (I presume it would have to be a privileged mochitest of some sort).

To make the jQuery people completely happy, this change would have to be backported to all branches that have these APIs (even those that implement the older "return false on failure" behavior).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Zack, see content/events/test/test_bug489671.html for an example of a test that sets up a console listener...  though that test expects to get a message.  Expecting to NOT get one is harder.  You could try logging a message to the console service yourself after the querySelector call and seeing whether you get any messages before you get the message you logged?

I don't think we need to backport this to branches, fwiw.  It's not like jquery itself has to deal with these messages.  It's just users that complain about them.... as long a they update, they'll be fine.  ;)

Comment 3

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

Comment on attachment 509849 [details] [diff] [review]
patch

r=dbaron

Comment 4

[Zack Weinberg (:zwol)]

Attachment: patch, now with automated test

(In reply to comment #2)
> Zack, see content/events/test/test_bug489671.html for an example of a test
> that sets up a console listener...

Thanks.  Have now implemented a test.

> though that test expects to get a message. 
> Expecting to NOT get one is harder.  You could try logging a message to the
> console service yourself after the querySelector call and seeing whether you
> get any messages before you get the message you logged?

Yeah, that's what I ended up doing.  It is asynchronous enough that my original idea of incrementing a message count in the listener and then checking for count==0 at the end of the test didn't work.

> I don't think we need to backport this to branches, fwiw.  It's not like
> jquery itself has to deal with these messages.  It's just users that 
> complain about them.... as long a they update, they'll be fine.  ;)

I can see people grumbling about how they have to test with 3.6 and 3.5 and maybe even 3.0 as well as 4.x and these messages, augh!  Note that in the jQuery bug people are threatening to not update past jq 1.4.2 because of this; it seems to be Serious Business for them anyway.

Comment 5

[Zack Weinberg (:zwol)]

Comment on attachment 509879 [details] [diff] [review]
patch, now with automated test

carrying r+ forward from crossed-wire review of previous patch.  Thanks dbaron.

Comment 6

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

Comment on attachment 509879 [details] [diff] [review]
patch, now with automated test

>+// override window.onerror so it won't see our exceptions
>+window.onerror = function() {}

Rather than doing this, could you just explicitly post a message to the console?  (If the test ever fails in the future, it could be valuable not to have this.)

r=dbaron with that

Comment 7

[Zack Weinberg (:zwol)]

Attachment: patch v3

(In reply to comment #6)
> Rather than doing this, could you just explicitly post a message to the
> console?  (If the test ever fails in the future, it could be valuable not to
> have this.)

Sure, that actually makes things tidier in the listener too.

I have to re-request a2.0.  Also, I'm not going to be able to watch the tree today, so I'm marking checkin-needed and perhaps someone can take this as a ride-along.

Comment 8

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/9d810f5ea61c

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 509896 [details] [diff] [review]
patch v3

Definitely not taking this on the unsupported 1.9.0.x

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 10

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52e0d3ae78dd
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b5a151fc0174

I think I may have missed 2.16 and 1.18 -- sorry about that.  The fixed-.17 and fixed-.19 flag values don't exist yet so I can't set them.