232095 - [FIX]JS syntax error in XBL binding used twice crashes Mozilla

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Alex Vincent [:WeirdAl]]

Crash testcase coming up.  Stack appears to be corrupted...  Just in case it's not:

#0  0x405a46f6 in nanosleep () from /lib/libc.so.6
#1  0x0000001c in ?? ()
#2  0x080721be in ah_crap_handler(int) (signum=11) at nsSigHandlers.cpp:135
#3  0x419338a9 in nsProfileLock::FatalSignalHandler(int) (signo=11)
    at nsProfileLock.cpp:209
#4  0x4012bc2d in __pthread_sighandler () from /lib/libpthread.so.0
#5  0x4051fd58 in __libc_sigaction () from /lib/libc.so.6
#6  0x41473a6a in nsXBLProtoImplMethod::InstallMember(nsIScriptContext*,
nsIContent*, void*, void*, nsCString const&) (this=0x87cf930, aContext=0x869efb0,
    aBoundElement=0x87a67e8, aScriptObject=0x83e0cf0,
    aTargetClassObject=0x83e0d00, aClassStr=@0x87cf5b0)
    at nsXBLProtoImplMethod.cpp:183
#7  0x414749b6 in nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding*,
nsIContent*) (this=0x87cf5b0, aBinding=0x876c578, aBoundElement=0x87a67e8)
    at nsXBLProtoImpl.cpp:84
#8  0x41465976 in nsXBLPrototypeBinding::InstallImplementation(nsIContent*) (
    this=0x876c578, aBoundElement=0x87a67e8) at nsXBLPrototypeBinding.cpp:440
#9  0x414614b2 in nsXBLBinding::InstallImplementation() (this=0x87cf858)
    at nsXBLBinding.cpp:814
#10 0x4147f22a in nsXBLService::LoadBindings(nsIContent*, nsIURI*, int,
nsIXBLBinding**, int*) (this=0x82042e8, aContent=0x87a67e8, aURL=0x87ae308,
    aAugmentFlag=0, aBinding=0xbfffe680, aResolveStyle=0xbfffe678)
    at nsXBLService.cpp:632
#11 0x411172d7 in nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell*,
n---Type <return> to continue, or q <return> to quit---
sIPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int,
nsStyleContext*, nsFrameItems&, int) (this=0x83daba0, aPresShell=0x86eade8,
    aPresContext=0x87562f8, aState=@0xbfffe820, aContent=0x87a67e8,
    aParentFrame=0x8782e44, aTag=0x87d3210, aNameSpaceID=9,
    aStyleContext=0x8783688, aFrameItems=@0xbfffe800, aXBLBaseTag=0)
    at nsCSSFrameConstructor.cpp:7025
#12 0x41117183 in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsIPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) (
    this=0x83daba0, aPresShell=0x86eade8, aPresContext=0x87562f8,
    aState=@0xbfffe820, aContent=0x87a67e8, aParentFrame=0x8782e44,
    aFrameItems=@0xbfffe800) at nsCSSFrameConstructor.cpp:6981
#13 0x4111c074 in nsCSSFrameConstructor::ContentInserted(nsIPresContext*,
nsIContent*, nsIFrame*, nsIContent*, int, nsILayoutHistoryState*, int) (
    this=0x83daba0, aPresContext=0x87562f8, aContainer=0x87ac8d0,
    aContainerFrame=0x0, aChild=0x87a67e8, aIndexInContainer=3,
    aFrameState=0x0, aInContentReplaced=0) at nsCSSFrameConstructor.cpp:8899
#14 0x4109f866 in PresShell::ContentInserted(nsIDocument*, nsIContent*,
nsIContent*, int) (this=0x86eade8, aDocument=0x87d1360, aContainer=0x87ac8d0,
    aChild=0x87a67e8, aIndexInContainer=3) at nsPresShell.cpp:5266
#15 0x41482b2a in nsXBLBindingRequest::DocumentLoaded(nsIDocument*) (
    this=0x8204348, aBindingDoc=0x87dc968) at nsXBLService.cpp:178
#16 0x4147dd9f in nsXBLStreamListener::Load(nsIDOMEvent*) (this=0x87d0f20,
    aEvent=0x87bedb0) at nsXBLService.cpp:429
