Closed
Bug 1172137
Opened 9 years ago
Closed 9 years ago
_outerWindowIDBrowserMap keeps browsers alive
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 41
People
(Reporter: ttaubert, Assigned: ttaubert)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
2.48 KB,
patch
|
mconley
:
review+
Sylvestre
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
2.24 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
I assume the same leak occurs when switching a browser's remoteness as we don't remote the old outerWindowID mapping.
Comment 2•9 years ago
|
||
(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 | ||
Comment 4•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 years ago
|
Component: Web Apps → General
Comment 10•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8616353 -
Flags: review?(mconley) → review+
Comment 11•9 years ago
|
||
[Tracking Requested - why for this release]: Leaks, leaks, glorious leaks.
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7a17afed88d3 https://hg.mozilla.org/integration/fx-team/rev/77621d399d1d https://hg.mozilla.org/integration/fx-team/rev/853fae656e75
Comment 14•9 years ago
|
||
Backed out for Win8 M(JP) failures. https://treeherder.mozilla.org/logviewer.html#?job_id=3365893&repo=fx-team
Comment 15•9 years ago
|
||
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.
Comment 16•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/c9f8f919578e
Assignee | ||
Comment 17•9 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.
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/05586e36413f https://hg.mozilla.org/integration/fx-team/rev/87dc1d71af51 https://hg.mozilla.org/integration/fx-team/rev/5097e037c4b0
Updated•9 years ago
|
Updated•9 years ago
|
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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Assignee | ||
Comment 20•9 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 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
Comment 23•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e08a95d877f1 https://hg.mozilla.org/releases/mozilla-aurora/rev/49147fda6a53 https://hg.mozilla.org/releases/mozilla-aurora/rev/ca0716b04edc
Comment 24•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/5ac2c3ff9a6d https://hg.mozilla.org/releases/mozilla-beta/rev/73d2a857261f https://hg.mozilla.org/releases/mozilla-beta/rev/71ce4845c6f6
You need to log in
before you can comment on or make changes to this bug.
Description
•