Embedded WebExtension not available after restart

VERIFIED FIXED in Firefox 51

Status

()

Toolkit
WebExtensions: Compatibility
P1
major
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: noitidart, Assigned: rpl)

Tracking

52 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox51 verified, firefox52 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8802663 [details]
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.
(Reporter)

Updated

a year ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
(Reporter)

Comment 1

a year ago
Created attachment 8802667 [details]
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
status-firefox51: --- → affected
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
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
Pushed to try:

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

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 8

a year ago
mozreview-review-reply
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
(Assignee)

Comment 9

a year ago
Last patch version pushed to try:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0ed5da73f8d76bab2ffac33db48df4400505d5
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Last push to try:

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

Updated

a year ago
Flags: needinfo?(lgreco)
Keywords: checkin-needed
(Assignee)

Comment 14

a year ago
(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

a year ago
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

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/426d6b4448a7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 17

a year ago
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+

Comment 19

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6055eeea19ee
status-firefox51: affected → fixed

Comment 20

a year ago
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
status-firefox51: fixed → verified
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.