1438985 - tabhide shutdown doesn't happen if api is unused

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

shutdown only occurs if the api has been used.  This can result in hidden tabs the user is unable to show.

STR:

- load a tab hide extension
- hide some tabs
- restart firefox
- upon restart, do not use extension
- open about:addons
- uninstall tab hide extension

expected:

hidden tabs are shown

actual:

hidden tabs are not shown

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1438985 handle shutdown and uninstall outside of api

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951758 [details]
Bug 1438985 handle shutdown and uninstall outside of api

This allows us to avoid any more general changes for now.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1438985 fix showing hidden tabs on extension shutdown,

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

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review227012

::: commit-message-9b69c:1
(Diff revision 1)
> +Bug 1438985 handle shutdown and uninstall outside of api

The "outside of api" bit here doesn't make any sense to me.  In any case, the commit message should mention that this is about the webextensions tabs api, not require somebody to read the patch to know what it is about.

::: toolkit/components/extensions/ExtensionParent.jsm:111
(Diff revision 1)
>        }));
>      });
> +
> +    this.on("disable", (e, {id}) => {
> +      let modules = this.eventModules.get("disable");
> +      return Promise.all(Array.from(modules).map(async apiName => {

what actually looks at this promise?

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco] (PTO -> Jan 7 2026)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review227560

::: browser/components/extensions/ext-tabs.js:105
(Diff revision 1)
> -          nativeTab.ownerGlobal.gBrowser.showTab(nativeTab);
> -        }
> -      }
> -    }
> +  }
> +
> +  static onUninstall(id) {

Like I described in a bit more detail in Bug 1436720 Comment 43, this static onUninstall is going to be called when the addon has been removed and not when it is "shutting down with an ADDON_UNINSTALL reason".

I think that the new behavior can be a bit more surprising for the user than the previous one (which was "to show the hidden tabs when the extension is shutting down with an "ADDON_DISABLE" or "ADDON_UNINSTALL" reason)

e.g. the hidden tabs are not going to appear when the user click the disable button, which most of the times is a direct action of the user (well, besides an extension that is being disabled by Firefox, e.g. because of the signature checks), but when the "about:addons" tab is closed or reloaded.

Given that we are defining a new static lifecycle method (currently named onDisabled), another option would be to provide a static lifecycle method that gets the reason for the extension shutdown as a parameter, this way we can just support the exact same behavior implemented by the previous non-static onShutdown lifecycle method.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

Still only feedback due to lack of test for updates.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review229458

::: toolkit/components/extensions/ExtensionParent.jsm:109
(Diff revision 2)
> +    this.on("disable", (e, {id, reason}) => {
> +      let modules = this.eventModules.get("disable");
> +      Array.from(modules).map(async apiName => {
> +        let module = await this.asyncLoadModule(apiName);
> +        module.onDisable(id, reason);
> +      });
> +    });

I don't think a new event is necessary here, you can just put this logic into `shutdownExtension()`

Also, I would prefer that we don't pass `reason` here but only actually dispatch `onDisable()` if reason is ADDON_DISABLE.  We already have the uninstall and update events for some of the other instances in which an extension can be shut down, and I don't think we have a need for this event at app shutdown, so we shouldn't be loading APIs then.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review229458

> I don't think a new event is necessary here, you can just put this logic into `shutdownExtension()`
> 
> Also, I would prefer that we don't pass `reason` here but only actually dispatch `onDisable()` if reason is ADDON_DISABLE.  We already have the uninstall and update events for some of the other instances in which an extension can be shut down, and I don't think we have a need for this event at app shutdown, so we shouldn't be loading APIs then.

The reason for doing this for addon_uninstall as well is described in comment 5.

Comment 10

[Andrew Swan [:aswan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> The reason for doing this for addon_uninstall as well is described in
> comment 5.

/me shakes fist at undo-able uninstalls

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

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

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review230152

no test for the disable handling?  a few little things but r=me with those addressed

::: browser/components/extensions/ext-tabs.js:108
(Diff revision 3)
> -          nativeTab.ownerGlobal.gBrowser.showTab(nativeTab);
> -        }
> +    }
> -      }
> +  }
> +
> +  static onDisable(id, reason) {
> +    if (["ADDON_DISABLE", "ADDON_UNINSTALL"].includes(reason)) {

Lets just define this event as happening for reasons ADDON_DISABLE and ADDON_UNINSTALL and move this check into the code in ExtensionParent.jsm that dispatches this call.

::: browser/components/extensions/test/browser/browser_ext_tabs_hide_update.js:3
(Diff revision 3)
> +"use strict";
> +
> +const {AddonTestUtils} = ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", {});

Is there a reason that this won't work by itself:
`ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm");`

::: browser/components/extensions/test/browser/browser_ext_tabs_hide_update.js:7
(Diff revision 3)
> +
> +const {AddonTestUtils} = ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm", {});
> +
> +AddonTestUtils.initMochitest(this);
> +
> +async function reloadWebExtension(ID, options) {

nit: this function name is misleading...

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review230152

> Is there a reason that this won't work by itself:
> `ChromeUtils.import("resource://testing-common/AddonTestUtils.jsm");`

In some earlier iteration I got test failures due to MockAsyncShutdown remaining on the window, but that isn't happening now.  It was probably something I was trying before I figured out the right way to do it.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

https://reviewboard.mozilla.org/r/221056/#review230152

> In some earlier iteration I got test failures due to MockAsyncShutdown remaining on the window, but that isn't happening now.  It was probably something I was trying before I figured out the right way to do it.

ahh, one must use "this" to avoid that problem.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8951788 [details]
Bug 1438985 fix showing hidden tabs on extension shutdown,

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #12)
> Comment on attachment 8951788 [details]
> Bug 1438985 fix showing hidden tabs on extension shutdown,
> 
> https://reviewboard.mozilla.org/r/221056/#review230152
> 
> no test for the disable handling?

added

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4ed9c1d6ba7
fix showing hidden tabs on extension shutdown, r=aswan

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d4ed9c1d6ba7

Comment 20

[CosminB]

Attachment: Bug1438985.gif

This issue is verified as fixed on Firefox 60.0a1 (20180304220118) under Windows 7 64-bit, Mac OS X 10.13.2.

After I uninstall the extension, the hidden tabs are restored.

Please see the attached video.