419655 - Refreshing a page leaks an nsGlobalWindow until shutdown

Status: VERIFIED FIXED

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

Description

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

There are at least 3 different causes:

1) nsSecureBrowserUIImpl is involved in an ownership cycle but is not cycle collected.
2) nsFocusController is improperly traversed.
3) There's something else that I haven't found yet...

Comment 1

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

Moving bugs that aren't beta 4 blockers to target final release.

Comment 2

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

Attachment: Patch, v1

So #3 was the navigator object's prototype keeping a reference to the window in which it was created.

This patch fixes issues 1-3 and allows us to reclaim the windows destroyed during a refresh.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 306147 [details] [diff] [review]
Patch, v1

>+  cb.NoteXPCOMChild(tmp->mOwnerContent->GetOwnerDoc()->GetWindow());

This seems rather unusual.  Does it really hold a reference directly to that window rather than to one of the intermediate objects?  If so, you should probably have a comment explaining how.

Comment 4

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

(In reply to comment #3)
> Does it really hold a reference 

Yeah, mDocShell holds an nsIScriptGlobalObject which is an nsGlobalWindow... Sadly nsDocShell is not cycle-collected so the previous Traverse code was basically a no-op, even though the docshell directly owns the window. This way we at least get the cycle collector happy by accounting for its last remaining unknown reference.

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 306147 [details] [diff] [review]
Patch, v1

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsFrameLoader)
>+  cb.NoteXPCOMChild(tmp->mOwnerContent->GetOwnerDoc()->GetWindow());

This won't work. You're recording one edge from nsFrameLoader to mOwnerContent->GetOwnerDoc()->GetWindow(), but you're ignoring other edges to mDocShell. Imagine that something else holds a strong reference to that docshell. So two objects hold the docshell and the docshell holds the window. The window has a reference count of 1 and you just recorded 1 edge to it here. So we'll unlink the window, even though the window might be held alive by something not in a cycle.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 306147 [details] [diff] [review]
Patch, v1

>diff -NprU8 mozilla.faca22b69aa0/security/manager/boot/src/nsSecureBrowserUIImpl.cpp mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp

> nsSecureBrowserUIImpl::Init(nsIDOMWindow *window)
> {
>+  nsCOMPtr<nsIDOMWindow> strongWindow = do_QueryReferent(mWindow);

ifdef PR_LOGGING.

>+  mWindow = do_GetWeakReference(window);
>+  NS_ENSURE_TRUE(mWindow, NS_ERROR_FAILURE);

Maybe do_GetWeakReference(window, &rv) and NS_ENSURE_SUCCESS.

>+  strongWindow = window;

No need for this, just use window instead below.

>@@ -611,17 +618,18 @@ nsSecureBrowserUIImpl::OnStateChange(nsI

>-  const PRBool isToplevelProgress = (windowForProgress.get() == mWindow.get());
>+  nsCOMPtr<nsIDOMWindow> strongWindow = do_QueryReferent(mWindow);
>+  const PRBool isToplevelProgress = (windowForProgress == strongWindow);

Should this check for non-null strongWindow?

r=peterv, except on the nsFrameLoader changes.

Comment 7

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

Attachment: Patch, v2

Fixed peterv's comments... Turns out that the window dies properly even without traversing the docshell held by the frame loader (although I don't really understand why) as long as these other two references get caught. I'm also ASSERTing that the window doesn't go away as I think it really should never happen as the window owns the secure impl.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 306375 [details] [diff] [review]
Patch, v2

sr=jst

I'd do a s/strongWindow/window/g on the secure browser UI changes, but I can deal either way.

Comment 9

[Johnny Stenback (:jst)]

Plussing, we should get this in for beta4, even!

Comment 10

[Mike Schroepfer]

Comment on attachment 306375 [details] [diff] [review]
Patch, v2

Let's get this in tonight for nightly baking

Comment 11

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

Fixed for beta 4.

Comment 12

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

Removing dependency on bug 372107 because the leak was fixed without it.

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032615 Firefox/3.0b5pre ID:2008032615