swapBrowsers() throws a TypeError because TabProgressListener isn't reachable as a window property
Categories
(Firefox :: Tabbed Browser, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox156 | --- | fixed |
People
(Reporter: dao, Assigned: dao)
References
Details
Attachments
(2 files)
Tabbrowser.swapBrowsers() restores the swapped-in tab's progress listener with:
tabListener = new otherTabBrowser.documentGlobal.TabProgressListener(...)
(browser/components/tabbrowser/content/tabbrowser.js:6727, where documentGlobal is the tab's window.)
But class TabProgressListener is declared inside the file's private block scope ({ // start private scope for Tabbrowser, line 5), so it is a lexical binding in that block and never becomes a property of the window. documentGlobal.TabProgressListener is therefore undefined and the call throws a TypeError. There is no other definition of TabProgressListener in browser/ or toolkit/, and this fails for same-window swaps just as it does for cross-window ones.
The path is reachable from nsIBrowser::swapBrowsers via browser-custom-element.mjs, which forwards to Tabbrowser.swapBrowsers() when both browsers are attached to a tabbrowser. NewTabPagePreloading doesn't hit it, since the preloaded browser has no tabbrowser and falls through to swapDocShells(). There appears to be no test coverage for the affected path. swapBrowsersAndCloseOther() is unaffected; it doesn't construct a listener.
Plan: rather than exposing the class on the window, make TabProgressListener window-agnostic and construct it directly. Its body relies on the constructing realm's globals -- roughly 50 bare gBrowser references plus gURLBar, PrivateBrowsingUtils.isWindowPrivate(window) and AIWindow.isAIWindowActive(window) -- which is what documentGlobal.TabProgressListener was reaching for, so simply constructing the local class first would trade the TypeError for a listener driving the wrong window's tabbrowser. So:
- Derive the tabbrowser and window from
this._tab/this._browser, which the constructor already receives, and drop the bare window globals. - Replace the construction at 6727 with
new TabProgressListener(...).
Both steps are prerequisites for bug 2049770, which needs the class free of window closures anyway, and step 2 is the shape the shared module wants -- nothing to undo at move time. Bug 2049770 also removes the interim wrinkle that a listener constructed in one window for another window's tab keeps the constructing window's global alive, since the shared class lives in the system realm.
|
Assignee | |
Comment 1•13 days ago
|
||
TabProgressListener is declared inside the file's private block scope, so it
never was a property of the window and swapBrowsers threw a TypeError where it
should have restored the tab's progress listener. Asking the owning tabbrowser
for the listener keeps it in the realm of the window whose tab it serves, so it
resolves that window's globals and doesn't keep the swapping window alive.
The test drives the swap through nsIBrowser::swapBrowsers, the path that reaches
this code, and checks both the listener's realm and that it notifies its own
window.
|
||
Updated•13 days ago
|
|
|
Assignee | |
Comment 2•13 days ago
|
||
The timer was scheduled by the window the tab is adopted from, so clearing the
id against the adopting window's timers leaves it pending and can cancel an
unrelated timer of the same id.
|
||
Comment 4•12 days ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/af0ec50a215c
https://hg.mozilla.org/mozilla-central/rev/cc05575d8e79
Description
•