489378 - Stack overflow in XPCThrower crashes Firebug FBTest

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

Using a home built (and slightly patched) FF:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre

We crash on Firebug's FBTest.  

First-chance exception at 0x7c90ede5 in firefox.exe: 0xC00000FD: Stack overflow.

Part of the stack that overflows is the repeating pattern:
 	js3250.dll!JS_GetScopeChain(JSContext * cx=0x03557d10)  Line 1802 + 0x39 bytes	C++
 	xpc3250.dll!XPCThrower::ThrowExceptionObject(JSContext * cx=0x03557d10, nsIException * e=0x00d397c0)  Line 320 + 0xa bytes	C++
 	xpc3250.dll!XPCThrower::BuildAndThrowException(JSContext * cx=0x03557d10, unsigned int rv=2147549183, const char * sz=0x0151005c)  Line 256 + 0x12 bytes	C++
 	xpc3250.dll!XPCThrower::Throw(unsigned int rv=2147549183, JSContext * cx=0x03557d10)  Line 57 + 0x11 bytes	C++
 	xpc3250.dll!Throw(unsigned int errNum=2147549183, JSContext * cx=0x03557d10)  Line 55 + 0xd bytes	C++
 	xpc3250.dll!XPC_WN_InnerObject(JSContext * cx=0x03557d10, JSObject * obj=0x05062e80)  Line 862 + 0xd bytes	C++
 	js3250.dll!JS_GetScopeChain(JSContext * cx=0x03557d10)  Line 1802 + 0x39 bytes	C++

The problem is a loop, we take the NS_FAILED path in XPC_WN_InnerObject:

    XPCNativeScriptableInfo* si = wrapper->GetScriptableInfo();
    if(si && si->GetFlags().WantInnerObject())
    {
        JSObject *newThis;
        nsresult rv =
            si->GetCallback()->InnerObject(wrapper, cx, obj, &newThis);

        if(NS_FAILED(rv))
        {
            Throw(rv, cx);

            return nsnull;
        }

        obj = newThis;
    }

The Throw() calls BuildAndThrowException, ThrowExceptionObject, and in then it calls JS_GetScopeChain() which calls XPC_WN_InnerObject.

Two recent changes may be related: wrapper code is on the stack and Bug 469492 avoided the opposite case, JS_GetScopeChain crashing on null.

I believe this can be reproduced as follows:
1) Install Firebug 1.4X.0a20 from http://getfirebug.com/releases/firebug/1.4X
2) Open Firebug on any page.
3) Use Firebug Icon Menu (upper left) Open Tracing 
4) Click "Run All" or "examples/exampleNetTest.js
5) crash

Comment 1

[John J. Barton]

Actually looks like another version of bug 485055.

Comment 2

[John J. Barton]

The crash requires chromebug, no crash with just the 5 steps I gave. This makes it even more likely that it is related to bug 48505.

Comment 3

[Rob Campbell [:rc] (:robcee)]

bug 485055. Missed a 5 there. ;)

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

So, uh, dupe?

Comment 5

[John J. Barton]

I believe this is the start of the stack overflow (if true this is not a dupe):
 	XPC_WN_InnerObject() Line 862	C++
 	JS_GetScopeChain() Line 1802	C++
 	XPCThrower::ThrowExceptionObject() Line 320	C++
 	XPCThrower::BuildAndThrowException() Line 256	C++
 	XPCThrower::Throw() Line 57	C++
 	Throw() Line 55	C++
 	XPC_WN_InnerObject() Line 862	C++
 	JS_GetScopeChain() Line 1802	C++
 	XPCThrower::ThrowExceptionObject() Line 320	C++
 	XPCThrower::BuildAndThrowException() Line 256	C++
 	XPCThrower::Throw() Line 57	C++
 	Throw() Line 55	C++
 	XPC_WN_InnerObject() Line 862	C++
 	JS_GetScopeChain() Line 1802	C++
 	XPC_WN_JSOp_ThisObject() Line 1327	C++
 	ComputeThis() Line 922	C++
 	js_ComputeThis() Line 935	C++
 	js_Invoke() Line 1229	C++
 	js_InternalInvoke() Line 1441	C++
 	js_TryMethod() Line 5527	C++
 	js_DefaultValue() Line 4752	C++
 	js_ValueToString() Line 2956	C++
 	js_ReportUncaughtException() Line 1263	C++
