1060526 - Shutdown leak of outer window in child process when running browser element tests out of process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P1)

Description

[Andrew McCreight [:mccr8]]

When running this on L64:
  ./mach mochitest-plain dom/browser-element/mochitest/priority/
...we leak through shutdown on a child process.  The leak involves an outer window, but not an inner window or any DOM nodes.

There are two known strong references to the nsGlobalWindow, and one unknown strong reference.  Conservative heap scanning (bug 1058178) indicates the unknown reference is either from the window itself, or an nsDocShell.  The latter seems more likely.

In IRC, Olli suggests that something isn't calling DocShell::Destroy that should be.

Comment 1

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

http://logs.glob.uno/?c=content#c236057

Comment 2

[Andrew McCreight [:mccr8]]

It looks like the nsWebBrowser is still holding its billion pointers to the doc shell.

nsWebBrowser::Destroy() calls nsWebBrowser::InternalDestroy() which calls nsWebBrowser::SetDocShell(nullptr), which looks like it should call Destroy() on the doc shell and clear out all of the pointers, so I guess nsWebBrowser::Destroy() isn't being called...

Comment 3

[Andrew McCreight [:mccr8]]

This may or may not have anything to do with e10s, as this is a leak in a non-e10s OOP test.

So, it looks like we properly tear things down when the TabChild gets a RecvDestroy(), but TabChild::RecvDestroy() is only being called once in the child process that leaks, and there are two TabChildren (we leak one).  So I'm going to look into how the parent side of things is supposed to dispatch the destroy.

Comment 4

[Andrew McCreight [:mccr8]]

Maybe we're deciding to shut down the child process while the the parent side of the second frame element is still alive, and so we don't do the teardown for it?  I'll have to look into where we decide to do that.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Call DestroyWindow when BrowserFrameProvideWindow does a __delete__

This seems to fix the leak I am seeing in the dom/browser-element/mochitest/priority/test_BackgroundLRU.html test.

TabChild::BrowserFrameProvideWindow creates a new TabChild, then does some stuff.  For whatever reason, in this test, SendBrowserFrameOpenWindow fails, so we abort and do a Send__delete__() to the new TabChild.  Unfortunately this means we never call DestroyWindow() on the new TabChild, but it is still set up enough to be owned by the doc shell or something, so we leak.  The patch just calls DestroyWindow() explicitly.  This patch seems pretty brittle.  Maybe it would be better to define the Recv__delete__ method to call DestroyWindow().

Comment 6

[Andrew McCreight [:mccr8]]

Well, this may or may not fix the leak.  With the patch, we seem to instead crash the content process:
  ###!!! [Child][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost
Due to bug 831223, this doesn't show up as a failure.  (I'm guessing that's the same process that is failing...)

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Call DestroyWindow when BrowserFrameProvideWindow does a __delete__.

Comment 8

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

Comment on attachment 8494685 [details] [diff] [review]
Call DestroyWindow when BrowserFrameProvideWindow does a __delete__.

I guess. A bit hackish

Comment 9

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=784aba9b4edf

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dccf72f5459d

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dccf72f5459d