Last Comment Bug 631615 - Suppress CSS parser diagnostics in ParseSelectorString
: Suppress CSS parser diagnostics in ParseSelectorString
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla2.0b12
Assigned To: Zack Weinberg (:zwol)
:
: Jet Villegas (:jet)
Mentors:
http://bugs.jquery.com/ticket/7535
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-04 12:00 PST by Zack Weinberg (:zwol)
Modified: 2011-03-21 22:03 PDT (History)
4 users (show)
philringnalda: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.17-fixed
.19-fixed


Attachments
test case (803 bytes, text/html)
2011-02-04 12:00 PST, Zack Weinberg (:zwol)
no flags Details
patch (1.40 KB, patch)
2011-02-04 12:12 PST, Zack Weinberg (:zwol)
dbaron: review+
Details | Diff | Splinter Review
patch, now with automated test (4.88 KB, patch)
2011-02-04 13:40 PST, Zack Weinberg (:zwol)
dbaron: review+
dbaron: approval2.0+
Details | Diff | Splinter Review
patch v3 (4.80 KB, patch)
2011-02-04 14:17 PST, Zack Weinberg (:zwol)
dbaron: review+
dbaron: approval2.0+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
dveditz: approval1.9.0.next-
Details | Diff | Splinter Review

Description Zack Weinberg (:zwol) 2011-02-04 12:00:47 PST
Created attachment 509844 [details]
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) 2011-02-04 12:12:54 PST
Created attachment 509849 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-02-04 12:21:39 PST
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-8 2011-02-04 13:23:47 PST
Comment on attachment 509849 [details] [diff] [review]
patch

r=dbaron
Comment 4 Zack Weinberg (:zwol) 2011-02-04 13:40:16 PST
Created attachment 509879 [details] [diff] [review]
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) 2011-02-04 13:42:24 PST
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-8 2011-02-04 13:49:17 PST
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) 2011-02-04 14:17:59 PST
Created attachment 509896 [details] [diff] [review]
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) 2011-02-05 08:55:28 PST
http://hg.mozilla.org/mozilla-central/rev/9d810f5ea61c
Comment 9 Daniel Veditz [:dveditz] 2011-02-23 10:34:46 PST
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) 2011-03-21 20:50:23 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.