Stack overflow in XPCThrower crashes Firebug FBTest

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
ASSIGNED
9 years ago
6 years ago

People

(Reporter: John J. Barton, Assigned: timeless)

Tracking

({crash})

1.9.1 Branch
x86
All
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p3])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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
Flags: blocking1.9.1?
(Reporter)

Comment 1

9 years ago
Actually looks like another version of bug 485055.
(Reporter)

Comment 2

9 years ago
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.
bug 485055. Missed a 5 there. ;)
So, uh, dupe?
(Reporter)

Comment 5

9 years ago
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++
(Reporter)

Comment 6

9 years ago
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
(Reporter)

Comment 7

9 years ago
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.
(Reporter)

Comment 8

9 years ago
I will work around this.  Is there any effective way to prevent this from falling through the cracks?
Flags: blocking1.9.1?
Whiteboard: [firebug-p3]
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.
Flags: wanted1.9.2?
Flags: wanted1.9.1?
Keywords: crash
(Assignee)

Comment 10

9 years ago
to me, comment 6 implies that js could/should perform a stack overflow check, this seems more reasonable.
Component: XPConnect → JavaScript Engine
QA Contact: xpconnect → general
found during triage. This hasn't had any attention in 6 months. Anything we can do here?
(Assignee)

Comment 12

8 years ago
Created attachment 409296 [details] [diff] [review]
proposal
Attachment #409296 - Flags: review?(mrbkap)
Comment on attachment 409296 [details] [diff] [review]
proposal

This could cause the global object to get GC'd.
Attachment #409296 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 14

8 years ago
Created attachment 409888 [details] [diff] [review]
try to handle gc
Assignee: nobody → timeless
Attachment #409296 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #409888 - Flags: review?(mrbkap)
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.

Updated

8 years ago
Attachment #409888 - Flags: review?(mrbkap) → review-
(Assignee)

Updated

7 years ago
Duplicate of this bug: 606169
(Assignee)

Comment 17

7 years ago
Created attachment 486564 [details] [diff] [review]
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.
Attachment #409888 - Attachment is obsolete: true
Attachment #486564 - Flags: review?(mrbkap)
clearing some ancient flags.
Flags: wanted1.9.2?
Flags: wanted1.9.1?
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.
Attachment #486564 - Flags: review?(mrbkap)
You need to log in before you can comment on or make changes to this bug.