353704 - crash (not sure what I did, could have been on blogger.com) [@ js_SetProtoOrParent] [@ xpsp2res.dll] [@ 0x20202020]

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

crash (not sure what I did, could have been on blogger.com) [@js_SetProtoOrParent]

this was on my mac, trunk, debug build from 20060921

I'll attach the crash report

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: crash report

Comment 2

[(not reading, please use [e-mail] instead)]

I think I had blogger.com going in a tab, and I was publishing a post, and then I clicked on "view blog" before it was done publishing.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Crashes in and at random addresses (primarily 0x20202020, which for many users shows up as xpsp2res.dll) within js_SetProtoOrParent are by far the #1 topcrash for trunk builds starting with 2006-09-20-04-trunk.  Many of the stacks are:

0x20202020
js_GetSlotThreadSafe (sometimes)
js_SetProtoOrParent
JS_SetPrototype
xpc_CloneJSFunction

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My steps to repro are:
 1. load bug 353596
 2. middle click on the attachment to load it in a new tab
 3. click the [X] in the corner of the dialog immediately after it pops up

Using these steps, I found that the XPCWrappedNativeScope::mPrototypeJSFunction that we're trying to access in xpc_CloneJSFunction was allocated here:

NS_LogCtor_P (/home/dbaron/builds/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1111)
js_NewGCThing (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:1448)
js_NewObject (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2342)
js_NewFunction (/home/dbaron/builds/trunk/mozilla/js/src/jsfun.c:2073)
js_DefineFunction (/home/dbaron/builds/trunk/mozilla/js/src/jsfun.c:2142)
JS_DefineFunctions (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:3792)
JS_InitClass (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:2290)
js_InitObjectClass (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2159)
js_InitFunctionAndObjectClasses (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:1186)
JS_ResolveStandardClass (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:1485)
TempGlobalResolve (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:443)
js_LookupPropertyWithFlags (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:3228)
js_GetProperty (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:3370)
XPCWrappedNativeScope::SetGlobal(XPCCallContext&, JSObject*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:212)
XPCWrappedNativeScope (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:159)
XPCWrappedNativeScope::GetNewOrUsed(XPCCallContext&, JSObject*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:120)
nsXPConnect::InitClasses(JSContext*, JSObject*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:419)
nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsID const&, unsigned int, nsIXPConnectJSObjectHolder**) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:488)
nsJSContext::InitContext(nsIScriptGlobalObject*) (/home/dbaron/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:2171)
...

and garbage collected very soon after:

NS_LogDtor_P (/home/dbaron/builds/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1152)
js_GC (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2940)
js_NewGCThing (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:1336)
js_NewObject (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2342)
XPCWrappedNativeProto::Init(XPCCallContext&, int, XPCNativeScriptableCreateInfo const*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativeproto.cpp:113)
XPCWrappedNativeProto::GetNewOrUsed(XPCCallContext&, XPCWrappedNativeScope*, nsIClassInfo*, XPCNativeScriptableCreateInfo const*, int, int) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativeproto.cpp:228)
XPCWrappedNative::GetNewOrUsed(XPCCallContext&, nsISupports*, XPCWrappedNativeScope*, XPCNativeInterface*, int, XPCWrappedNative**) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:373)
nsXPCComponents::AttachNewComponentsObject(XPCCallContext&, XPCWrappedNativeScope*, JSObject*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpccomponents.cpp:3827)
nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsID const&, unsigned int, nsIXPConnectJSObjectHolder**) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:540)
nsJSContext::InitContext(nsIScriptGlobalObject*) (/home/dbaron/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:2171)


That space in memory was then reused for two other objects (both allocated and then collected) before we ran into a problem.

Comment 5

[(not reading, please use [e-mail] instead)]

Attachment: another crash report, this time, I was closing tabs.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And the XPCWrappedNativeScope in question is in gScopes (first, in fact), and its mGlobalJSObject was destroyed at the same stack as the mPrototypeJSFunction.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, the third use of the pointer was for the same type of object, and I think it's that one I care about.  It was created here:

