nsJSContext destroys the JSContext that it holds from its Unlink function. Normally objects are supposed to unlink their JS objects from the RootAndUnlinkJSObjects function. Because the nsJSContext does it from Unlink we often need two cycle collections to release a window. Here's what happens in a case where there are cycles involving a nsGlobalWindow and its XPCWrappedNative and a nsJSContext and its JSContext. We run the JS GC without marking the global object of JSContexts. From the JSGC_MARK_END notification we first run the traversal phase of cycle collection, which ends by calling RootAndUnlinkJSObjects on all the white objects (objects that are part of a collectable cycle). The nsGlobalWindow, the XPCWrappedNative, the nsJSContext and the JSContext are white. Still from the JSGC_MARK_END notification we call XPCJSRuntime::TraceXPConnectRoots, which iterates over all JSContexts and marks their global object. This ends up marking the JSObject of the XPCWrappedNative of the nsGlobalWindow, which puts the nsGlobalWindow and its XPCWrappedNative in a cycle again, and marks a number of other JS objects. From the JSGC_END notification we run the unlink phase of the cycle collector. We unlink the nsJSContext, which destroys its JSContext. There's still a cycle with the nsGlobalWindow and its XPCWrappedNative, so the cycle collection has failed to collect the nsGlobalWindow. The next cycle collection will go through the same steps, but without the JSContext, so everything gets collected. I tried destroying the JSContext from the RootAndUnlinkJSObjects for the nsJSContext, and that works fine. We do hit an assertion in XPCJSRuntime::TraceXPConnectRoots which Igor and I discussed before (see bug 458099, comment 21). Igor mentioned that we shouldn't destroy a context between JSGC_BEGIN and JSGC_MARK_END, but in this case the context would be destroyed from JSGC_MARK_END but before TraceXPConnectRoots. Igor, would that be fine?
Hmm, this triggers a bunch of other assertions though.
Created attachment 359339 [details] [diff] [review] v1.1 This one seems to work fine. No need for additional GC if we're destroying a context while cycle collecting.
Attachment #359292 - Attachment is obsolete: true
It should be fine to call JS_DestroyContext() from JSGC_MARK_END. There are number of restrictions, like the context must not be running a request or the code should use JS_DestoyContextNoGC(), not JS_DestoyContext(). Of cause, the context must not be the one that is passes to JS_GC. Now, to be completely safe the patch should enforce these restrictions using asserts - I will post tomorrow the necessary changes to js/src/*.
Created attachment 362612 [details] [diff] [review] v1.2
Attachment #359339 - Attachment is obsolete: true
igor, have you had a chance to look at the assertions you were interested in adding here? This patch solves some CC problems I was having with Gmail...
(In reply to comment #6) > igor, have you had a chance to look at the assertions you were interested in > adding here? I will try to look at this but cannot promise anything for this week.
Created attachment 375041 [details] [diff] [review] v1.3
Attachment #362612 - Attachment is obsolete: true
Created attachment 375257 [details] [diff] [review] v1.4
Attachment #375041 - Attachment is obsolete: true
Comment on attachment 375257 [details] [diff] [review] v1.4 Igor looked over my shoulder and directed me while I wrote the JS_ASSERTs in jscntxt.cpp.
Comment on attachment 375257 [details] [diff] [review] v1.4 Yay! r=me.
Attachment #375257 - Flags: review?(bent.mozilla) → review+
Attachment #375257 - Flags: superreview?(jst) → superreview+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Backed this out to see if it fixes Tshutdown regression on OS X: http://hg.mozilla.org/mozilla-central/rev/fb7f8956aff5.
Attachment #375257 - Flags: approval1.9.1?
Comment on attachment 375257 [details] [diff] [review] v1.4 I think we should take this on branch. Together with bug 490592 this decreased Tshutdown on Linux by about 9%. OS X also showed a decrease when this landed, but it regressed in the meantime (I backed out briefly to make sure this patch wasn't implicated in the regression). This patch also needs attachment 376888 [details] [diff] [review].
Comment on attachment 375257 [details] [diff] [review] v1.4 a191=beltzner
Attachment #375257 - Flags: approval1.9.1? → approval1.9.1+
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.