1458918 - browser.window.getLastFocused may allocate a tabId for an an adopting tab

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This issue is being extracted from Bug 1443221, to cover a issue similar to the one fixed on the webNavigation API (and triggered by the STR in Bug 1443221 comment 0) that can be still triggered by using the STR from Bug 1443221 comment 10:

> 1. Install Tile Tabs WE 9.0.
> 2. Open the Browser Toolbox (Tools > Web Developer > Browser Toolbox).
> 3. Click on the Tile Tabs WE button to create a layout with 2 tiled windows.
> 4. Right-click on the Tile Tabs WE button and select Toggle Toolbars a few times, or use the Toggle Toolbars keyboard shortcut
>   (Alt+Shift+RightArrow on Windows/Linux or MacCtrl+Shift+RightArrow on Mac)

By using the above STR, at some point a "chromeWin is null" error is going to be raised from PrivateBrowsingUtils.isBrowserPrivate, because the tabId is actually related to a tab that has been removed (and by inspecting the tabTracker._tabIds Map we can notice that the particular tabId is related to a nativeTab which has a null ownerGlobal).

It looks that the tabId has been assigned to the "removed tab" when the extension has called browser.windows.getLastFocused for a window opened to adopt an existent tab, as a side-effect of:

- getting the title for the window (actually as a side-effect of calling `this.activeTab` on the window wrapper):
  https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/toolkit/components/extensions/parent/ext-tabs-base.js#1016

- getting the list of the tabs in the window because of the `populate` option of the getLastFocused API call:
  https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/toolkit/components/extensions/parent/ext-tabs-base.js#856

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review commit: https://reviewboard.mozilla.org/r/242104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242104/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242102/#review247922

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:238
(Diff revision 1)
> +      // Keep calling getLastFocused while browser.windows.create is creating
> +      // a new window to adopt the test tab, so that the test recreates
> +      // conditions similar to the extension that has been triggered this leak
> +      // (See Bug 1458918 for a rationale).
> +      Promise.resolve().then(async () => {
> +        while (true) {
> +          browser.windows.getLastFocused({populate: true});
> +          // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
> +          await new Promise(resolve => setTimeout(resolve, 50));
> +        }
> +      });

I'm not thrilled by this way of triggering the leak (the infinite loop that keeps calling windows.getLastFocused), but I've tried a bunch of different approaches (and different events) and I wasn't able to consistently trigger the issue from the test (it was triggered but very intermittently).

Then I tried to use this approach because it is pretty similar to the way the actualy extension (Tile Tabs WE) has been able to trigger this issue, and it is consistently triggering it in the test case as well.

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:291
(Diff revision 1)
> +  // Check that no tabs have been leaked by the internal tabTracker helper class.
> +  const {ExtensionParent} = ChromeUtils.import("resource://gre/modules/ExtensionParent.jsm", {});
> +  const {tabTracker} = ExtensionParent.apiManager.global;
> +
> +  for (const [tabId, nativeTab] of tabTracker._tabIds) {
> +    if (!nativeTab.ownerGlobal) {
> +      ok(false, `A tab with tabId ${tabId} has been leaked by the WebExtensions' tabTracker`);
> +    }
> +  }
> +
> +  is(tabTracker._tabIds.size, 1, "Got the expected number of tab ids in tabTracker");

This is the part that explicitly checks for any tab leaked by tabTracker.

