Closed Bug 628599 Opened 9 years ago Closed 9 years ago
Post-close() navigation causes ns
Global Window leak
The mozilla-central regression range is a TM merge: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7ee91bd90e7a The testcase has to be local to trigger the bug.
Peter, can you have a look here?
Assignee: nobody → peterv
This is interesting. This is effectively a cross-origin expando since the location object has been destroyed. In Firefox 3.6 the attempt to set window.location throws NS_ERROR_NOT_AVAILABLE.
It's weird, because it looks like the object is being kept alive by the stack somehow.
By the stack scanner peterv?
I think that's a bad interaction of the stack scanner with JS_DumpHeap, since we pass the object we want to dump to JS_DumpHeap (so it will always be on the stack). So it's only bad debug output, not really marked. Trying to figure that out.
I have a patch that fixes this, but it's not quite right. The problem is that bug 613515 added an edge from wrapped native to expando object. Adding that to the CC helps, but I need to figure out how/when we want to unlink it. Currently I stop marking the expando when the wrapped native is expired, but that isn't correct.
You could clear the expando object to unlink it. Would that work? Or is its parent the issue here?
I think it's the parent, but I'm still trying to find the exact cycles.
And clearing the expando slot in all the holders looks hard.
nsInProcessTabChildGlobal doesn't traverse its JSContext and it unlinks its global instead of traversing. The JSContext was at the root of the chain that was holding the expando object, but it looks like there's multiple roots :-(.
I'm actually not enirely sure that the expando object is still a problem with these two patches, I don't find it in the CC log anymore. But we're still leaking.
This seems to fix all the leaks.
Attachment #508775 - Attachment is obsolete: true
Who should review? /be
Comment on attachment 508776 [details] [diff] [review] Correct nsInProcessTabChildGlobal CC participant > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsInProcessTabChildGlobal, > nsDOMEventTargetHelper) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mMessageManager) >- NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mGlobal) >+ nsFrameScriptExecutor::Traverse(tmp, cb); > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END Huh. Quite a mistake from me.
Attachment #508776 - Flags: review+
Comment on attachment 508844 [details] [diff] [review] Add the edge from WN to expando The patch for bug 613515 added a hashtable that stores expando JS objects with the wrapped native as the key. We trace these objects from TraceXPConnectRoots, and they are removed when the JS object of the wrapped native dies. This means that the lifetime of the expando JS object is identical to the lifetime of the wrapped native JS object, which means that there is a CC edge from wrapped native to expando. This patch starts recording that edge (TraverseExpandoObjects). It also makes us not mark the expando object if the wrapped native has expired (we've unlinked it), since in that case we won't actually mark the wrapped native's JS object either (see WrappedNativeJSGCThingTracer). The last thing it does is suspect any wrapped native's JS objects if they have an expando object, in general we need to suspect all objects that have an edge to a JS object. We already suspect wrapped natives that have an edge to their JS object (HasExternalReference()), we need to do the same thing if they have an edge to their expando JS object. The slightly confusing part is that there's three objects involved here, the XPCWrappedNative, its JS object and its expando JS object. The lifetime of the expando JS object is bound to the lifetime of the wrapped native JS object, which is not exactly the same as the lifetime of the XPCWrappedNative, but I think relying on XPCWrappedNative::IsWrapperExpired() is good enough since it really means the wrapped native JS object will die soon. If their wrapped native JS object dies but their wrapped native hasn't been unlinked we shouldn't have dangling pointers, since we already sweep expando JS objects in SweepExpandos in that case.
Attachment #508844 - Flags: review?(jst)
Attachment #508844 - Flags: review?(jst) → review+
Both patches landed: http://hg.mozilla.org/mozilla-central/rev/e039358204f5 http://hg.mozilla.org/mozilla-central/rev/f83ed5f4e885
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
And the test: http://hg.mozilla.org/mozilla-central/rev/61db518f85e3
You need to log in before you can comment on or make changes to this bug.