#17 0x412d7597 in DispatchToInterface (aEvent=0x87bedb0, aListener=0x87d0f24,
---Type <return> to continue, or q <return> to quit---
    aMethod={__pfn = 0x11, __delta = 0}, aIID=@0x415b7c3c,
    aHasInterface=0xbfffeb28) at nsEventListenerManager.cpp:128
#18 0x412db353 in nsEventListenerManager::HandleEvent(nsIPresContext*, nsEvent*,
nsIDOMEvent**, nsIDOMEventTarget*, unsigned, nsEventStatus*) (this=0x87d0f78,
    aPresContext=0x0, aEvent=0xbfffec50, aDOMEvent=0xbfffebc4,
    aCurrentTarget=0x87dca00, aFlags=7, aEventStatus=0xbfffec8c)
    at nsEventListenerManager.cpp:1504
#19 0x41220840 in nsDocument::HandleDOMEvent(nsIPresContext*, nsEvent*,
nsIDOMEvent**, unsigned, nsEventStatus*) (this=0x87dc968, aPresContext=0x0,
    aEvent=0xbfffec50, aDOMEvent=0xbfffebc4, aFlags=7, aEventStatus=0xbfffec8c)
    at nsDocument.cpp:3632
#20 0x4145820f in nsXMLDocument::EndLoad() (this=0x87dc968)
    at nsXMLDocument.cpp:667
#21 0x4144d29e in nsXMLContentSink::DidBuildModel() (this=0x87e9d38)
    at nsXMLContentSink.cpp:290
#22 0x419d2124 in nsExpatDriver::DidBuildModel(unsigned, int, nsIParser*,
nsIContentSink*) (this=0x87c9478, anErrorCode=0, aNotifySink=1, aParser=0x87d0dd0,
    aSink=0x87e9d88) at nsExpatDriver.cpp:1042
#23 0x419f0212 in nsParser::DidBuildModel(unsigned) (this=0x87d0dd0,
    anErrorCode=0) at nsParser.cpp:1245
#24 0x419f12e3 in nsParser::ResumeParse(int, int, int) (this=0x87d0dd0,
    allowIteration=1, aIsFinalChunk=1, aCanInterrupt=1) at nsParser.cpp:1806
#25 0x419f304b in nsParser::OnStopRequest(nsIRequest*, nsISupports*, unsigned)
    (this=0x87d0dd0, request=0x87086b8, aContext=0x0, status=0)
---Type <return> to continue, or q <return> to quit---
    at nsParser.cpp:2472
#26 0x4147d750 in nsXBLStreamListener::OnStopRequest(nsIRequest*, nsISupports*,
unsigned) (this=0x87d0f20, request=0x87086b8, aCtxt=0x0, aStatus=0)
    at nsXBLService.cpp:322
#27 0x40c16ae1 in nsFileChannel::OnStopRequest(nsIRequest*, nsISupports*,
unsigned) (this=0x87086b8, req=0x87d10e0, ctx=0x0, status=0) at
nsFileChannel.cpp:577
#28 0x40b81318 in nsInputStreamPump::OnStateStop() (this=0x87d10e0)
    at nsInputStreamPump.cpp:498
#29 0x40b80cdb in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (
    this=0x87d10e0, stream=0x87d1234) at nsInputStreamPump.cpp:339
#30 0x407f49d1 in nsInputStreamReadyEvent::EventHandler(PLEvent*) (
    plevent=0x8758ce4) at nsStreamUtils.cpp:118
#31 0x4081a2f4 in PL_HandleEvent (self=0x8758ce4) at plevent.c:671
#32 0x4081a195 in PL_ProcessPendingEvents (self=0x813e118) at plevent.c:606
#33 0x4081d4ca in nsEventQueueImpl::ProcessPendingEvents() (this=0x813df08)
    at nsEventQueue.cpp:391
#34 0x41888d74 in event_processor_callback (data=0x813df08, source=4,
    condition=GDK_INPUT_READ) at nsAppShell.cpp:186
#35 0x418886dd in our_gdk_io_invoke (source=0x8262f90, condition=G_IO_IN,
    data=0x8286080) at nsAppShell.cpp:71
