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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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?
(Assignee)

Comment 2

10 years ago
Hmm, this triggers a bunch of other assertions though.
(Assignee)

Comment 3

10 years ago
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

Comment 4

10 years ago
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/*.
(Assignee)

Comment 5

10 years ago
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...

Comment 7

10 years ago
(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.
(Assignee)

Comment 8

10 years ago
Created attachment 375041 [details] [diff] [review]
v1.3
Attachment #362612 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 375257 [details] [diff] [review]
v1.4
Attachment #375041 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #375257 - Flags: superreview?(jst)
Attachment #375257 - Flags: review?(bent.mozilla)
(Assignee)

Comment 10

10 years ago
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+

Updated

10 years ago
Depends on: 492483
(Assignee)

Comment 12

10 years ago
http://hg.mozilla.org/mozilla-central/rev/0c8d4f846be8
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

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

Updated

10 years ago
Attachment #375257 - Flags: approval1.9.1?
(Assignee)

Comment 15

10 years ago
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].

Updated

10 years ago
Depends on: 493720
Comment on attachment 375257 [details] [diff] [review]
v1.4

a191=beltzner
Attachment #375257 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 17

10 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/277444eb93f4
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.