NS_LogCtor_P (/home/dbaron/builds/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1111)
js_NewGCThing (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:1448)
js_NewObject (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2342)
JS_InitClass (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:2217)
js_InitFunctionClass (/home/dbaron/builds/trunk/mozilla/js/src/jsfun.c:2022)
js_InitFunctionAndObjectClasses (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:1181)
JS_ResolveStandardClass (/home/dbaron/builds/trunk/mozilla/js/src/jsapi.c:1485)
nsWindowSH::NewResolve(nsIXPConnectWrappedNative*, JSContext*, JSObject*, long, unsigned int, JSObject**, int*) (/home/dbaron/builds/trunk/mozilla/dom/src/base/nsDOMClassInfo.cpp:6084)
XPC_WN_Helper_NewResolve (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1065)
js_LookupPropertyWithFlags (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:3181)
js_GetProperty (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:3370)
XPCWrappedNativeScope::SetGlobal(XPCCallContext&, JSObject*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp:212)
nsXPConnect::InitClassesWithNewWrappedGlobal(JSContext*, nsISupports*, nsID const&, unsigned int, nsIXPConnectJSObjectHolder**) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/nsXPConnect.cpp:532)
nsJSContext::CreateNativeGlobalForInner(nsIScriptGlobalObject*, int, void**, nsISupports**) (/home/dbaron/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:2055)
nsGlobalWindow::SetNewDocument(nsIDocument*, nsISupports*, int, int) (/home/dbaron/builds/trunk/mozilla/dom/src/base/nsGlobalWindow.cpp:1333)

and destroyed on a different event off the event loop, probably on the first GC under that event.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

For a while I was thinking this was more complicated than it really was, even though I'd noticed the problem.  This makes mPrototypeJSFunction (which was added later) be managed more like mPrototypeJSObject -- so that it can't be dangling.

This is almost definitely a regression from my leak fix to bug 353090, although I haven't actually tested that.  What it did is it turned some places where we were already seeing the assertion:

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 594

into crashes.


(I haven't yet looked into whether JS_ClearScope clears things that the page can't.  But either way I think this would probably be good to get on branches as well.)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 239754 [details] [diff] [review]
patch

r+sr=jst

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 11

[Adam Guthrie]

*** Bug 354076 has been marked as a duplicate of this bug. ***

Comment 12

[timeless]

*** Bug 354291 has been marked as a duplicate of this bug. ***

Comment 13

[Brian Polidoro]

*** Bug 353555 has been marked as a duplicate of this bug. ***

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 239754 [details] [diff] [review]
patch

This claims to be a regression from bug 353090 which is requesting 1.8.0.9 approval. Neither is requesting 1.8.1 approval which seems odd if this is an important fix.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix error pointed out by brendan

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: combined patch for 1.8 branch

I just discussed this with brendan, and although we weren't sure if a page (rather than the C++ code I added to fix a leak) could have gotten us into a situation where we could trigger this bug and I'm at least not even sure how to figure out, this patch is quite safe and I think it's worth taking for 1.8.1.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 240244 [details] [diff] [review]
fix error pointed out by brendan

Yeah, of course. Should've seen that when reviewing :(

r+sr=jst

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 240244 [details] [diff] [review]
fix error pointed out by brendan

Checked in to trunk.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 240244 [details] [diff] [review]
fix error pointed out by brendan

>+        else {

Nit: jband-style braces go on their own lines.

/be

Comment 20

[Mike Schroepfer]

Brendan/DBaron - so we are not sure if this is content-induced crash and we are touching XPConnect/XPCNativeWrappers (which worries me some).  Are we sure two days before release we should take this?

Comment 21

[Adam Guthrie]

No crashes in Talkback [@ xpsp2res.dll] or [@ js_SetProtoOrParent] since 2006-09-23-04. Marking as VERIFIED.

Comment 22

[Mike Schroepfer]

Comment on attachment 240249 [details] [diff] [review]
combined patch for 1.8 branch

Approved for RC2.

Comment 23

[Mike Schroepfer]

Comment on attachment 240249 [details] [diff] [review]
combined patch for 1.8 branch

Disregard the approval - wrong bug.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not sure if it could be content-induced.

However, the patch is quite safe.  All it's doing is:
 * marking a pointer that it wasn't marking before (to prevent it from being garbage collected and thus dangling)
 * nulling out that pointer if it actually is garbage collected so we don't have a dangling pointer

This is exactly the same thing we've been doing for a similar pointer that was there longer.

Comment 25

[Mike Schroepfer]

Comment on attachment 240249 [details] [diff] [review]
combined patch for 1.8 branch

Ok then.  Let's get this in for RC2.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 240249 [details] [diff] [review]
combined patch for 1.8 branch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to MOZILLA_1_8_0_BRANCH.

Comment 29

[Marcia Knous [:marcia]]

verified on the 1.8.0 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.8) Gecko/20061025 Firefox/1.5.0.8. I used dbaron's STR in Comment 4 and did not see the crash. Adding keyword.