Closed Bug 1311508 Opened 3 years ago Closed 3 years ago

Embedded WebExtension not available after restart

Categories

(WebExtensions :: Compatibility, defect, P1, major)

52 Branch
defect

Tracking

(firefox51 verified, firefox52 verified)

VERIFIED FIXED
mozilla52
Tracking Status
firefox51 --- verified
firefox52 --- verified

People

(Reporter: noitidart, Assigned: rpl)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

Attached file 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.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Attached file timer loop.xpi
Checks every 2 seconds to see if aData.webExtension is available. It never becomes available.
Apparently this is happening because we're not storing the hasEmbeddedWebExtension flag in the bootstrappedAddons pref.
Assignee: nobody → lgreco
Severity: normal → major
Component: WebExtensions: Untriaged → WebExtensions: Compatibility
Summary: Embeded webext, after installing embeded webext, then restarting, on startup aData.webExtension never becomes available → Embedded WebExtension not available after restart
Priority: -- → P1
Status: NEW → ASSIGNED
Blocks: 1252227
Whiteboard: triaged
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.
Attachment #8802843 - Flags: review?(kmaglione+bmo)
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 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.
Attachment #8802843 - Flags: review?(kmaglione+bmo) → review+
Luca, can you please request uplift to 51 for this as soon as this lands? Thanks.
Flags: needinfo?(lgreco)
Flags: needinfo?(lgreco)
Keywords: checkin-needed
(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
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/426d6b4448a7
Store the hasEmbeddedWebExtension flag in the bootstrappedAddons pref. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/426d6b4448a7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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
Attachment #8802843 - Flags: approval-mozilla-aurora?
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.
Attachment #8802843 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.