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

RESOLVED FIXED in mozilla35

Status

()

Core
DOM: Content Processes
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
http://logs.glob.uno/?c=content#c236057
(Assignee)

Comment 2

3 years ago
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...

Updated

3 years ago
tracking-e10s: --- → ?
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Updated

3 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
Created attachment 8483019 [details] [diff] [review]
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().
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Updated

3 years ago
Summary: Shutdown leak of outer window when running browser element tests out of process → Shutdown leak of outer window in child process when running browser element tests out of process
(Assignee)

Comment 6

3 years ago
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...)
tracking-e10s: ? → +
Priority: -- → P1

Updated

3 years ago
Blocks: 984139
(Assignee)

Comment 7

3 years ago
Created attachment 8494685 [details] [diff] [review]
Call DestroyWindow when BrowserFrameProvideWindow does a __delete__.
Attachment #8483019 - Attachment is obsolete: true
Attachment #8494685 - Flags: review?(bugs)
Comment on attachment 8494685 [details] [diff] [review]
Call DestroyWindow when BrowserFrameProvideWindow does a __delete__.

I guess. A bit hackish
Attachment #8494685 - Flags: review?(bugs) → review+
(Assignee)

Comment 9

3 years ago
try run: https://tbpl.mozilla.org/?tree=Try&rev=784aba9b4edf
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccf72f5459d
https://hg.mozilla.org/mozilla-central/rev/dccf72f5459d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.