>	JS_ReportPendingException() Line 5815	C++
 	nsXPCWrappedJSClass::CheckForException() Line 1079	C++
 	nsXPCWrappedJSClass::CallMethod() Line 1645	C++
 	nsXPCWrappedJS::CallMethod() Line 562	C++
 	PrepareAndDispatch() Line 114	C++
 	SharedStub() Line 142	C++
 	nsObserverList::NotifyObservers() Line 129	C++
 	nsObserverList::NotifyObservers() Line 129	C++
 	nsObserverService::NotifyObservers() Line 184	C++
 	nsGlobalWindow::NotifyDOMWindowDestroyed() Line 5762	C++

Comment 6

[John J. Barton]

Note the following comment around
http://mxr.mozilla.org/mozilla1.9.1/source/js/src/jsapi.cpp#5809   
    /*
     * Set cx->generatingError to suppress the standard error-to-exception
     * conversion done by all {js,JS}_Report* functions except for OOM.  The
     * cx->generatingError flag was added to suppress recursive divergence
     * under js_ErrorToException, but it serves for our purposes here too.
     */
    save = cx->generatingError;
    cx->generatingError = JS_TRUE;
    ok = js_ReportUncaughtException(cx);

This is the flag that needs to be checked to prevent the stack overflow

Comment 7

[John J. Barton]

The crash comes from accessing nsIDOMWindow.location on a closed window in an observer of topic = dom-window-destroyed. The property is invalid, causing an exception, which fails because the InnerObject call in the original report fails, throwing an exception, and we loop.

This explains why it happens in Chromebug and no where else, *yet*. 

I think the bug here is that some method in the call stack above is failing to set cx->throwing.  That would prevent the recursion.

Comment 8

[John J. Barton]

I will work around this.  Is there any effective way to prevent this from falling through the cracks?

Comment 9

[Rob Campbell [:rc] (:robcee)]

I think setting the crash keyword and marking these as wanted 1.9.1 and 1.9.2 is probably the best way to keep these on the radar for now. If we need higher priority we can request blocking.

Comment 10

[timeless]

to me, comment 6 implies that js could/should perform a stack overflow check, this seems more reasonable.

Comment 11

[Rob Campbell [:rc] (:robcee)]

found during triage. This hasn't had any attention in 6 months. Anything we can do here?

Comment 12

[timeless]

Attachment: proposal

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 409296 [details] [diff] [review]
proposal

This could cause the global object to get GC'd.

Comment 14

[timeless]

Attachment: try to handle gc

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

As much as I try to convince myself that the JS engine is somehow responsible for this crash, I can't do it. XPConnect needs to stop the infinite recursion here.

One (easy) way to do this would simply be to check cx->generatingError in XPCThrower::BuildAndThrowException. Otherwise, we'll probably need to add something to XPCPerThreadData.

Comment 17

[timeless]

Attachment: jsapi take 3

> As much as I try to convince myself that the JS engine is somehow responsible
> for this crash, I can't do it.

Very simple: the field you point to is private. thus js engine is entirely responsible for this crash. grr.

Comment 18

[Rob Campbell [:rc] (:robcee)]

clearing some ancient flags.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 486564 [details] [diff] [review]
jsapi take 3

After talking this over with Luke, we think this fix needs to be entirely in XPConnect, so basically having a CreatingXPConnectException API somewhere and using that here.

Comment 20

[André Bargull [:anba]]

Firebug related bug -> no longer valid.