Closed Bug 628599 Opened 9 years ago Closed 9 years ago

Post-close() navigation causes nsGlobalWindow leak

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, testcase, Whiteboard: [hardblocker][has patch])

Attachments

(3 files, 1 obsolete file)

Attached file testcase
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.
blocking2.0: --- → ?
Blocks: 613515
No longer depends on: 613515
Bleh.
blocking2.0: ? → betaN+
Whiteboard: [hardblocker]
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.
Attached patch Add the edge from WN to expando (obsolete) — Splinter Review
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
Nice.
Whiteboard: [hardblocker] → [hardblocker][has patch]
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)
Comment on attachment 508844 [details] [diff] [review]
Add the edge from WN to expando

- In XPCJSRuntime::SuspectWrappedNative():

+    // Only record objects that might be part of a cycle as roots, unless
+    // the callback wants all traces (a debug feature).
+    if(!cb.WantAllTraces() && !nsXPConnect::IsGray(obj))
+        return;
+
+    cb.NoteRoot(nsIProgrammingLanguage::JAVASCRIPT, obj, nsXPConnect::GetXPConnect());

If you swap the conditions here you'd avoid checking for the unlikely debug condition being true in the cases where obj is gray. And in fact, this would read even easier as:

    if (nsXPConnect::IsGray(obj) || cb.WantAllTraces())
        cb.NoteRoot(...)

...

+    TraverseExpandoObjectClosure exposure = {

"exposure"? :) How about just closure, or expclosure?

r=jst either way.
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.