Closed Bug 1279012 Opened 8 years ago Closed 8 years ago

implement chrome.runtime.reload() and onUpdateAvailable event

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

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.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: triaged
Depends on: 1290617
(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.
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.)
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 test_delay_update_complete_webext@tests.mozilla.org: TypeError: extension is undefined (resource://gre/modules/ExtensionManagement.jsm:145:5) JS Stack trace: shutdownExtension@ExtensionManagement.jsm:145:5 < shutdown@Extension.jsm:1521:7 < shutdown@WebExtensionBootstrap.js:23:3 < this.XPIProvider.callBootstrapMethod@XPIProvider.jsm:4805:9 < this.XPIProvider.startup/<.observe@XPIProvider.jsm:2815:15 < promiseShutdownManager@head_addons.js:805:3 < shutdownManager@head_addons.js:796:20 < delay_updates_defer@test_delay_update_webextension.js:253:3 < _run_next_test@head.js:1564:9 < do_execute_soon/<.run@head.js:712:9 < _do_main@head.js:209:5 < _execute_test@head.js:544:5 < @-e:1:1"

We could check that `extension` is defined, but maybe I'm just using this wrong? :)
Flags: needinfo?(aswan)
(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
> test_delay_update_complete_webext@tests.mozilla.org: TypeError: extension is
> undefined (resource://gre/modules/ExtensionManagement.jsm:145:5) JS Stack
> trace: shutdownExtension@ExtensionManagement.jsm:145:5 <
> shutdown@Extension.jsm:1521:7 < shutdown@WebExtensionBootstrap.js:23:3 <
> this.XPIProvider.callBootstrapMethod@XPIProvider.jsm:4805:9 <
> this.XPIProvider.startup/<.observe@XPIProvider.jsm:2815:15 <
> promiseShutdownManager@head_addons.js:805:3 <
> shutdownManager@head_addons.js:796:20 <
> delay_updates_defer@test_delay_update_webextension.js:253:3 <
> _run_next_test@head.js:1564:9 < do_execute_soon/<.run@head.js:712:9 <
> _do_main@head.js:209:5 < _execute_test@head.js: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.
Flags: needinfo?(aswan)
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/
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 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/
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.
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
Attachment #8777012 - Flags: review?(aswan)
Hm, I don't think the runtime.reload() test is good enough. Going to amend that momentarily.
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 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?
Attachment #8777012 - Flags: review?(aswan)
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 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...
Attachment #8777012 - Flags: review?(aswan) → review+
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
(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.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e88a4b19b2d1
implement onUpdateAvailable and runtime.reload() for WebExtensions r=aswan
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/7c91ca8250b4
implement onUpdateAvailable and runtime.reload() for WebExtensions: remove unnecessary semicolon. r=eslint-fix
https://hg.mozilla.org/mozilla-central/rev/e88a4b19b2d1
https://hg.mozilla.org/mozilla-central/rev/7c91ca8250b4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.