#36 0x4030d7d6 in g_io_channel_unix_get_fd () from /usr/lib/libglib-1.2.so.0
#37 0x403103ee in g_idle_remove_by_data () from /usr/lib/libglib-1.2.so.0
#38 0x40310199 in g_idle_remove_by_data () from /usr/lib/libglib-1.2.so.0
#39 0x4030f174 in g_main_run () from /usr/lib/libglib-1.2.so.0

On frame 6:
#6  0x41473a6a in nsXBLProtoImplMethod::InstallMember(nsIScriptContext*,
nsIContent*, void*, void*, nsCString const&) (this=0x87cf930, aContext=0x869efb0,
    aBoundElement=0x87a67e8, aScriptObject=0x83e0cf0,
    aTargetClassObject=0x83e0d00, aClassStr=@0x87cf5b0)
    at nsXBLProtoImplMethod.cpp:183
183         JSObject * method = ::JS_CloneFunctionObject(cx, mJSMethodObject,
globalObject);
(gdb) print mJSMethodObject
No symbol "mJSMethodObject" in current context.

Per line 181 of nsXBLProtoImplMethod.cpp, you would think this isn't even called...

Also reference bug 231396, which has a JS syntax error unreported by XBL (but
does not crash).  Crash testcase coming up.

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: crash testcase

It took me a very long time to figure out the minimal testcase for this bug. 
Please note bug 231396, which might've made it easier.	:-)

In this crash testcase, there's a valid method which follows an invalid one
(this is actually required for the testcase to crash in current builds), and
two elements tied to the same binding (again, required).  Other permutations
(such as swapping the methods around) do not crash Mozilla.

Comment 2

[[e-mail]]

Attachment: Another partial backtrace

Note, this is from a patched debug build, so some line numbers may be wrong.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The problem here is a follows:

1)  nsXBLProtoImpl::InstallImplementation calls InitTargetObjects.
2)  The first time we attach the binding, InitTargetObjects sees no mClassObject
    and calls CompilePrototypeMembers.
3)  CompilePrototypeMembers creates mClassObject, starts compiling members,
    fails on the first one, and bails out.  This makes InitTargetObjects bail
    out too, and makes InstallImplementation bail out as well, which means we
    never try to install the members on the object.
4)  The second time through, mClassObject is set (by step 3).  So
    InitTargetObjects doesn't bother calling CompilePrototypeMembers and we end
    up in the InstallMember loop.
5)  Calling InstallMember on an uncompiled (because we bailed before we got to
    it in step 3) nsXBLProtoImplMethod crashes (because we think we know what's
    in the union, but we're wrong).

The core problem is that the binding has a single "compiled" flag -- the binding
is compiled if mClassObject is set.  So even if steps 4 and 5 above were
avoided, at binding teardown the Destroy() method would be called with the
aCompiled flag set to "true" anyway, and that would cause similarly bogus access
to the union.  So just messing with mClassObject is not enough -- we need to
ensure that all proto members are in the "compiled" (this can include
"destroyed") state after the compilation phase.

The two posssible solutions I see:

1)  Ignore failure returns in the CompileMember loop and make sure to call
    CompileMember on all the members.  The CompileMember functions do
    appropriate cleanup when compilation fails, so this would do the trick.
2)  On failure return in the loop, go through and destroy all members (with
    aCompiled set to true for the already-compiled ones and to false for the
    yet-to-be-compiled ones).

The former would mean that all methods but the one that failed would be defined;
the latter would mean that none of the methods would be defined.  I'm not sure
which is better here...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible fix

This destroys all the members and properties on the binding if a compilation
error happens.	Per conversation with WeirdAl this is better than having
sorta-working franken-bindings.

I'd love it if we could prevent binding attachment completely in such cases...

Comment 5

[Boris Zbarsky [:bzbarsky]]

>+    curr->Destroy(compiled);
>+    if (curr == aBrokenMember) {
>+      compiled = PR_FALSE;
>+    }

I switched the Destroy() call and the if places to correspond to the comment I
put in the header...  Checked in.

Comment 6

[Bob Clary [:bc] (inactive)]

content/xbl/crashtests/232095-1.xul
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3