1311508 - Embedded WebExtension not available after restart

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, defect, P1)

Description

[noitidart]

Attachment: embed.xpi

Steps to reproduce:
* I am using nightly.
* XPI consists of - `function startup(aData) { console.log('aData.webExtension:', aData.webExtension) }`
* Disable signing so you can install a XPI permanently. 
* Drag XPI and install it
* Restart Firefox, it is undefined, looping it with a nsITimer and you will see aData.webExtension never becomes available

Demo attached.

Comment 1

[noitidart]

Attachment: timer loop.xpi

Checks every 2 seconds to see if aData.webExtension is available. It never becomes available.

Comment 2

[Kris Maglione [:kmag]]

Apparently this is happening because we're not storing the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

Comment 3

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

Attachment: Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

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

Comment 4

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

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

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

Comment 5

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

Pushed to try:

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

https://reviewboard.mozilla.org/r/87140/#review86302

Main code changes look good, but we need to test that this actually still works after a browser restart.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4780
(Diff revision 2)
>     *         An array of add-on IDs on which this add-on depends.
>     * @return a JavaScript scope
>     */
>    loadBootstrapScope: function(aId, aFile, aVersion, aType,
>                                 aMultiprocessCompatible, aRunInSafeMode,
> -                               aDependencies) {
> +                               aDependencies, hasEmbeddedWebExtension) {

Please add to doc comment.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:96
(Diff revision 2)
> +  // Check that the addon and its hasEmbeddedWebExtension flag has been stored and
> +  // persisted to the `extensions.bootstrappedAddons` pref.
> +  ok(ID in XPIProvider.bootstrappedAddons,
> +     "Hybrid add-on listed in XPIProvider bootstrapped list.");
> +  equal(XPIProvider.bootstrappedAddons[ID].hasEmbeddedWebExtension, true,
> +        "hasEmbeddedWebExtension flag is stored in XPIProvider bootstrapped list.");
> +  let persisted = JSON.parse(Services.prefs.getCharPref("extensions.bootstrappedAddons"));
> +  ok(ID in persisted, "Hybrid add-on persisted to bootstrappedAddons.");
> +  equal(persisted[ID].hasEmbeddedWebExtension, true,
> +        "hasEmbeddedWebExtension flag persisted to bootstrappedAddons.");
> +

We should just test that it works after a browser restart rather than that it's included in this pref.

Comment 7

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

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

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

Comment 8

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

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

https://reviewboard.mozilla.org/r/87140/#review86302

> We should just test that it works after a browser restart rather than that it's included in this pref.

I suppose that "test that it works after a browser restart" is referred to the `promiseRestartManager()` test helper (but let me know if it is not and I will look into it again), and so I've reworked the test as follows (the actual change is much simpler than the interdiff that mozreview is generating):

- split the test to check that hasEmbeddedWebExtension has been persisted into a separate test case
- installed the addon as permanent (instead of being installed as a temporary addon like in the other tests from this file, or the addon will be uninstalled when we restart the AddonManager)
- restart the addon manager using `yield promiseRestartManager();` (like this scenario seems to be tested in "test_experiment.js")
- check that the addon is in the persisted bootstrappedAddons (like in "test_experiment.js")
- check that the addon still receive the expected `embeddedExtension` property in its bootstrap method after the addon manager restart

Comment 9

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

Last patch version pushed to try:

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

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

https://reviewboard.mozilla.org/r/87140/#review86728

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:5
(Diff revision 3)
> +var scope = Components.utils.import("resource://gre/modules/addons/XPIProvider.jsm");
> +const XPIProvider = scope.XPIProvider;
> +

No need for this import.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:60
(Diff revision 3)
> +function promisePermanentAddonInstall(xpiFile) {
> +  return new Promise((resolve, reject) => {
> +    AddonManager.getInstallForFile(xpiFile, install => {
> +      let listener = {
> +        onInstallFailed: reject,
> +        onInstallEnded: (install, newAddon) => {
> +          this.addon = newAddon;
> +          resolve();
> +        },
> +      };
> +
> +      install.addListener(listener);
> +      install.install();
> +    });
> +  });
> +}

This already exists as `promiseInstallFile`.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:116
(Diff revision 3)
> +  equal(XPIProvider.bootstrappedAddons[ID].hasEmbeddedWebExtension, true,
> +        "hasEmbeddedWebExtension flag is stored in XPIProvider bootstrapped list.");

Please remove these checks. They shouldn't be necessary.

Comment 11

[Kris Maglione [:kmag]]

Luca, can you please request uplift to 51 for this as soon as this lands? Thanks.

Comment 12

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

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

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

Comment 13

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

Last push to try:

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

Comment 14

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

(In reply to Kris Maglione [:kmag] from comment #11)
> Luca, can you please request uplift to 51 for this as soon as this lands?
> Thanks.

Sure, I've already tried to apply the patch on aurora (to be sure that it can be applied cleanly without any sort of conflict) and pushed it to try:

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/426d6b4448a7
Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref. r=kmag

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/426d6b4448a7

Comment 17

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

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

Approval Request Comment

[Feature/regressing bug #]:
Regression
"Bug 1311508 - Embedded WebExtension not available after restart"
on Feature
"Bug 1269342 - XPIProvider should provide optional embedded webextension instance to classic extensions"

[User impact if declined]:
Hybrid addon will not work after a browser restart (because the embedded webextension will be missing from the bootstrap method parameters).

[Describe test coverage new/current, TreeHerder]:
There are tests which covers hybrid addons and the embedded
webextension behavior, but none of the existent tests was restarting the addon manager.

This patch (besides the actual fix) contains a new test case which
ensures that the hybrid addons behavior is preserved across
browser restarts.

Links to treeheader for nightly and aurora (none of the oranges seems to be related to this change):

- aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e79465c5481bdb0ac3d4388d37454bf2de5f84e
- trunk (before landing): https://treeherder.mozilla.org/#/jobs?repo=try&revision=dca4f4df14e9408608f621c9c0de887b98274dc4

[Risks and why]:The risks of this change seems to be low: the fix in the XPIProvider, which ensures that the "hasEmbeddedExtension" addon property has been stored and preserved across browser restarts, is pretty small and the existent XPIProvider tests with the addition of the new test case provides a good coverage.
The feature is currently used on Aurora and Nightly only by addon developers that are explicitly enabling the related install.rdf flag to prepare their transitional hybrid addons and test them on Aurora and Nightly.

[String/UUID change made/needed]:
None

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8802843 [details]
Bug 1311508 - Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.

Fix an issue related to hybrid addon. Take it in 51 aurora.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6055eeea19ee

Comment 20

[CosminB]

I can reproduce this issue on Firefox 52.0a1 (20161010030204), here is a video:
http://screencast.com/t/iTmZW71O , after restart aData.webExtension is not available.
 
This issue is verified as fixed on Firefox 52.0a1 (20161030030204) http://screencast.com/t/A7SPm4KZ1k  and Firefox  51.0a2 (20161031004002) http://screencast.com/t/ff6CXoE7w under Windows 7 64-bit, after restart aData.webExtension is available.