292027 - code to release nsISupportsWeakReference wrapped JS objects crashes when GC repeats

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

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

The code to handling nsXPCWrappedJS objects that implement
nsISupportsWeakReference can crash when js_GC does more than one pass of garbage
collection.  I'm running into this crash with my patch to bug 241518.

The problem is the following:  XPCJSRuntime::GCCallback deals with
self->mWrappedJSToReleaseArray in two places, a JSGC_MARK_END callback and a
JSGC_END callback.  In the first, it calls JS_IsAboutToBeFinalized on the JS
object for each wrapper that could be finalized.  In the second, it releases
those wrappers.  This all works fine if js_GC doesn't "goto repeat".  However,
when it does, JSGC_MARK_END can be called multiple times before JSGC_END is
called.  When the second call happens, the objects for which
JS_IsAboutToBeFinalized was true have *already* been finalized, but the
nsXPCWrappedJS has a dangling mJSObj pointer, so we crash calling
JS_IsAboutToBeFinalized on a JSObject that was already finalized:

js_IsAboutToBeFinalized
JS_IsAboutToBeFinalized (/builds/trunk/mozilla/js/src/jsapi.c:1796)
WrappedJSDyingJSObjectFinder
(/builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:92)
JS_DHashTableEnumerate (/builds/trunk/mozilla/js/src/jsdhash.c:619)
JSObject2WrappedJSMap::Enumerate(JSDHashOperator (*)(JSDHashTable*,
JSDHashEntryHdr*, unsigned int, void*), void*)
(/builds/trunk/mozilla/js/src/xpconnect/src/xpcmaps.h:156)
XPCJSRuntime::GCCallback(JSContext*, JSGCStatus)
(/builds/trunk/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:290)
DOMGCCallback (/builds/trunk/mozilla/dom/src/base/nsJSEnvironment.cpp:2107)
js_GC (/builds/trunk/mozilla/js/src/jsgc.c:1780)

Comment 1

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

So, after looking back at bug 66950, JSGC_FINALIZE_END didn't exist back then,
so the choice to use JSGC_END rather than JSGC_FINALIZE_END may not have been as
intentional as it seemed from looking at the current code.

It actually almost seems better to me to do the releases in the
JSGC_FINALIZE_END callback -- the destruction of the wrapped JS and the call to
ClearWeakReferences are probably better off happening sooner than later, and
shouldn't be able to cause any cascading destructors, and the removal from the
JSObject2WrappedJS map is what would fix this bug.  The only issue seems to me
to be potential cascading destructors from releasing mOuter (and maybe mClass).
 Perhaps they could use XPCJSRuntime::DeferredRelease while GC is running (via a
boolean we note on the runtime using the GC callback)?

Comment 2

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

Attachment: patch

Comment 3

[Johnny Stenback (:jst)]

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

r=jst

Comment 4

[Brendan Eich [:brendan]]

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

sr+a=me, thanks for the in-person explanation -- a trip down cvs-annotate
memory lane!

/be

Comment 5

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

Fix checked in to trunk, 2005-06-14 17:21/23 -0700.