Suppressing CSS error reporting when using the DOM selector APIs
Boris Zbarsky
throw when the selector can't be parsed. Given that, do we still want
to report parse errors as warnings to the error console? Doing this
apparently causes at least some people angst: see
http://bugs.jquery.com/ticket/7535
(I can see the argument for reporting it, since the DOM exception gives
no indication of where the error occurred, by the way.)
-Boris
Zack Weinberg
> The DOM selector APIs (matchesSelector, querySelector, querySelectorAll)
> throw when the selector can't be parsed. Given that, do we still want to
> report parse errors as warnings to the error console? Doing this
> apparently causes at least some people angst: see
> http://bugs.jquery.com/ticket/7535
I am generally for changes that reduce the amount of useless chatter in
the error console.
> (I can see the argument for reporting it, since the DOM exception gives
> no indication of where the error occurred, by the way.)
We could put that information into the DOM exception, couldn't we? And
pull it back out and log it if and only if the exception is not caught.
(While I'm complaining, this isn't exactly the same problem, but the
error console messages we produce for uncaught JS exceptions that arise
from DOM APIs are full of XPCOM jargon, so they give the impression that
we're reporting some kind of internal failure rather than a bug in web
page JavaScript.)
zw
Boris Zbarsky
> We could put that information into the DOM exception, couldn't we?
Not without major surgery. Right now, what we have to work with is an
nsresult.
> And pull it back out and log it if and only if the exception is not caught.
If we put the info in the DOM exception somehow, I'd think just the
normal uncaught exception reporting would report everything that's needed.
> (While I'm complaining, this isn't exactly the same problem, but the
> error console messages we produce for uncaught JS exceptions that arise
> from DOM APIs are full of XPCOM jargon, so they give the impression that
> we're reporting some kind of internal failure rather than a bug in web
> page JavaScript.)
Is this just for the ones going through XPConnect goop, or also for the
quickstubbed ones?
-Boris
Zack Weinberg
> On 1/28/11 7:33 PM, Zack Weinberg wrote:
>> We could put that information into the DOM exception, couldn't we?
>
> Not without major surgery. Right now, what we have to work with is an
> nsresult.
Bother.
Hang on, though -- we don't produce meaningful position information
for these warnings anyway (because the CSS parser is being given the
owner document of the base DOM node for the query rather than the
calling JavaScript, and line number zero --
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5416
) and the error console log you get if you don't catch the exception *does*:
[09:01:04.453] uncaught exception: [Exception... "An invalid or illegal
string was specified" code: "12" nsresult: "0x8053000c
(NS_ERROR_DOM_SYNTAX_ERR)" location: "file:///tmp/test.html Line: 14"]
Of course that's full of the XPCOM jargon I was complaining about, but
it does point you right at the offending call, and we could easily
improve it a bit (starting with pulling out the location information and
putting it into the console's file/line display).
So on that basis I support disabling CSS-parser diagnostics for all the
entry points that take a short string and throw an exception on failure.
(Which might only be ParseSelectorString, I don't remember anymore.)
>> (While I'm complaining, this isn't exactly the same problem, but the
>> error console messages we produce for uncaught JS exceptions that arise
>> from DOM APIs are full of XPCOM jargon, so they give the impression that
>> we're reporting some kind of internal failure rather than a bug in web
>> page JavaScript.)
>
> Is this just for the ones going through XPConnect goop, or also for the
> quickstubbed ones?
Dunno. The thing I quoted above is for matchesSelector, which I think
is going through xpconnect goop; can you think of a quickstubbed API
that throws exceptions offhand?
zw
Boris Zbarsky
> Hang on, though -- we don't produce meaningful position information
> for these warnings anyway (because the CSS parser is being given the
> owner document of the base DOM node for the query rather than the
> calling JavaScript, and line number zero --
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsGenericElement.cpp#5416
> ) and the error console log you get if you don't catch the exception
> *does*:
Yes, but for the CSS parser we often produce meaningful information
about what the error is (though not where it happened, in this case).
> [09:01:04.453] uncaught exception: [Exception... "An invalid or illegal
> string was specified" code: "12" nsresult: "0x8053000c
> (NS_ERROR_DOM_SYNTAX_ERR)" location: "file:///tmp/test.html Line: 14"]
>
> Of course that's full of the XPCOM jargon I was complaining about
Apart from the "nsresult" bit, the hex code for the nsresult, and the
"NS_ERROR" prefix, is there other jargon?
> and we could easily improve it a bit (starting with pulling out the location information and
> putting it into the console's file/line display).
Hmm. We have bugs on that, for what it's worth; it's not that easy last
I checked.
> So on that basis I support disabling CSS-parser diagnostics for all the
> entry points that take a short string and throw an exception on failure.
> (Which might only be ParseSelectorString, I don't remember anymore.)
Indeed, it's just ParseSelectorString.
> Dunno. The thing I quoted above is for matchesSelector, which I think is
> going through xpconnect goop; can you think of a quickstubbed API that
> throws exceptions offhand?
matchesSelector is quickstubbed.
-Boris
Mike Shaver
>> [09:01:04.453] uncaught exception: [Exception... "An invalid or illegal
>> string was specified" code: "12" nsresult: "0x8053000c
>> (NS_ERROR_DOM_SYNTAX_ERR)" location: "file:///tmp/test.html Line: 14"]
>>
>> Of course that's full of the XPCOM jargon I was complaining about
>
> Apart from the "nsresult" bit, the hex code for the nsresult, and the
> "NS_ERROR" prefix, is there other jargon?
The extra [Exception...] and code 12, at least. In general we also
don't say which method is being called incorrectly, IIRC.
>> and we could easily improve it a bit (starting with pulling out the
>> location information and
>> putting it into the console's file/line display).
>
> Hmm. We have bugs on that, for what it's worth; it's not that easy last I
> checked.
Seems like a last-error singleton would work, in concert with our
single-threadedness?
Mike
Boris Zbarsky
> On Mon, Jan 31, 2011 at 6:16 AM, Boris Zbarsky<bzba...@mit.edu> wrote:
>>> [09:01:04.453] uncaught exception: [Exception... "An invalid or illegal
>>> string was specified" code: "12" nsresult: "0x8053000c
>>> (NS_ERROR_DOM_SYNTAX_ERR)" location: "file:///tmp/test.html Line: 14"]
>>>
>>> Of course that's full of the XPCOM jargon I was complaining about
>>
>> Apart from the "nsresult" bit, the hex code for the nsresult, and the
>> "NS_ERROR" prefix, is there other jargon?
>
> The extra [Exception...] and code 12, at least.
"12" is the per-spec value of DOM_SYNTAX_ERR, which is the exception
being thrown (see the DOMException interface in DOM Core). It's not
xpcom anything.
> In general we also don't say which method is being called incorrectly, IIRC.
Yes, in this case. I'm not saying this exception message doesn't have
major issues, which we should fix! It's just that most of those issues
have nothing to do with "xpcom".
>> Hmm. We have bugs on that, for what it's worth; it's not that easy last I
>> checked.
>
> Seems like a last-error singleton would work, in concert with our
> single-threadedness?
I think the hard part was finding all the fiddly xpconnect bits that
needed fixing (well, and the fact that exception throwing can nest and
such).
-Boris
Zack Weinberg
> On 1/29/11 12:12 PM, Zack Weinberg wrote:
>> Hang on, though -- we don't produce meaningful position information
>
> Yes, but for the CSS parser we often produce meaningful information
> about what the error is (though not where it happened, in this case).
That's true, and I would like to preserve that information, but I don't
see a good way to do it without much more drastic changes than are
practical, if we want to make the spurious messages go away. (The
*least* invasive thing I can think of is to define a whole bunch of
NS_ERROR_DOM_SYNTAX_CSS_whatever subcodes, but then we'd have no way of
grouping them together when they all needed to be treated as
DOM_SYNTAX_ERR per spec.)
In this case, selector strings are usually pretty short and I've seen
far more people complaining about useless noise in the error console
than I have complaining about not being able to figure out what was
wrong with their CSS selectors. (JS is a different story.)
[... will respond to deleted stuff here downthread ...]
>> So on that basis I support disabling CSS-parser diagnostics for all the
>> entry points that take a short string and throw an exception on failure.
>> (Which might only be ParseSelectorString, I don't remember anymore.)
>
> Indeed, it's just ParseSelectorString.
In that case the patch might be as simple as removing any OUTPUT_ERROR
calls from ParseSelectorString and its subroutines.
>> Dunno. The thing I quoted above is for matchesSelector, which I think is
>> going through xpconnect goop; can you think of a quickstubbed API that
>> throws exceptions offhand?
>
> matchesSelector is quickstubbed.
Ok, then can you suggest a *non*-quickstubbed API that throws
exceptions? I'm pretty sure it does do the same thing, but I looked for
the code that handles the conversion from nsresult to JS exception (in
general) and didn't find it.
zw
Zack Weinberg
> On 1/31/11 11:35 AM, Mike Shaver wrote:
>> On Mon, Jan 31, 2011 at 6:16 AM, Boris Zbarsky<bzba...@mit.edu> wrote:
>>> Apart from the "nsresult" bit, the hex code for the nsresult, and the
>>> "NS_ERROR" prefix, is there other jargon?
>>
>> The extra [Exception...] and code 12, at least.
>
> "12" is the per-spec value of DOM_SYNTAX_ERR, which is the exception
> being thrown (see the DOMException interface in DOM Core). It's not
> xpcom anything.
>
>> In general we also don't say which method is being called incorrectly,
>> IIRC.
>
> Yes, in this case. I'm not saying this exception message doesn't have
> major issues, which we should fix! It's just that most of those issues
> have nothing to do with "xpcom".
I suppose I should have said "internal jargon" rather than "XPCOM jargon".
My idea of a makes-sense-to-web-developers version of this message would be
Uncaught exception, thrown by matchesSelector:
DOM_SYNTAX_ERR (An invalid or illegal string was specified)
File: file:///tmp/test.html Line: 14
with the file and line properly hooked into the error console's location
machinery.
>>> Hmm. We have bugs on that, for what it's worth; it's not that easy
>>> last I checked.
>>
>> Seems like a last-error singleton would work, in concert with our
>> single-threadedness?
>
> I think the hard part was finding all the fiddly xpconnect bits that
> needed fixing (well, and the fact that exception throwing can nest and
> such).
As I mentioned a bit upthread, I looked for the code that does the
nsresult-to-JS-exception conversion and didn't find it. Well, I found
things that *might* be it, but they were doing all sorts of complicated
stuff that didn't make sense to me and so I wasn't sure.
It seems like that code is already doing almost what it needs to,
though. For the exception we've been kicking around, it fabricates a
DOMException object with separate properties for the location, line
number, error code name, and error message. It ought to also record the
name of the function that threw the error; I could believe that that's a
pain, though.
The real problem as I see it is that the error console (and the "web
console") don't break the exception down appropriately; they seem to be
just calling toString on the exception object and dumping the result
into the log window. *That* should be fixable without mucking with
XPConnect. I would like to think so anyway.
zw
Boris Zbarsky
>> Indeed, it's just ParseSelectorString.
>
> In that case the patch might be as simple as removing any OUTPUT_ERROR
> calls from ParseSelectorString and its subroutines.
Hmm. Yeah, that would work; there are no subroutines to remove from, in
fact.
>> matchesSelector is quickstubbed.
>
> Ok, then can you suggest a *non*-quickstubbed API that throws
> exceptions? I'm pretty sure it does do the same thing, but I looked for
> the code that handles the conversion from nsresult to JS exception (in
> general) and didn't find it.
Actually, the simplest thing if you have a tree is to remove this line:
'nsIDOMNSElement.*'
from dom_quickstubs.qsconf and rebuild, then retest matchesSelector.
Finding a non-quickstubbed method and then finding a way to make it fail
is a bit of a pain...
At this point, I expect it behaves the same as a quickstubbed method,
though.
-Boris
Boris Zbarsky
> It seems like that code is already doing almost what it needs to,
> though. For the exception we've been kicking around, it fabricates a
> DOMException object with separate properties for the location, line
> number, error code name, and error message.
No, XPConnect takes such an object as _input_. Then it converts it to a
string and throws that string to JS, iirc. Yes, this is absolutely
dumb... and maybe I'm misremembering. But something stupid like that
happens.
> The real problem as I see it is that the error console (and the "web
> console") don't break the exception down appropriately
They do if you hand them an exception with something other than just a
string in it.
-Boris
Zack Weinberg
> On 1/31/11 6:19 PM, Zack Weinberg wrote:
>> It seems like that code is already doing almost what it needs to,
>> though. For the exception we've been kicking around, it fabricates a
>> DOMException object with separate properties for the location, line
>> number, error code name, and error message.
>
> No, XPConnect takes such an object as _input_. Then it converts it to a
> string and throws that string to JS, iirc. Yes, this is absolutely
> dumb... and maybe I'm misremembering. But something stupid like that
> happens.
That can't be right, because if I _catch_ the exception within
JavaScript, I get a broken down DOMException.
Maybe the lossage you remember happens at the point where we convert
uncaught JS exceptions to console messages? (Which I still can't find.)
>> In that case the patch might be as simple as removing any
>> OUTPUT_ERROR calls from ParseSelectorString and its subroutines.
>
> Hmm. Yeah, that would work; there are no subroutines to remove
> from, in fact.
Is there a bug already? I can write a patch sometime this week.
> Actually, the simplest thing if you have a tree is to remove
> this line:
>
> 'nsIDOMNSElement.*'
>
> from dom_quickstubs.qsconf and rebuild, then retest matchesSelector.
Will try that when I get a chance.
zw
Boris Zbarsky
here, and very on-topic for there.
On 1/31/11 10:01 PM, Zack Weinberg wrote:
> That can't be right, because if I _catch_ the exception within
> JavaScript, I get a broken down DOMException.
>
> Maybe the lossage you remember happens at the point where we convert
> uncaught JS exceptions to console messages? (Which I still can't find.)
http://hg.mozilla.org/mozilla-central/file/0a47be5cdd94/dom/base/nsJSEnvironment.cpp#l584
and
http://hg.mozilla.org/mozilla-central/file/0a47be5cdd94/dom/base/nsJSEnvironment.cpp#l448
And if you breakpoint in NS_ScriptErrorReporter, the incoming
JSErrorReport object has no filename and lineno set to 0, even though I
can confirm that catching the exception in JS has a useful lineNumber.
So something between when you catch the exception and when the JS engine
reports it to the embedding messes with it...
Looks like what happens is that XPConnect actually throws an object as
the exception. Then js_ReportUncaughtException calls
js_ReportErrorNumberVA which calls PopulateReportBlame which sees that
no script is on the stack (because this is happening after the JS is
done running) and does nothing. Then it calls js_ExpandErrorArguments
which sees js_GetErrorMessage as the callback, ends up with a
JSErrorFormatString with one argument, stringifies the exception, sticks
that into the report (but nothing else, so far), and reports _that_ to
the embedding.
Given that we have a nice exception object here with all sorts of
location info and jazz, could we somehow teach jseng to not forget that
when generating the JSErrorReport?
> Is there a bug already? I can write a patch sometime this week.
Not yet. Want to file?
-Boris