1409245 - Implement a bootstrap "update" method

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement, P2)

Description

[Andrew Swan [:aswan]]

Right now, when a bootstrapped extension is updated, we call a series of bootstrap methods:

disable() on the old extension
uninstall() on the old extension
install() on the new extension
enable() on the new extension

In the WebExtensions code for handling preferences that can be affected by extensions, we have some elaborate code to try to handle this sequence of events but it has led to some bugs including bug 1407029 and bug 1404584.  The preferences code would be much simpler and those bugs would be easy to fix if we didn't make the WebExtensions framework split the work of an update across several handlers, but just generated a single update() event when we decide to go ahead with the update.

There are a bunch of problems with doing this for things that aren't WebExtensions so I propose that (at least for now) we limit this to the case where both the old and new extensions are WebExtensions.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1409245 Part 3 Handle update and uninstall events in WebExtensions

This patch just forwards invocations of the bootstrap update() and
uninstall() methods to events that other parts of the WebExtension
framework can listen for.  We have an existing event that fires while
an extension is being uninstalled.  This event was named "uninstall"
but it gets renamed here to "onUninstalling" to disambiguate the two
events.

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

Comment 2

[Andrew Swan [:aswan]]

In case this doesn't make sense by itself, I'll push my wip patches to bug 1404584 too, that might be useful context for reviewing the patches here.

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1409245 Part 1 Move temporary addon cleanup earlier in shutdown

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

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1409245 Part 2 Implement bootstrap update() method

When we are updating versions of an extension, instead of
calling uninstall() on the old version followed by calling
install() on the new version, we now call update() once with
details about the old and new versions.

This logic is only applied for webextensions, any other
bootstrapped extensions retain the old behavior.

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

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8922426 [details]
Bug 1409245 Part 1 Move temporary addon cleanup earlier in shutdown

https://reviewboard.mozilla.org/r/193456/#review199826

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2386
(Diff revision 1)
>      } catch (err) {
>        this._shutdownError = err;
>      }
>    },
>  
> +  _cleanupTemporaryAddons() {

No need for underscore. This is a non-public class.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8922427 [details]
Bug 1409245 Part 2 Implement bootstrap update() method

https://reviewboard.mozilla.org/r/193458/#review199828

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1809
(Diff revision 1)
> +          if (isWebExtension(this.addon.type) && isWebExtension(this.existingAddon.type)) {
> +            callUpdate = true;
> +          }

Nit: `callUpdate = isWebExtension(...) ...;`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3590
(Diff revision 1)
> +        if (isWebExtension(oldAddon.type) && isWebExtension(addon.type)) {
> +          callUpdate = true;
> +        }

Nit: `callUpdate = ...`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4675
(Diff revision 1)
> +      if (isWebExtension(aAddon.type) && isWebExtension(existingAddon.type)) {
> +        callUpdate = true;
> +      }

Nit: ...

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8922424 [details]
Bug 1409245 Part 3 Handle update and uninstall events in WebExtensions

https://reviewboard.mozilla.org/r/193454/#review199830

::: toolkit/components/extensions/Extension.jsm:248
(Diff revision 1)
>    onUninstalling(addon) {
>      let extension = GlobalManager.extensionMap.get(addon.id);
>      if (extension) {
>        // Let any other interested listeners respond
>        // (e.g., display the uninstall URL)
> -      Management.emit("uninstall", extension);
> +      Management.emit("onUninstalling", extension);

No `on` in EventEmitter event names, please.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8922426 [details]
Bug 1409245 Part 1 Move temporary addon cleanup earlier in shutdown

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

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8922427 [details]
Bug 1409245 Part 2 Implement bootstrap update() method

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

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8922424 [details]
Bug 1409245 Part 3 Handle update and uninstall events in WebExtensions

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa2de963897c
Part 1 Move temporary addon cleanup earlier in shutdown r=kmag
https://hg.mozilla.org/integration/autoland/rev/ffa3f56ba479
Part 2 Implement bootstrap update() method r=kmag
https://hg.mozilla.org/integration/autoland/rev/652456ce8315
Part 3 Handle update and uninstall events in WebExtensions r=kmag

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fa2de963897c
https://hg.mozilla.org/mozilla-central/rev/ffa3f56ba479
https://hg.mozilla.org/mozilla-central/rev/652456ce8315

Comment 13

[Victor Carciu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe verify-“ flag.

Thanks!