_outerWindowIDBrowserMap keeps browsers alive

RESOLVED FIXED in Firefox 39

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({regression})

Trunk
Firefox 41
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39+ fixed, firefox40+ fixed, firefox41+ fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Hitting a leak with my patches in bug 1167508 that is caused by _outerWindowIDBrowserMap keeping <xul:browser>s and their parent browser windows alive after docShell swapping. That happens specifically with gBrowser.replaceTabWithWindow() where the newly opened window is closed at the end of the test but the torn-off tab's browser in the original window is kept alive.

<3 about:ccdump
(Assignee)

Comment 1

3 years ago
I assume the same leak occurs when switching a browser's remoteness as we don't remote the old outerWindowID mapping.
(In reply to Tim Taubert [:ttaubert] from comment #0)
> Hitting a leak with my patches in bug 1167508 that is caused by
> _outerWindowIDBrowserMap keeping <xul:browser>s and their parent browser
> windows alive after docShell swapping. That happens specifically with
> gBrowser.replaceTabWithWindow() where the newly opened window is closed at
> the end of the test but the torn-off tab's browser in the original window is
> kept alive.
> 
> <3 about:ccdump

Strange... how could that happen? gBrowser.replaceTabWithWindow should result in the new window's arguments handler in browser.js to be called, which should, I believe, call gBrowser.swapBrowsersAndCloseOther in the newly opened window...

and that should (eventually) call _endRemoveTab on the original window for the tab that's being torn out, and _that_ should remove the browser from the mapping.

Or is something with my mental model here kinda screwy?
Flags: needinfo?(ttaubert)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1172134
(Assignee)

Comment 4

3 years ago
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> Strange... how could that happen? gBrowser.replaceTabWithWindow should
> result in the new window's arguments handler in browser.js to be called,
> which should, I believe, call gBrowser.swapBrowsersAndCloseOther in the
> newly opened window...
> 
> and that should (eventually) call _endRemoveTab on the original window for
> the tab that's being torn out, and _that_ should remove the browser from the
> mapping.

The problem is that after docShell swapping (for non-remote browsers) it uses the browser's new outerWindowID and tries to delete that entry from the map. There is no entry for it and the old mapping keeps the browser alive.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(ttaubert)
Summary: _outerWindowIDBrowserMap keeps browsers alive after docShell swapping → _outerWindowIDBrowserMap keeps browsers alive
(Assignee)

Comment 5

3 years ago
Created attachment 8616352 [details] [diff] [review]
0001-Bug-1172137-Record-outerWindowID-for-new-tabs-first-.patch

The first leak is caused by trying to access browser.outerWindowID too early, before we have a frameLoader. This leads to:

> this._outerWindowIDBrowserMap.set(undefined, browser);
Attachment #8616352 - Flags: review?(mconley)
(Assignee)

Comment 6

3 years ago
Created attachment 8616353 [details] [diff] [review]
0002-Bug-1172137-Update-outerWindowID-after-a-browser-s-r.patch

The second leak is caused by not updating the outerWindowID mapping when switching a browser's remoteness. The old outerWindowID will keep mapping to the browser and that won't be removed when the tab is. We also get wrong results for getBrowserForOuterWindowID() and won't find the browser under its new ID.
Attachment #8616353 - Flags: review?(mconley)
(Assignee)

Comment 7

3 years ago
Created attachment 8616354 [details] [diff] [review]
0003-Bug-1172137-Update-outerWindowID-mappings-when-swapp.patch

This third and last leak is caused by not updating outerWindowID mappings when swapping docShells. We not only need to register the new mappings but also unregister old ones when swapping docShells between windows. For remote browsers we manually have to copy over the cached outerWindowIDs.
Attachment #8616354 - Flags: review?(mconley)
(Assignee)

Comment 8

3 years ago
FWIW, these aren't three separate leaks but each of them contributes to a single leak that I can reproduce locally. All of them are needed to fix this one leak.
(Assignee)

Comment 9

3 years ago
Created attachment 8616367 [details] [diff] [review]
0003-Bug-1172137-Update-outerWindowID-mappings-when-swapp.patch, v2

Oops, when swapping in a preloaded tab (i.e. about:customizing) we have no |remoteBrowser| so we need to check for that.
Attachment #8616354 - Attachment is obsolete: true
Attachment #8616354 - Flags: review?(mconley)
Attachment #8616367 - Flags: review?(mconley)
(Assignee)

Updated

3 years ago
Component: Web Apps → General
Comment on attachment 8616352 [details] [diff] [review]
0001-Bug-1172137-Record-outerWindowID-for-new-tabs-first-.patch

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

::: browser/base/content/tabbrowser.xml
@@ +1878,5 @@
>              // We start our browsers out as inactive, and then maintain
>              // activeness in the tab switcher.
>              b.docShellIsActive = false;
>  
> +            // A remote browser doesn't initially have the outerWindowID

Can you please add a comment here about how we need to wait until we've loaded a URI before we can access the outerWindowID for non-remote browsers?
Attachment #8616352 - Flags: review?(mconley) → review+

Comment 11

3 years ago
[Tracking Requested - why for this release]:
Leaks, leaks, glorious leaks.
status-firefox39: --- → affected
status-firefox40: --- → affected
tracking-firefox39: --- → ?
Comment on attachment 8616367 [details] [diff] [review]
0003-Bug-1172137-Update-outerWindowID-mappings-when-swapp.patch, v2

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

:( Reaching all up into those private members is kinda gross, but I can't think of anything better at this time.

_outerWindowIDBrowserMap kinda seems like a memory leak footgun. Perhaps Cu.getWeakReference for the browsers would be a good idea for a follow-up.
Attachment #8616367 - Flags: review?(mconley) → review+
Note that due to SETA, Win8 is the only platform to run M(JP) since this was pushed, so it may in fact affect everything else as well.
(Assignee)

Comment 17

3 years ago
Sorry for that, wouldn't have imagined the jetpack tests to interfere. They do fail because of the very first patch here, the tests call gBrowser.removeTab() synchronously from a TabOpen listener. So by the time we get to the line that stores the outerWindowID the docShell is already gone. I added a simple check for the docShell here, I didn't feel like moving the TabOpen dispatch further down and changing possible implicit expectations. Will re-land when the trees are open.
tracking-firefox39: ? → +
tracking-firefox40: --- → +
tracking-firefox41: --- → +
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/05586e36413f
https://hg.mozilla.org/mozilla-central/rev/87dc1d71af51
https://hg.mozilla.org/mozilla-central/rev/5097e037c4b0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Assignee)

Comment 20

3 years ago
Comment on attachment 8616352 [details] [diff] [review]
0001-Bug-1172137-Record-outerWindowID-for-new-tabs-first-.patch

(Requesting approval for all three patches.)

Approval Request Comment
[Feature/regressing bug #]: bug 1077168
[User impact if declined]: leaks in rare cases, probably nothing noticeable
[Describe test coverage new/current, TreeHerder]: covered mostly by our leak checking code when running tests
[Risks and why]: low risk
[String/UUID change made/needed]: none
Attachment #8616352 - Flags: approval-mozilla-beta?
Attachment #8616352 - Flags: approval-mozilla-aurora?
Comment on attachment 8616352 [details] [diff] [review]
0001-Bug-1172137-Record-outerWindowID-for-new-tabs-first-.patch

Remove leaks. Taking it.
Attachment #8616352 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8616352 [details] [diff] [review]
0001-Bug-1172137-Record-outerWindowID-for-new-tabs-first-.patch

approved for uplift to beta.
Attachment #8616352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.