::: toolkit/components/extensions/parent/ext-webNavigation.js:125
(Diff revision 1)
>          const chromeWin = data.browser.ownerGlobal;
> -        if (chromeWin && chromeWin.arguments && chromeWin.arguments[0] instanceof chromeWin.XULElement &&
> +        const winWrapper = chromeWin && context.extension.windowManager.getWrapper(chromeWin);
> +        if (winWrapper && winWrapper.isAdoptingTab &&
>              chromeWin.gBrowser.selectedBrowser === data.browser) {
>            return;
>          }

This part is not actually mandatory for this fix, but if we are going to defined an `isAdoptingTab` property in the `WindowBase` wrapper class, it seems better if we could use that property to recognize "a window that is adopting a tab" also for the fix applied to the webNavigation API by Bug 1443221.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/1-2/

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review247932

Overall I think that having nativeTab.inAdoptingMove (or something like that) would be better than having some of the specifics at this level.  See following comments.

::: browser/components/extensions/parent/ext-browser.js:857
(Diff revision 1)
> +    // Return an empty iterator when the window is being opened to
> +    // adopt a tab.

I'm not sure I like this but I'm not sure where better to address it yet.

::: toolkit/components/extensions/parent/ext-tabs-base.js:1021
(Diff revision 1)
> +  get isAdoptingTab() {
> +    return this.window.arguments && this.window.arguments[0] instanceof this.window.XULElement;
> +  }

I'm not clear that window.arguments is a good idea.  It seems that it is cleared[1] so we could be in some race condition here.

[1] https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/browser/base/content/browser.js#1357-1374

Perhaps a promise could be added on gBrowserInit that is resolved when the initial tab is ready?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

Review commit: https://reviewboard.mozilla.org/r/242820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242820/

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/2-3/

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8974485 [details]
Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

https://reviewboard.mozilla.org/r/242820/#review248666

::: browser/components/extensions/parent/ext-browser.js:709
(Diff revision 1)
>        hidden: tabData.state ? tabData.state.hidden : tabData.hidden,
>        incognito: Boolean(tabData.state && tabData.state.isPrivate),
>        lastAccessed: tabData.state ? tabData.state.lastAccessed : tabData.lastAccessed,
>      };
>  
> -    if (extension.tabManager.hasTabPermission(tabData)) {
> +    if (extension.hasPermission("tabs")) {

The leak check that I added in the new test case has made me find this other scenario that was leaking tabIds:

The session API was implicitly allocating a tabId and stored in the `tabTracker._tabIds` Map, and they are never going to be removed because we cleanup that map when a tab is closed, and here we are actually passing the tabData from the session instead of a real tab, and so there will never be a tab closed event for them.

I looked into what extension.tabManager.hasTabPermission is actually checking for this method and the only part that should matter for this tab data is actually the "tabs" permission of the extension (because a tab closed and stored in the session cannot be the activeTab and so there should be any reason to check that other condition in this method).

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review247932

> I'm not sure I like this but I'm not sure where better to address it yet.

My current vision is that if an extension calls getLastFocused (or another API which lists all the tabs for a given window), it should be reasonable enough to get an empty tabs array in the result of the API call, while the tab is still being adopted, but a new API call after the adoption phase is going to return that tab in the tabs property as expected (and it is one of the conditions that the new test verifies explicitly, when it calls `browser.windows.getLastFocused({populate: true})` one last time once the tab adoption has been completed).

> I'm not clear that window.arguments is a good idea.  It seems that it is cleared[1] so we could be in some race condition here.
> 
> [1] https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/browser/base/content/browser.js#1357-1374
> 
> Perhaps a promise could be added on gBrowserInit that is resolved when the initial tab is ready?

In the updated patch I've changed it as we briefly discussed over IRC, and gBrowserInit is not where everything related to the 'first tab adoption' is abstracted.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8974485 [details]
Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

https://reviewboard.mozilla.org/r/242820/#review248684

::: browser/components/extensions/parent/ext-browser.js:709
(Diff revision 1)
>        hidden: tabData.state ? tabData.state.hidden : tabData.hidden,
>        incognito: Boolean(tabData.state && tabData.state.isPrivate),
>        lastAccessed: tabData.state ? tabData.state.lastAccessed : tabData.lastAccessed,
>      };
>  
> -    if (extension.tabManager.hasTabPermission(tabData)) {
> +    if (extension.hasPermission("tabs")) {

Add a comment here, something along the line of:

tabManager.hasTabPermission has the side affect of creating tabId.  Having that happen here would result in invalid and leaking tabIds.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review247932

> In the updated patch I've changed it as we briefly discussed over IRC, and gBrowserInit is not where everything related to the 'first tab adoption' is abstracted.

typo in the above comment :-(

s/gBrowserInit is not where/gBrowserInit is now where/

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review248712

Looks good.  I'd like to have Gijs or Dao review the gBrowserInit changes as well.

::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:259
(Diff revision 3)
> +      // Keep calling getLastFocused while browser.windows.create is creating
> +      // a new window to adopt the test tab, so that the test recreates
> +      // conditions similar to the extension that has been triggered this leak
> +      // (See Bug 1458918 for a rationale).
> +      Promise.resolve().then(async () => {
> +        while (true) {

I don't like this.  Can you use a flag that you set before notifyPass?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8974485 [details]
Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242820/diff/1-2/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/3-4/

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8974485 [details]
Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

Hi :dao,
do you mind to take a look at the 'browser/base/content/browser.js' bits from this patch for an additional sign off? (https://reviewboard.mozilla.org/r/242104/diff/4#1)

This patch contains a proposed change to gBrowserInit, to avoid to directly checking the condition `window.arguments && window.arguments[0] instanceof window.XULElement` in the WebExtensions internals.

We were already using this check in a couple of places (e.g. ext-browser.js and ext-webNavigation.js) to detect a window that is in the process of adopting its first tab as soon as it is opened, Shane and I agreed that it would be nicer if we could let gBrowserInit be responsible of the implementation details of this check.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review252498

::: browser/base/content/browser.js:1187
(Diff revision 4)
>  var gBrowserInit = {
>    delayedStartupFinished: false,
>  
> +  adoptingTab: null,
> +
> +  getAdoptingTab() {

nit: let's rename this to getTabToAdopt

::: browser/base/content/browser.js:1202
(Diff revision 4)
> +      // Clear the reference of the tab being adopted from the arguments.
> +      window.arguments[0] = null;
> +    }
> +
> +    return this.adoptingTab;
> +  },

The way you implemented this, getAdoptingTab will try to find the tab again and again when being called repeatedly in a window that isn't adopting a tab. Instead, it should run once and save the result.

Comment 16

[Dão Gottwald [:dao]]

Sorry for the delay. When you post an updated patch, I'll review that quicker.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/4-5/

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review252498

> The way you implemented this, getAdoptingTab will try to find the tab again and again when being called repeatedly in a window that isn't adopting a tab. Instead, it should run once and save the result.

Yeah, and that is definitely something that it shouldn't happen. I've renamed the method to getTabToAdopt as suggested (and renamed also the property accordingly) and then added a boolean flag that is used to check if there ever was a tab to adopt when the window has been opened.

Let me know how it looks to you.

Thanks a lot for your review comments.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review252596

::: browser/base/content/browser.js:1185
(Diff revision 5)
>  });
>  
>  var gBrowserInit = {
>    delayedStartupFinished: false,
>  
> +  openedWithTabToAdopt: window.arguments && window.arguments[0] instanceof window.XULElement,

You can get rid of this if you let getTabToAdopt differentiate between tabToAdopt being undefined and it being null. I.e. make it undefined initially, set it to null when getTabToAdopt doesn't find a tab in window.arguments, and return early when tabToAdopt isn't undefined.

::: browser/base/content/browser.js:1186
(Diff revision 5)
>  
>  var gBrowserInit = {
>    delayedStartupFinished: false,
>  
> +  openedWithTabToAdopt: window.arguments && window.arguments[0] instanceof window.XULElement,
> +  tabToAdopt: null,

Please rename this to _tabToAdopt to indicate it's private / not meant for use outside of gBrowserInit.

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/5-6/

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review252648

::: browser/base/content/browser.js:1206
(Diff revision 6)
> +    }
> +
> +    return this._tabToAdopt;
> +  },
> +
> +  clearTabToAdopt() {

I'm not sure why you added this method since it's only called once. If you want to keep it anyway, please rename it to _clearTabToAdopt.

::: browser/base/content/browser.js:1404
(Diff revision 6)
>        } catch (e) {
>          Cu.reportError(e);
>        }
> +
> +      // Clear the reference to the tab once its adoption has been completed.
> +      this.clearTabToAdopt() ;

nit: remove the space before the semicolon

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/6-7/

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review252648

> I'm not sure why you added this method since it's only called once. If you want to keep it anyway, please rename it to _clearTabToAdopt.

I was also a bit in doubt about it (sorry, I should really have mentioned why I added in a reply to the previous review comments). I had mainly two concerns:

- ensuring that `this._tabToAdopt` is set to `null` (and not `undefined`) to clear it (to be fair I think that it is extremely unlikely that it would be set to `undefined` by mistake in a further change)

- ensuring that the right property is set to `null` when we are going to clear the adopted tab, and that a typo would less likely be missed

I've renamed it to `_clearTabToAdopt`, but let me know if the above looks like unreasonable concerns and I'll happily remove it in favor of just setting the property to null where we are calling it in the current version of the patch.

> nit: remove the space before the semicolon

ouch, I should have not trusted that eslint would have caught this kind of styling issue on this file.

Fixed.

Comment 24

[Dão Gottwald [:dao]]

(In reply to Luca Greco [:rpl] from comment #23)
> Comment on attachment 8973735 [details]
> Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted
> by a new window.
> 
> https://reviewboard.mozilla.org/r/242104/#review252648
> 
> > I'm not sure why you added this method since it's only called once. If you want to keep it anyway, please rename it to _clearTabToAdopt.
> 
> I was also a bit in doubt about it (sorry, I should really have mentioned
> why I added in a reply to the previous review comments). I had mainly two
> concerns:
> 
> - ensuring that `this._tabToAdopt` is set to `null` (and not `undefined`) to
> clear it (to be fair I think that it is extremely unlikely that it would be
> set to `undefined` by mistake in a further change)
> 
> - ensuring that the right property is set to `null` when we are going to
> clear the adopted tab, and that a typo would less likely be missed
> 
> I've renamed it to `_clearTabToAdopt`, but let me know if the above looks
> like unreasonable concerns and I'll happily remove it in favor of just
> setting the property to null where we are calling it in the current version
> of the patch.

Fine with me either way.

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8974485 [details]
Bug 1458918 - Prevent the session API from leaking tabIds when checking tab permissions on session data.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242820/diff/2-3/

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242104/diff/7-8/

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8973735 [details]
Bug 1458918 - Prevent windows.getLastFocused from leaking tab being adopted by a new window.

https://reviewboard.mozilla.org/r/242104/#review247932

> My current vision is that if an extension calls getLastFocused (or another API which lists all the tabs for a given window), it should be reasonable enough to get an empty tabs array in the result of the API call, while the tab is still being adopted, but a new API call after the adoption phase is going to return that tab in the tabs property as expected (and it is one of the conditions that the new test verifies explicitly, when it calls `browser.windows.getLastFocused({populate: true})` one last time once the tab adoption has been completed).

I tweaked a bit the inline comment here as Shane and I agreed over IRC, to more clearly explain why it is reasonable to return an empty iterator here (and also rebased the two patches on a more recent mozilla-central tip).

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2359fe9a68a6
Prevent the session API from leaking tabIds when checking tab permissions on session data. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c27c992dc802
Prevent windows.getLastFocused from leaking tab being adopted by a new window. r=dao,mixedpuppy

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2359fe9a68a6
https://hg.mozilla.org/mozilla-central/rev/c27c992dc802

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2359fe9a68a6
https://hg.mozilla.org/mozilla-central/rev/c27c992dc802

Comment 31

[CosminB]

Attachment: Bug1458918.gif

I can reproduce this issue on Firefox 62.0.1 (20180525102548) under Win 7 64-bit and Mac OS X 10.13.3.

The issue can be reproduced with the (Alt+Shift+RightArrow on Windows/Linux or MacCtrl+Shift+RightArrow on Mac) commands.

This issue is verified as fixed on Firefox 62.0a1 (20180606220131) under Win 7 64-bit and Mac OS X 10.13.3.