628599 - Post-close() navigation causes nsGlobalWindow leak

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is a regression from Bug 613515

Comment 2

[Andreas Gal :gal]

Bleh.

Comment 3

[Johnny Stenback (:jst)]

Peter, can you have a look here?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Peter Van der Beken [:peterv]]

It's weird, because it looks like the object is being kept alive by the stack somehow.

Comment 6

[Andreas Gal :gal]

By the stack scanner peterv?

Comment 7

[Peter Van der Beken [: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.

Comment 8

[Peter Van der Beken [:peterv]]

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.

Comment 9

[Andreas Gal :gal]

You could clear the expando object to unlink it. Would that work? Or is its parent the issue here?

Comment 10

[Peter Van der Beken [:peterv]]

I think it's the parent, but I'm still trying to find the exact cycles.

Comment 11

[Peter Van der Beken [:peterv]]

And clearing the expando slot in all the holders looks hard.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Add the edge from WN to expando

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Correct nsInProcessTabChildGlobal CC participant

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

Comment 14

[Peter Van der Beken [:peterv]]

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.

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Add the edge from WN to expando

This seems to fix all the leaks.

Comment 16

[Andreas Gal :gal]

Nice.

Comment 17

[Brendan Eich [:brendan]]

Who should review?

/be

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Peter Van der Beken [:peterv]]

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.

Comment 20

[Johnny Stenback (: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.

Comment 21

[Johnny Stenback (:jst)]

Both patches landed:

http://hg.mozilla.org/mozilla-central/rev/e039358204f5
http://hg.mozilla.org/mozilla-central/rev/f83ed5f4e885

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And the test:

http://hg.mozilla.org/mozilla-central/rev/61db518f85e3