231396 - JS syntax error in XBL binding is not reported

Status: RESOLVED INVALID

(Core :: XBL, defect)

Description

[Alex Vincent [:WeirdAl]]

A simple JavaScript syntax error in an XBL binding causes the entire binding to
fall apart.  But the syntax error is never reported to anyone.  JS Console never
gets it, the debug terminal never gets it, and Venkman never notices it.

Testcase coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Brendan, what's XBL doing wrong here?

Comment 3

[Brendan Eich [:brendan]]

Need to JS_SetErrorReporter on whatever context XBL uses, if it isn't using a
DOM context (created by code in dom/src/base/nsJSEnvironment.cpp).

/be

Comment 4

[Alex Vincent [:WeirdAl]]

After #mozilla explained to me what brendan was saying, I took a look at LXR.

In the XBL source code, only nsXBLDocumentInfo.cpp uses JS_SetErrorReporter, on
lines 183 and 204.

The JSContext (through a context variable) apparently enters the picture in
several files:  nsXBLProtoImpl.cpp#110, nsXBLProtoImplField.cpp#106,
nsXBLProtoImplMethod.cpp#171, #266, nsXBLProtoImplProperty.cpp#202, #275, #317
are most directly related to this bug on first inspection.

From those, I notice that nsXBLProtoImpl::InstallImplementation calls the
relevant functions which define the context variable via
nsXBLProtoImpl::InitTargetObjects() and later through the InstallMember method
of nsXBLProtoImplMethod, nsXBLProtoImplProperty, and nsXBLProtoImplField (as
appropriate for the method, property or field being added, I gather).

This leads me to suggest that perhaps nsXBLProtoImpl::InitTargetObjects() is the
right place to add the JS_SetErrorReporter() code.  I don't want to do it, as
I'm still learning C++.

I didn't bother to check XBL handlers (nsXBLPrototypeHandler.cpp#412) for the
same syntax glitch in my testcase, but I believe those fail to report syntax
errors too.

Am I close, or is this another red herring?

Comment 5

[Brendan Eich [:brendan]]

If XBL creates its own JSContext, where?  That is the place to add one, and only
one, JS_SetErrorReporter call.  You shouldn't set an error reporter on every
call to bind to an object, e.g. -- all but the first would be redundant,
*assuming* the same, XBL-private, JSContext is being used.

If XBL creates a JSContext on the fly to do something, throws it away, and
repeats, then again: you want to find the JS_NewContext call and add a call to
JS_SetErrorReporter soon after.

/be

Comment 6

[Alex Vincent [:WeirdAl]]

LXR doesn't see JS_NewContext anywhere in XBL code.  :(  Oh, well.  This stuff is out of 
my ballpark anyway, and I am trying to learn. 

Comment 7

[Alex Vincent [:WeirdAl]]

Following up on comment 3, I decided to see where the context came from.  It called on the 
GetNativeContext() method of the passed aContext argument, which is defined only in 
dom/src/base/nsJSEnvironment.cpp#1356. 
 
If I'm reading comment 3 right, then I was on a red herring on comment 4. 

Comment 8

[Brendan Eich [:brendan]]

Ok, someone needs to breakpoint NS_ScriptErrorReporter and see why it doesn't do
its thing when called from XBL.  Or if it's not called, why not -- harder, bonus
points ;-).

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

See also
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLDocumentInfo.cpp#183
and
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLDocumentInfo.cpp#204

Though in this case, assuming I set up my breakpoints right, neither
XBL_ProtoErrorReporter nor NS_ScriptErrorReporter is called when that page is
loaded.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that when I checked in the patch in bug 127567 I _did_ most definitely test
JS error reporting at least for chrome XBL fields.  And it worked.

Comment 11

[Brendan Eich [:brendan]]

I shoulda lxr'd.  I don't see why there need to be two calls to
JS_SetErrorReporter in xbl.  The CreateScriptContext call passes null instead of
a global, to bypass the usual DOM window-ish global setup; well and good.  But
that means that NS_CreateScriptContext does not call back through
aGlobal->SetContext, so there's no point in nsXBLDocGlobalObject::SetContext
calling JS_SetErrorReporter.

The JS_SetErrorReporter call in nsXBLDocGlobalObject::GetContext's bootstrapping
case is enough.

Sorry, that's all nit-picking, probably not related to the bug.  Is it possible
that the error is being raised before the error reporter has been set?  Try a
breakpoint in js_ErrorToException.

/be

Comment 12

[Alex Vincent [:WeirdAl]]

be: I'm thinking the same thing, but for a different reason:  I don't know the 
order of operations when it comes to XBL.  That is, the order in which 
functions are called, from the moment a CSS -moz-binding rule is discovered to 
the binding being fully applied and the constructors firing.  It doesn't make 
sense any other way for XBL to report some JS errors but not syntax errors 
unless the error reporter is set much later than it should be.

Comment 13

[Alex Vincent [:WeirdAl]]

Please note bug 232095, which has another JS syntax error in XBL test page, on a
binding which is called twice, and leads to a crash therein.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Um.  That testcase DOES NOT APPLY THE BINDING TO ANYTHING.  If I add a CSS rule
to apply the binding to <foo> then I get errors in the JS console.

Is there a testcase that actually shows a problem?

Comment 15

[Alex Vincent [:WeirdAl]]

I am feeling about two millimeters tall right now.  Major apologies to everyone 
for wasting their time.