Closed Bug 1285638 Opened 5 years ago Closed 5 years ago

network monitor leaks window when opened on parent window and window is closed

Categories

(DevTools :: General, defect)

49 Branch
defect
Not set
normal

Tracking

(firefox47 unaffected, firefox48 unaffected, firefox49+ fixed, firefox50+ fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 + fixed
firefox50 + fixed

People

(Reporter: bkelly, Assigned: jsnajdr)

References

Details

(Keywords: regression, Whiteboard: [MemShrink])

Attachments

(1 file)

STR:

1) Open fresh browser in e10s mode
2) Open a parent-side window (I used about:config)
3) Open devtools toolbox in parent-side window
4) Select network monitor tab
5) Refresh parent-side window
6) Close parent-side window with toolbox still open
7) Open about:memory tab, minimize, and measure
8) Observe the parent-side window from (2) is detached

I can reproduce in 49 and 50, but not in early versions.  So it seems like a regression in 49.  I can reproduce in both e10s and non-e10s modes.

As far as I can tell the server/actor/webconsole.js code is not getting disconnect() called the toolbox has been opened for a parent window.  It only gets disconnect called for actors going away.

Eddy, do you know where we should call disconnect() here?
Flags: needinfo?(ejpbruel)
I also tested this with the fixes for bug 1261869 and bug 1267693.  Neither helped here.

The cc/gc logs showed this as the retaining path:

via persistent-Object :
00000270F6125BA0 [BackstagePass 270f65e0580]
    --[loader, devtools]--> 00000270F0422880 [Object <no private>]
    --[_provider]--> 00000270F0422920 [Object <no private>]
    --[loader]--> 00000270F6393140 [Proxy <no private>]
    --[private]--> 00000270F6354E80 [Object <no private>]
    --[sandboxes]--> 00000270F6393900 [Object <no private>]
    --[resource://devtools/server/actors/webconsole.js]--> 00000270FAF6B3A0 [Proxy <no private>]
    --[private]--> 00000270FF5F7A00 [Object <no private>]
    --[NetworkMonitor]--> 00000270FD8974C0 [Function NetworkMonitor]
    --[fun_environment]--> 00000270FF5C53D0 [Block <no private>]
    --[ChannelEventSinkFactory]--> 00000270F52B5100 [Proxy <no private>]
    --[private]--> 00000270F8A65FC0 [Object <no private>]
    --[_instance]--> 0000027080E88660 [Proxy <no private>]
    --[private]--> 00000270FAF827E0 [Object <no private>]
    --[collectors]--> 00000270FAF8C3D0 [Set 27087beacd0]
    --[key]--> 00000270FAF82740 [Object <no private>]
    --[filters]--> 00000270F5344A00 [Object <no private>]
    --[window]--> 00000270810F39C0 [Proxy <no private>]
    --[private]--> 000002708AEBC040 [Proxy <no private>]
    --[private]--> 00000270F7F2E100 [Window <no private>]

If we could just get disconnect() called in webconsole.js I think all this would get cleaned up properly.
The retained path suggests this was caused bug 1134073.  Or rather, it caused us to start leaking a window in addition to the previously leaking sandbox.

I guess the best solution would get Loader.jsm's DevtoolsLoader.prototype.destroy() to get called?  That might also fix bug 1283587.
Blocks: 1134073
See Also: → 1283587
Version: unspecified → 49 Branch
Assignee: nobody → jsnajdr
This leak is caused by the StackTraceCollector not being actually destroyed. It's my omission when implementing bug 1134073.

If the tab is kept open and only the toolbox is closed, the console client will send a stopListeners request to the console actor, and the WCA_onStopListeners function will destroy the StackTraceCollector (and other objects).

If the whole tab with an opened toolbox is closed, however, stopListeners is not sent and only WCA_disconnect is called. The code that calls destroy() on the StackTraceCollector is missing there.

I added the missing destructor call and also added a test. The teardown() function in netmonitor's head.js closes the tab without previously closing the toolbox - exactly what we need here. Without the fix, this test leaks a window in the debug build. With the fix, the leak disappears.

This bug happens only on tabs that have parent-side content (about:config etc), because in this case, all NetworkMonitor stuff is happening in the parent process. If the content is in child process, the StackTraceCollector destroying happens in a different code path - some messaging between parent and child is involved - and it actually happens.

You only need to open and close a new tab to reproduce this. No new browser window is needed.
Attachment #8769498 - Flags: review?(poirot.alex)
(In reply to Ben Kelly [:bkelly] from comment #0)
> As far as I can tell the server/actor/webconsole.js code is not getting
> disconnect() called the toolbox has been opened for a parent window.  It
> only gets disconnect called for actors going away.

The disconnect() is called, but it doesn't do what it should (i.e., this.stackTraceCollector.destroy()). But maybe there are some circumstances where disconnect() is not called (maybe related to bug 1283587?). Please let me know if you can reproduce the leak even with my patch.
Flags: needinfo?(ejpbruel)
I did two try runs. The first one is only the test without the fix, to check if it leaks as expected. It does:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81061d09a0fe

And a second try with complete patch. The leaks disappeared:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=617b8eb23d04
Comment on attachment 8769498 [details] [diff] [review]
Network monitor leaks window when a parent-side tab is closed

Review of attachment 8769498 [details] [diff] [review]:
-----------------------------------------------------------------

That may be the first time I see a leak fix with an associated test.
Thanks Jarda!
Attachment #8769498 - Flags: review?(poirot.alex) → review+
Keywords: checkin-needed
Comment on attachment 8769498 [details] [diff] [review]
Network monitor leaks window when a parent-side tab is closed

Approval Request Comment
[Feature/regressing bug #]: Falloff from bug 1134073 - leaks windows when using the feature in a certain way
[User impact if declined]: Windows will leak when netmonitor is opened and then closed on about:* pages
[Describe test coverage new/current, TreeHerder]: Added new mochitests that tests the leak.
[Risks and why]: low: added an object destructor call at one place, covered by tests.
[String/UUID change made/needed]: none
Attachment #8769498 - Flags: approval-mozilla-aurora?
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/a2f084333bf3
Network monitor leaks window when a parent-side tab is closed. r=ochameau
Keywords: checkin-needed
Thanks for the quick fix and adding a test!
https://hg.mozilla.org/mozilla-central/rev/a2f084333bf3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Tracking 49/50+ for this memory leak regression.
Comment on attachment 8769498 [details] [diff] [review]
Network monitor leaks window when a parent-side tab is closed

Fixes regression in 49, includes new tests, let's patch the leak
Attachment #8769498 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.