1286526 - Exception raised on WebExtension shutdown on empty addon with a valid manifest and addons that have never been started

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

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

During a debugging sessions on tests that I'm preparing for Bug 1285557 running on a debug build, I've noticed that:

if GlobalManager.init is never called, during the shutdown GlobalManager.uninit raises an exception trying to unregister itself as an observer that has never been registered, and a leak is detected in the group of tests that I'm working on (which are "about:debugging" tests on addons):

- GlobalManager.init: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#507
- GlobalManager.uninit: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#518


During the above debugging session I noticed that some of the API modules can raise similar exceptions in the shutdown handler, because they doesn't check correctly (like other API modules do) if the extension has been started, e.g.:

- ext-backgroundPage.js shutdown handler checks are correct:
  http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-backgroundPage.js#148

- ext-notifications.js shutdown handler checks are missing:
  http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#84

Comment 1

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

Attachment: Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid manifest.

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

Comment 2

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

(In reply to Luca Greco [:rpl] from comment #1)
> Created attachment 8770522 [details]
> Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid
> manifest.
> 
> Review commit: https://reviewboard.mozilla.org/r/63928/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/63928/

Attached patch with a proposed fix for the issues described above.

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8770522 [details]
Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid manifest.

https://reviewboard.mozilla.org/r/63928/#review61028

Looks good, just a nitpick about the name of the property.

::: toolkit/components/extensions/Extension.jsm:504
(Diff revision 1)
>  // Responsible for loading extension APIs into the right globals.
>  GlobalManager = {
>    // Map[extension ID -> Extension]. Determines which extension is
>    // responsible for content under a particular extension ID.
>    extensionMap: new Map(),
> +  removeObserverNeeded: false,

can we just call this "initialized"?

Comment 4

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

Comment on attachment 8770522 [details]
Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid manifest.

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

Comment 5

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

https://reviewboard.mozilla.org/r/63928/#review61028

> can we just call this "initialized"?

Absolutely, changed in the updated patch.

Comment 6

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

https://reviewboard.mozilla.org/r/63928/#review61056

::: toolkit/components/extensions/Extension.jsm:481
(Diff revision 2)
> -let UninstallObserver = {
> +var UninstallObserver = {
>    initialized: false,
>  
>    init: function() {
>      if (!this.initialized) {
>        AddonManager.addAddonListener(this);
>        this.initialized = true;
>      }
>    },
>  
> +  uninit: function() {
> +    if (this.initialized) {
> +      AddonManager.removeAddonListener(this);
> +      this.initialized = false;
> +    }
> +  },

Besides the change suggested in the previous review comment, in the updated patch I've added an uninit method in the UninstallObserver (it is not a big issue, but we can unregister the AddonManager listener if there isn't any webextensions add-on installed) and added a new test (named "test_chrome_ext_shutdown_cleaup.html") that checks that the GlobalManager and the UninstallObserver are both initialized when the first extension has been loaded and uninitialized when the last extension has been unloaded.

NOTE: the UninstallObserver definition has been changed from "let" to "var" to be able to reach the object from the new test.

Comment 7

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

Push to try of the updated patch:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=83e9c202eeeaa98b74f8f6caa578fb4ef3c6a0b9

Comment 8

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

Comment on attachment 8770522 [details]
Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid manifest.

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

Comment 9

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

(In reply to Luca Greco [:rpl] from comment #8)
> Comment on attachment 8770522 [details]
> Bug 1286526 - [webext] Fix shutdown issues on empty addon with a valid
> manifest.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/63928/diff/2-3/

Patch rebased on a recent fx-team tip.

Comment 10

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

Push to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c9ce109a6f

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19ff5ac3420f
[webext] Fix shutdown issues on empty addon with a valid manifest. r=aswan

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19ff5ac3420f