Closed Bug 1172137 Opened 9 years ago Closed 9 years ago

_outerWindowIDBrowserMap keeps browsers alive

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox39 + fixed
firefox40 + fixed
firefox41 + fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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
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)
(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
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)
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)
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)
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.
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)
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+
Attachment #8616353 - Flags: review?(mconley) → review+
[Tracking Requested - why for this release]:
Leaks, leaks, glorious leaks.
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.
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.
Keywords: regression
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.