898811 - JS method parsing failure is swallowed

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Tom S [:evilpie]]

When this code (http://dxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLProtoImplMethod.cpp#l210) fails, we basically swallow the exception from what I have seen. And if the member was in some important xul file like tabbrowser.xml or browser.xml, you can't event debug this. We should print a message to the console, with the name, file and line when this problem arises.

Comment 1

[Tom S [:evilpie]]

Attachment: xbl-compile-error

This is more of a wallpaper patch that explicitly dumps something. I could imagine that a real solution would try to always report JS exceptions. A hunch tells me that the dummy context stuff created for compiling xul files are not wired up correctly to the error reporting.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bobby, thoughts?  I thought we had an XBL error reporter for this stuff...

Comment 3

[Bobby Holley (:bholley)]

Yeah, I'd think we should end up here:

http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLDocumentInfo.cpp#214

I couldn't say why we're not without debugging. Tom, do you have simple STR?

Comment 4

[Tom S [:evilpie]]

Mhm I this supposed to be visible in the console? My test case was to add something that doesn't parse to "reload" in browser.xml. This pretty much makes the browser unusable.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Use the system error reporter for XBL compilation. v1

The current XBL error reporter logs things to the console service, but not to
stderr. And in certain situations (especially if there's a parse error in
frontend XBL that leaves the browser in a half-baked state), it can be very
difficult to get to the error console.

So we should log to stderr too, which is exactly what the system error reporter
does.

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=6a4c729c3e7b

Comment 7

[Tom S [:evilpie]]

Comment on attachment 783278 [details] [diff] [review]
Use the system error reporter for XBL compilation. v1

Review of attachment 783278 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for fixing this! I have no idea how we could endure this problem :)

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +249,5 @@
>    AutoPushJSContext cx(mScriptContext->GetNativeContext());
>  
>    // nsJSEnvironment set the error reporter to NS_ScriptErrorReporter so
>    // we must apparently override that with our own (although it isn't clear 
>    // why - see bug 339647)

Do you know the answer to this? Maybe you should remove this comment.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 783278 [details] [diff] [review]
Use the system error reporter for XBL compilation. v1

Review of attachment 783278 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +249,5 @@
>    AutoPushJSContext cx(mScriptContext->GetNativeContext());
>  
>    // nsJSEnvironment set the error reporter to NS_ScriptErrorReporter so
>    // we must apparently override that with our own (although it isn't clear 
>    // why - see bug 339647)

Yeah, this comment should go. The bug itself has the answer (NS_ScriptErrorReporter expects the context and global object to be DOM things, which isn't the case for XBL).

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57cda2d1472f

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57cda2d1472f