1279012 - implement chrome.runtime.reload() and onUpdateAvailable event

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Robert Helmer [:rhelmer]]

Web extensions should be able to listen for available updates and delay them, either until they are ready to explicitly confirm or upon application restart.

Per https://developer.chrome.com/extensions/runtime#event-onUpdateAvailable web extensions should be able to listen for the onUpdateAvailable event:

chrome.runtime.onUpdateAvailable.addListener(function callback)

The callback is passed two arguments: https://developer.chrome.com/extensions/runtime#type-RequestUpdateCheckStatus and also an optional "details" object, which may contain additional details such as the version string for the pending update.

If the extensions wishes to apply the update then it may call chrome.runtime.reload() to apply the update.

chrome.runtime.reload() without a pending update will just reload the extension.

This should be implemented using the new AddonManager.addUpgradeListener() API added in bug 1231172.

Comment 1

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #0)
> The callback is passed two arguments:
> https://developer.chrome.com/extensions/runtime#type-
> RequestUpdateCheckStatus and also an optional "details" object, which may
> contain additional details such as the version string for the pending update.

Correction: that's what the callback for chrome.runtime.requestUpdateCheck passes to its callback ^

onUpdateAvailable.addListener only needs to pass the "details" object, and it just contains a version string.

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 3

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/68602/#review65696

