475737 - Windows stay alive too long because nsJSContext doesn't unlink correctly

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

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?

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

Hmm, this triggers a bunch of other assertions though.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1.1

This one seems to work fine. No need for additional GC if we're destroying a context while cycle collecting.

Comment 4

[Igor Bukanov]

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/*.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 7

[Igor Bukanov]

(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.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.3

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1.4

Comment 10

[Peter Van der Beken [:peterv]]

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 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 375257 [details] [diff] [review]
v1.4

Yay! r=me.

Comment 12

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/0c8d4f846be8

Comment 13

[Peter Van der Beken [:peterv]]

Backed this out to see if it fixes Tshutdown regression on OS X: http://hg.mozilla.org/mozilla-central/rev/fb7f8956aff5.

Comment 14

[Peter Van der Beken [:peterv]]

Relanded: http://hg.mozilla.org/mozilla-central/rev/a1de0e8a0490.

Comment 15

[Peter Van der Beken [:peterv]]

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 16

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

Comment on attachment 375257 [details] [diff] [review]
v1.4

a191=beltzner

Comment 17

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/277444eb93f4