::: toolkit/components/extensions/Extension.jsm:1277
(Diff revision 1)
>      } else {
>        throw Error("installType must be one of: temporary, permanent");
>      }
>    },
>  
> -  shutdown() {
> +  shutdown(uninstall=true) {

The reason this is necessary is that in order to test updates, we need to restart the AddonManager to ensure that deferred updates are applied, so having `extension.unload()` uninstall the extension isn't helpful (since after restart the extension will be loaded via the AddonManager and not via explicit `extension.startup()` in the test.)

Comment 4

[Robert Helmer [:rhelmer]]

This is logged when shutting down the extension after restart:

0:02.23 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1470158836750	addons.xpi	WARN	Exception running bootstrap method shutdown on [e-mail]: TypeError: extension is undefined (resource://gre/modules/ExtensionManagement.jsm:145:5) JS Stack trace: [e-mail]:145:5 < [e-mail]:1521:7 < [e-mail]:23:3 < [e-mail]:4805:9 < this.XPIProvider.startup/<[e-mail]:2815:15 < promiseShutdownManager@head_addons.js:805:3 < shutdownManager@head_addons.js:796:20 < delay_updates_defer@test_delay_update_webextension.js:253:3 < [e-mail]:1564:9 < do_execute_soon/<[e-mail]:712:9 < [e-mail]:209:5 < [e-mail]:544:5 < @-e:1:1"

We could check that `extension` is defined, but maybe I'm just using this wrong? :)

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> This is logged when shutting down the extension after restart:
> 
> 0:02.23 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (info) 1470158836750	addons.xpi
> WARN	Exception running bootstrap method shutdown on
> [e-mail]: TypeError: extension is
> undefined (resource://gre/modules/ExtensionManagement.jsm:145:5) JS Stack
> trace: [e-mail]:145:5 <
> [e-mail]:1521:7 < [e-mail]:23:3 <
> [e-mail]:4805:9 <
> this.XPIProvider.startup/<[e-mail]:2815:15 <
> promiseShutdownManager@head_addons.js:805:3 <
> shutdownManager@head_addons.js:796:20 <
> delay_updates_defer@test_delay_update_webextension.js:253:3 <
> [e-mail]:1564:9 < do_execute_soon/<[e-mail]:712:9 <
> [e-mail]:209:5 < [e-mail]:544:5 < @-e:1:1"
> 
> We could check that `extension` is defined, but maybe I'm just using this
> wrong? :)

Disregard, figured this out. We need to wait for the WebExtension to actually finish startup, all of the post-upgrade extensions in this patch are just `manifest.json` and nothing else, so they didn't have time to complete startup before we started to shut the managers down.

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 7

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68602/#review65770

::: toolkit/components/extensions/ExtensionXPCShellUtils.jsm:130
(Diff revision 2)
> +    if (uninstall) {
> -    this.extension.shutdown();
> +      this.extension.shutdown();
> +    }

I don't get this, what's the difference between just not calling unload() and calling unload() with uninstall=false?  I see that it sets the state property, does something reference that later?

::: toolkit/components/extensions/ext-runtime.js:5
(Diff revision 2)
>  "use strict";
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +Cu.import("resource://gre/modules/AddonManager.jsm");

This could be made lazy since it isn't actually referenced until a listener is added to onUpdateAvailable.

::: toolkit/components/extensions/ext-runtime.js:37
(Diff revision 2)
>        onMessage: context.messenger.onMessage("runtime.onMessage"),
>  
>        onConnect: context.messenger.onConnect("runtime.onConnect"),
>  
> +      onUpdateAvailable: new EventManager(context, "runtime.onUpdateAvailable", fire => {
> +        extension.onUpdateAvailable = fire;

is this used anywhere?

::: toolkit/components/extensions/ext-runtime.js:41
(Diff revision 2)
> +      onUpdateAvailable: new EventManager(context, "runtime.onUpdateAvailable", fire => {
> +        extension.onUpdateAvailable = fire;
> +        let instanceID = extension.addonData.instanceID;
> +        AddonManager.addUpgradeListener(instanceID, upgrade => {
> +            extension.upgrade = upgrade;
> +            let details = upgrade.details();

The details object is quite simple but as a general rule, I think it would make sense to have the code that constructs it live here in this file, since the structure of that object is really defined by the webextensions/browserext standard.

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js:10
(Diff revision 2)
> +// This verifies that delaying an update works for WebExtensions.
> +
> +// The test extension uses an insecure update url.
> +Services.prefs.setBoolPref(PREF_EM_CHECK_UPDATE_SECURITY, false);
> +// Do not require signing.
> +Services.prefs.setBoolPref(PREF_XPI_SIGNATURES_REQUIRED, false);

this shouldn't be necessary for xpcshell?

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js:106
(Diff revision 2)
> +
> +  yield extension.awaitFinish("delay");
> +
> +  // restarting allows upgrade to proceed
> +  yield extension.unload(false);
> +  restartManager();

can you "yield promiseRestartManager()" here instead?

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js:120
(Diff revision 2)
> +  do_check_false(addon_upgraded.appDisabled);
> +  do_check_true(addon_upgraded.isActive);
> +  do_check_eq(addon_upgraded.type, "extension");
> +
> +  yield addon_upgraded.uninstall();
> +  shutdownManager();

"yield promiseShutdownManager()" ?

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 9

[Robert Helmer [:rhelmer]]

https://reviewboard.mozilla.org/r/68602/#review65770

> I don't get this, what's the difference between just not calling unload() and calling unload() with uninstall=false?  I see that it sets the state property, does something reference that later?

We discussed this - the problem is that it calls `shutdown()`, and `MockExtension` uninstalls the extension at shutdown. What we really want here is for `ExtensionWrapper` to mark the extension as shutdown, for this test we want to have more control over the addons manager.

To make this clearer, I am going to add a new method, `markUnloaded()`, which does just this part and has comments to explain why you would want to use it.

> is this used anywhere?

Hm I think this was vestigial from an older approach I took, I removed it.

Comment 10

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/68602/#review66518

::: toolkit/components/extensions/ext-runtime.js:38
(Diff revision 3)
>  
>        onMessage: context.messenger.onMessage("runtime.onMessage"),
>  
>        onConnect: context.messenger.onConnect("runtime.onConnect"),
>  
> +      onUpdateAvailable: new EventManager(context, "runtime.onUpdateAvailable", fire => {

We've been moving toward using SingletonEventManager instead of EventManager

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 13

[Robert Helmer [:rhelmer]]

Hm, I don't think the runtime.reload() test is good enough. Going to amend that momentarily.

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 15

[Robert Helmer [:rhelmer]]

This is all working, but I realized that the `addon.userDisabled` setter won't work for hidden add-ons (this is on purpose from bug 1281077).

Instead, we can make the `addon.reload()` method do the right thing here for non-temp add-ons, which simplifies the webextension side of the code.

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 17

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 18

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

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

Comment 19

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68602/diff/9-10/

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

https://reviewboard.mozilla.org/r/68602/#review68798

::: toolkit/components/extensions/ext-runtime.js:49
(Diff revision 10)
> +            version: upgrade.version(),
> +          };
> +          context.runSafe(fire, details);
> +        });
> +        return () => {
> +          extension.upgrade = null;

Why do you clear this here?  Its a peculiar sequence and I'm not sure why anyone would do it but with this sequence of events I would expect the update to be applied:
1. extension adds an onUpdateAvailable listener
2. receives an update but doesn't immediately call reload()
3. removes its onUpdateAvailable listener
4. calls reload()

::: toolkit/components/extensions/ext-runtime.js:60
(Diff revision 10)
> +        if (extension.upgrade) {
> +          // If there is a pending update, install it now.
> +          extension.upgrade.install();
> +        } else {
> +          // Otherwise, reload the current extension.
> +          return new Promise(resolve =>

This method isn't supposed to return anything

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6206
(Diff revision 10)
>  
>              // upgrade has been staged for restart, notify the add-on and give
>              // it a way to resume.
>              let callback = AddonManagerPrivate.getUpgradeListener(this.addon.id);
>              callback({
> +              version: () => this.version,

why is this a function and not just a simple property?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7644
(Diff revision 10)
> +        XPIProvider.updateAddonDisabledState(addon, false);
> +        let addonFile = addon.getResourceURI;
> +        Services.obs.notifyObservers(addonFile, "flush-cache-entry", null);

The flush should be after disabling but before re-enabling

::: toolkit/mozapps/extensions/test/xpcshell/test_reload.js:88
(Diff revision 10)
>  
> -add_task(function* test_cannot_reload_permanent_addon() {
> +add_task(function* test_can_reload_permanent_addon() {
>    yield promiseRestartManager();
>    const addon = yield installAddon(sampleAddon.name, sampleAddon.id);
>  
> -  yield Assert.rejects(addon.reload(), /Only temporary add-ons can be reloaded/);
> +  addon.reload();

This test should also test that onDisabled / onEnabled get triggered, that the addon is still active, etc.

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini:41
(Diff revision 10)
>  [test_proxies.js]
>  [test_proxy.js]
>  [test_pass_symbol.js]
>  [test_delay_update.js]
>  [test_nodisable_hidden.js]
> +[test_delay_update_webextension.js]

Since this is primarily testing webextension apis, I think it would ideally be in toolkit/components/extensions.  But the certificate shimming code is currently limited to toolkit/mozapps/extensions so that would take some extra work.  Its fine to skip that for now but can you file a bug to make the shimming code reusable in other places and note that this test should move when that is done?

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68602/diff/10-11/

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

https://reviewboard.mozilla.org/r/68602/#review68798

> Since this is primarily testing webextension apis, I think it would ideally be in toolkit/components/extensions.  But the certificate shimming code is currently limited to toolkit/mozapps/extensions so that would take some extra work.  Its fine to skip that for now but can you file a bug to make the shimming code reusable in other places and note that this test should move when that is done?

Filed bug 1294811, also mentioned in there that being able to restart the AddonManager is something we can't do in `toolkit/components/extensions` tests yet AFAICT.

Comment 23

[Andrew Swan [:aswan]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

https://reviewboard.mozilla.org/r/68602/#review68876

nice!

::: toolkit/components/extensions/ext-runtime.js:59
(Diff revision 11)
> +          new Promise(resolve =>
> +            AddonManager.getAddonByID(extension.id, addon => {
> +              addon.reload();
> +              resolve();
> +            })

the Promise is now unnecessary...

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68602/diff/11-12/

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8777012 [details]
Bug 1279012 - implement onUpdateAvailable and runtime.reload() for WebExtensions

https://reviewboard.mozilla.org/r/68602/#review68898

::: toolkit/components/extensions/ExtensionUtils.jsm:819
(Diff revision 12)
>  //
>  // The result is an object with addListener, removeListener, and
>  // hasListener methods. |context| is an add-on scope (either an
>  // ExtensionContext in the chrome process or ExtensionContext in a
>  // content process). |name| is for debugging. |register| is a function
> -// to register the listener. |register| is only called once, event if
> +// to register the listener. |register| is only called once, even if

Hello. \o

Comment 26

[Robert Helmer [:rhelmer]]

(In reply to Kris Maglione [:kmag] from comment #25)
> Comment on attachment 8777012 [details]
> Bug 1279012 - implement onUpdateAvailable and runtime.reload() for
> WebExtensions
> 
> https://reviewboard.mozilla.org/r/68602/#review68898
> 
> ::: toolkit/components/extensions/ExtensionUtils.jsm:819
> (Diff revision 12)
> >  //
> >  // The result is an object with addListener, removeListener, and
> >  // hasListener methods. |context| is an add-on scope (either an
> >  // ExtensionContext in the chrome process or ExtensionContext in a
> >  // content process). |name| is for debugging. |register| is a function
> > -// to register the listener. |register| is only called once, event if
> > +// to register the listener. |register| is only called once, even if
> 
> Hello. \o

There appears to be a bug in mozreview (bug 1294917) - holding off on landing this until mozreview folks have a chance to take a look.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e88a4b19b2d1
implement onUpdateAvailable and runtime.reload() for WebExtensions r=aswan

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c91ca8250b4
implement onUpdateAvailable and runtime.reload() for WebExtensions: remove unnecessary semicolon. r=eslint-fix

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e88a4b19b2d1
https://hg.mozilla.org/mozilla-central/rev/7c91ca8250b4

Comment 30

[Will Bamberg [:wbamberg]]

I've updated the compat data for reload() and onUpdateAvailable, and tried to make the descriptions clearer too:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/reload
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onUpdateAvailable

Marking as dev-doc-complete, but please let me know if you see anything else needed.