Closed Bug 1269342 Opened 8 years ago Closed 8 years ago

XPIProvider should provide optional embedded webextension instance to classic extensions

Categories

(WebExtensions :: Untriaged, defect, P1)

Product:
WebExtensions
Component:
Untriaged
Type:
defect

Tracking

(e10s+, firefox50 wontfix, firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
e10s + ---
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Whiteboard: triaged)

Attachments

(2 files, 2 obsolete files)

Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.
58 bytes, text/x-review-board-request
aswan
: review+
kmag
: review+
Details
[rebased to aurora] 1-Bug_1269342___Integrate_LegacyExtensionsUtils_helpers_into_XPIProvider__r_aswan_kmag.patch
21.91 KB, patch
ritu
: approval-mozilla-beta-
Details | Diff | Splinter Review 
The embedded webextensions feature implemented in Bug 1252215 (minimal webextension messaging API provided to non-webextension contexts) and Bug 125227 (embed a webextension instance in a classic extension) should be exposed to the classic extension through the XPIProvider, to provide to the addon developers a simpler and safer way to integrate the feature in their extensions.

This issue is related to the specs of the XPIProvider integrations:

- The addon developer should be able to enable the embedded webextension feature through a new install.rdf property (e.g. if a "enableEmbeddedWebExtension" property is defined and set to true in the install.rdf)

- The webextension embedded in a classic extension should be provided of the same addon id of its "classic extension container"

- The embedded webextension resources should be all contained in a "webextension/" sub-directory of the classic extension resources (to make it simple to understand which code is running in the embedded webextension and which is still running with full privileges as part of the classic webextension code) 

- The embedded webextension lifecycle (startup and shutdown) should be automatically managed by the XPIProvider

- The embedded webextension startup should happen after the "bootstrap startup" method, so that the classic extension has the opportunity to subscribe listeners to the onConnect event and receive connection from the webextension background page when it is started

- The embedded webextension shutdown should happen after the "bootstrap shutdown" method, so that the classic extension can receive disconnect events on the Port API event listeners

- the "minimal webextension messaging API" should be provided to classic extension through the bootstrap method parameters (e.g. as `startup({..., embeddedWebExtensionAPI}) { ... }`)

- the above "minimal webextension messaging API" instance should provide the capability to receive connection only from the embedded webextension
Blocks: 1252227
Whiteboard: triaged
This patch introduces a new exported helper (EmbeddedWebExtensionsUtils)
and its related testcase.

EmbeddedWebExtensionsUtils is going to be integrated in the XPIProvider
to provide the Embedded WebExtension to the Classic Extensions which
have enabled it in their install.rdf

Review commit: https://reviewboard.mozilla.org/r/50505/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50505/
Depends on: 1252215
Attachment #8748738 - Flags: feedback?(aswan)
Before reviewing the patch, a few questions on the requirements you outlined:

(In reply to Luca Greco [:rpl] from comment #0)
> - The webextension embedded in a classic extension should be provided of the
> same addon id of its "classic extension container"

This sounds like a potential problem, if we actually have separate Addon instances for the two pieces, then things like getAddonByID() are going to break (e.g., you try to find the classic extension but get back the webextension or vice versa).  If we have a single Addon instance then we need to make sure things like bootstrap method calls get delivered to both extensions.

> - The embedded webextension shutdown should happen after the "bootstrap
> shutdown" method, so that the classic extension can receive disconnect
> events on the Port API event listeners

This sounds backward?  If bootstrap shutdown happens first then the classic extension won't get to see those disconnect events.
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50505/diff/1-2/
Attachment #8748738 - Flags: feedback?(aswan)
(In reply to Andrew Swan [:aswan] from comment #2)
> Before reviewing the patch, a few questions on the requirements you outlined:
> 
> (In reply to Luca Greco [:rpl] from comment #0)
> > - The webextension embedded in a classic extension should be provided of the
> > same addon id of its "classic extension container"
> 
> This sounds like a potential problem, if we actually have separate Addon
> instances for the two pieces, then things like getAddonByID() are going to
> break (e.g., you try to find the classic extension but get back the
> webextension or vice versa).  If we have a single Addon instance then we
> need to make sure things like bootstrap method calls get delivered to both
> extensions.

From an XPIProvider point of view, the only addon installed is the "classic extension container",
and its bootstrap file the only one executed.

The XPIProvider "embedded webextension" integrations are going to be responsible of the
"embedded webextension startup and shutdown", when it is going to make the
corresponding bootstrap method calls on the "classic extension container" addon.

e.g. as in the current prototyped patch:

- embeded webextension startup:
  https://github.com/rpl/firefox-patches/blob/master/webext/bug-1252215-classic-ext-messaging/3-Bug_1269342___Integrate_EmbeddedWebExtensionsUtils_helper_into_XPIProvider__f_aswan.patch#L97 

- embedded webextension shutdown: 
  https://github.com/rpl/firefox-patches/blob/master/webext/bug-1252215-classic-ext-messaging/3-Bug_1269342___Integrate_EmbeddedWebExtensionsUtils_helper_into_XPIProvider__f_aswan.patch#L85

> 
> > - The embedded webextension shutdown should happen after the "bootstrap
> > shutdown" method, so that the classic extension can receive disconnect
> > events on the Port API event listeners
> 
> This sounds backward?  If bootstrap shutdown happens first then the classic
> extension won't get to see those disconnect events.

Yes, definitely, my apologies.

The correct spec is:
"embedded webextension shutdown should be called BEFORE the classic extension "shutdown bootstrap method" is being called.
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50505/diff/2-3/
Attachment #8748738 - Attachment description: MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper. f?aswan → MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper. r?aswan
Attachment #8748738 - Flags: review?(aswan)
(In reply to Luca Greco [:rpl] from comment #5)
> Comment on attachment 8748738 [details]
> MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils
> helper. r?aswan
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/50505/diff/2-3/

Added to the test case: "remove generated xpi file and flush its jar cache" on clean up.
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

https://reviewboard.mozilla.org/r/50505/#review48215

I think it is important to nail down how we want failures, especially partial failures (e.g., the classic extension itself is okay but the webextension has a startup error) to get handled.
And, to be clear, this is only part of bug 1269342 right?  the additional patches that are currently on github will also be part of this bug?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:194
(Diff revision 3)
>      return this[ns].api;
>    }
>  };
> +
> +/**
> + *  This class instances are used internally by the exported EmbeddedWebExtensionsUtils

"This class instances" -> "Instances of this class"

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:244
(Diff revision 3)
> +  /**
> +   *  Start the embedded webextension (and report any loading error in the Browser console).
> +   */
> +  startup() {
> +    this.webextension.startup()
> +      .catch((err) => {

Shouldn't this propagate and cause startup of the containing extension to fail?
Attachment #8748738 - Flags: review?(aswan)
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50505/diff/3-4/
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

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

> I think it is important to nail down how we want failures, especially partial failures (e.g., the classic extension itself is okay but the webextension has a startup error) to get handled.

Good call!

In the last pushed version of this patches (https://reviewboard.mozilla.org/r/50503/diff/3-5/) I've added a proposal of how to expose the results of the embedded webextension startup through the exposed API and test case to start exploring the failures scenarios (currently related to the potential manifest loading errors)

> And, to be clear, this is only part of bug 1269342 right?  the additional patches that are currently on github will also be part of this bug?

Yes, there is another patch which integrates the newly created helpers into the XPIProvider and the related additional tests (on the contrary the SDK part of the "embedded webextension" feature will be tracked by Bug 1269347)
Comment on attachment 8751343 [details]
MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals.

https://reviewboard.mozilla.org/r/51977/#review48919

looks right to me but you probably want a testing peer to okay it
Attachment #8751343 - Flags: review?(aswan) → review+
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

https://reviewboard.mozilla.org/r/50505/#review48935

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:228
(Diff revisions 3 - 5)
>        senderURL: resourceURI.resolve("/"),
>      });
>  
> +    // Expose an extended API to help handling startup errors
> +    // on the embedded webextension.
> +    this.classicExtensionContext.api.waitForStartup = new Promise((resolve, reject) => {

I think this would be neater if you created the promise here and passed it into the ClassicExtensionContext constructor.
Then, the code to expose it in the API and to catch errors and expose them in the startupError property could be done in ClassicExtensionContext rather than here.

Whoops, reading further that won't work if the embedded webextension is shut down and then re-started.  If that's something that can happen, I think this would be neater by creating something like ClassicExtensionContenxt.setStartupPromise() that would be called from here and from shutdown.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:144
(Diff revisions 3 - 5)
> +  } catch (e) {
> +    startupError = e;
> +  }
> +
> +  if (!startupError) {
> +    yield new Promise(() => {});

what is this for?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:185
(Diff revisions 3 - 5)
> +    {
> +      id: "empty-manifest@test.embedded.web.extension",
> +      files: {
> +        "webextension/manifest.json": ``,
> +      },
> +      expected: {isExceptionError, errorMessageIncludes: "(NS_BASE_STREAM_CLOSED)"},

yikes, we should definitely improve this (in a separate bug as we discussed earlier today)
Attachment #8748738 - Flags: review?(aswan)
https://reviewboard.mozilla.org/r/51977/#review48919

Sure, My apologies: I forgot to remove the r? after the split of the mochitest.eslintrc change from the other patch.
https://reviewboard.mozilla.org/r/50505/#review48935

> I think this would be neater if you created the promise here and passed it into the ClassicExtensionContext constructor.
> Then, the code to expose it in the API and to catch errors and expose them in the startupError property could be done in ClassicExtensionContext rather than here.
> 
> Whoops, reading further that won't work if the embedded webextension is shut down and then re-started.  If that's something that can happen, I think this would be neater by creating something like ClassicExtensionContenxt.setStartupPromise() that would be called from here and from shutdown.

That is definitely an option that I'm evaluating (and one of the prototypes was actually done mostly as describe above), I'm a bit in doubt  if it is "correct" to give that "responsability" to the ClassicExtensionContext, which currently doesn't manage the webextension lifecycle on its own.

Nevertheless I agree with your point, I'm going to look again into it to find a nicer approach.

> what is this for?

Ouch, it is a debugging trick that I forgot to remove (I was exploring the scenarios that doesn't generate a startup error and that empty promise stop the test execution and prevents the test from unloading the addon).

Thanks for spotting it, it wasn't supposed to be committed into the patch (even if the patch is currently in a feedback loop and it is not supposed to land as it is)

> yikes, we should definitely improve this (in a separate bug as we discussed earlier today)

Definitely, I must say that by testing the startup errors at this abstraction level helped me to get a better picture of how much the error reporting can be (and have to be) improved.
Comment on attachment 8751343 [details]
MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51977/diff/1-2/
Attachment #8751343 - Attachment description: MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals. r?aswan → MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals.
Attachment #8748738 - Attachment description: MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper. r?aswan → MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.
Attachment #8748738 - Flags: review?(aswan)
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

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

> That is definitely an option that I'm evaluating (and one of the prototypes was actually done mostly as describe above), I'm a bit in doubt  if it is "correct" to give that "responsability" to the ClassicExtensionContext, which currently doesn't manage the webextension lifecycle on its own.
> 
> Nevertheless I agree with your point, I'm going to look again into it to find a nicer approach.

In the last updated version of this patch, the new `ClassicExtensionContext.setupStartupPromise` handle the setup of the promise (and set/unset of the `startupError` property as well).
Attachment #8748738 - Flags: review?(aswan)
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

https://reviewboard.mozilla.org/r/50505/#review49794

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:137
(Diff revisions 5 - 6)
> +  setupStartupPromise() {
> +    // XXX: Should we check if there was another promise set and resolve/reject it?
> +    let webextStartupPromise = {};
> +    this[ns].api.startupError = null;
> +    this[ns].api.waitForStartup = new Promise((resolve, reject) => {
> +      webextStartupPromise.resolve = resolve;
> +      webextStartupPromise.reject = reject;
> +    }).then(() => {
> +      // Reset the startupError when the startup promise has been resolved.
> +      this[ns].api.startupError = null;
> +    }, (err) => {
> +      // Set the startupError when the startup promise has been rejected.
> +      this[ns].api.startupError = err;
> +      // let the promise to reject.
> +      throw err;
> +    });
> +
> +    return webextStartupPromise;
> +  }

I'm curious why you did it this way rather than just taking the promise as a parameter?
Also about the XXX comment, I would think that calling this when there already is a promise set is a programming error so throw an error back to the caller?
Comment on attachment 8751343 [details]
MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51977/diff/2-3/
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

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

> I'm curious why you did it this way rather than just taking the promise as a parameter?
> Also about the XXX comment, I would think that calling this when there already is a promise set is a programming error so throw an error back to the caller?

I wrote it in both ways (at least in the "editor embedded in the mind" ;-)) and (at least to me) this one looked cleaner from a "caller" point of view (basically less code on the caller side).

I like the idea of raising an exception when there is a pending startup promise, and I've added it in the last update of this patch (and added a test for it in the existent xpcshell test).
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

I've added to the mozreview queue this new patch which contains:
- updated version of the changes needed to integrate the EmbeddedWebExtensionUtils helper into the XPI Provider
- add a first group of xpcshell test related to the changes introduced in the XPI Provider

I'm adding f? to get a first feedback about it (especially about the xpcshell tests).
Attachment #8753348 - Flags: feedback?(aswan)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

https://reviewboard.mozilla.org/r/50505/#review50100

Do you want to test the startup promise?  (ie, test that the classic extension can chain a handler onto it and that handler gets called, chain an error hanlder onto it, check that the handler is called if the webextension has a manifest error or something, etc.?)

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:169
(Diff revisions 5 - 7)
> +      webextStartupPromise.resolve = resolve;
> +      webextStartupPromise.reject = reject;
> +    }).then(() => {
> +      this[ns].pendingStartupPromise = false;
> +      // Reset the startupError when the startup promise has been resolved.
> +      this[ns].api.startupError = null;

I don't think this is wrong but you set it a few lines above and nothing else should be setting it between there and here?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:323
(Diff revision 7)
> +
> +  /**
> +   *  Shuts down the embedded webextension.
> +   */
> +  shutdown() {
> +    // Run shutdown only if the embedded webextension has been started and

I think you have a race here if shutdown() gets called right after startup() (ie, before the startup promise has resolved).  Instead of checking this.started, you could just chain it onto the startup promise?
That's getting a little awkward since the startup promise is stored in the api object, if you created the promise here instead it would be nicer :-)
Attachment #8748738 - Flags: review?(aswan) → review+
https://reviewboard.mozilla.org/r/53230/#review50104

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4752
(Diff revision 1)
> +      if (aAddon.enableEmbeddedWebExtension) {
> +        if (aMethod == "startup") {
> +          // The startup bootstrap method is the only method that needs to receive
> +          // the embededWebExtensionAPI in its params.
> +          params.embeddedWebExtensionAPI = EmbeddedWebExtensionsUtils.getAPIFor(params);
> +        } else if (aMethod == "shutdown") {
> +          // Shutdown the embedded webextension here, so that the onConnect event listener
> +          // on the classic extension side will be able to receive a disconect on the
> +          // WebExtension port API.
> +          EmbeddedWebExtensionsUtils.shutdownFor(params);
> +        }
> +      }

can you move this block down and merge it with the code that calls startupFor() so it is all in one place?  Also, I don't think the startupFor() code needs to be in a try block, any startup errors should result in rejection of the startup promise, not an immediate throw.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:17
(Diff revision 1)
> +profileDir.append("extensions");
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "49");
> +startupManager();
> +
> +// NOTE: this needs to be imported after the startupManager helper has been called.

why?

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:20
(Diff revision 1)
> +startupManager();
> +
> +// NOTE: this needs to be imported after the startupManager helper has been called.
> +const { Management } = Components.utils.import("resource://gre/modules/Extension.jsm", {});
> +
> +function promiseAddonStartup() {

This exists in BootstrapMonitor, can you just use that version?

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:77
(Diff revision 1)
> + */
> +add_task(function* run_embedded_webext_bootstrap() {
> +  const ID = "embedded-webextension-addon2@tests.mozilla.org";
> +
> +  yield Promise.all([
> +    promiseInstallAllFiles([do_get_addon("test_embedded_webext2")], true),

I think you forgot to add the actual addon contents to this patch?

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:84
(Diff revision 1)
> +    // because we are going to remove the xpi file during a running "locales reading" promise
> +    // executed by the webextension startup method.
> +    promiseAddonStartup(),
> +  ]);
> +
> +  let a2 = yield promiseAddonByID(ID);

nitpick, you've got separate scopes for each test case so you could just call this "addon"

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:100
(Diff revision 1)
> +
> +  // Check that the addon has been installed and started.
> +  BootstrapMonitor.checkAddonInstalled(ID, "1.0");
> +
> +  let installInfo = BootstrapMonitor.installed.get(ID);
> +  do_check_false(Object.keys(installInfo.data).includes("embeddedWebExtensionAPI"),

Why did you write it this way instead of just `is(installInfo.data.embeddedWebExtensionAPI, null, "...")` (and likewise for the other similar tests below)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Looking good!
Attachment #8753348 - Flags: feedback?(aswan) → feedback+
https://reviewboard.mozilla.org/r/50505/#review50100

I've currently some basic testing on the startup promise in the "test_classic_extension_utils.js" xpcshell test but 
I'm definitely going to add more tests on both the startup promise and the full life-cycle on a embedded webextensions (about this one my main doubt is into which one of the
test suites it is going to end up, between the XPIProvider mochitests or our WebExtension mochitests).

> I think you have a race here if shutdown() gets called right after startup() (ie, before the startup promise has resolved).  Instead of checking this.started, you could just chain it onto the startup promise?
> That's getting a little awkward since the startup promise is stored in the api object, if you created the promise here instead it would be nicer :-)

That's true, and paired with the addon reload feature, it is probably not so unlikely to happen.

I'm going to look into it.
https://reviewboard.mozilla.org/r/53230/#review50104

> why?

it happened during the writing of this xpcshell test, the tests were failing immediately and there were complaining about a non-initialized AddonManager,

I was supposing there was some usage of the AddonManager in the Extension.jsm and so
I put that note to look into it again, but unfortunately it seems that I'm currently unable to reproduce it (I was using an artifact debug build which are currently not downloadable on linux64, I'm going to try again by building a debug version manually)

> This exists in BootstrapMonitor, can you just use that version?

This one is different from what the BoostrapMonitor is currently providing, mainly because it is "WebExtension" specific (and it is currently the same helper used in the test_webextension.js test file https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_webextension.js#17)

I need it to be sure that the embedded webextension is fully started (or there will be an exception raised by the locale loading started inside the webextension startup because we are going to remove the xpi file before it has completed its job)

I initially tried to use the waitForStartup Promise from the `startupInfo.data.embeddedWebExtensionAPI` object but it is not a promise object (probably because it has been converted to be sent from the BootstrapMonitor.jsm helpers loaded in the test addon bootstrap file to the BootstrapMonitor singleton)

I'm going to rename it into `promiseWebExtensionStartup()` to avoid to confuse it with the helpers already provided by the BootstrapMonitor.

> I think you forgot to add the actual addon contents to this patch?

ouch, definitely, my apologies.
https://reviewboard.mozilla.org/r/53230/#review50104

> can you move this block down and merge it with the code that calls startupFor() so it is all in one place?  Also, I don't think the startupFor() code needs to be in a try block, any startup errors should result in rejection of the startup promise, not an immediate throw.

I cannot move this block down because of the following reasons:

- the `embeddedWebExtensionAPI` needs to be set in the bootstrap method params before the bootstrap method is going to be called

- we want to execute the shutdown on the embedded webextension before calling the shutdown bootstrap method (so that the classic extension addon will be able to receive the disconnect event on the ports listeners)

- we want to execute the startup on the embedded webextension after calling the startup bootstrap method (so that the classic extension addon has the opportunity to subscribe its listeners to the onConnect/onMessage API objects).

I agree on the unnecessary try block which surround the startupFor.
https://reviewboard.mozilla.org/r/53230/#review50104

> it happened during the writing of this xpcshell test, the tests were failing immediately and there were complaining about a non-initialized AddonManager,
> 
> I was supposing there was some usage of the AddonManager in the Extension.jsm and so
> I put that note to look into it again, but unfortunately it seems that I'm currently unable to reproduce it (I was using an artifact debug build which are currently not downloadable on linux64, I'm going to try again by building a debug version manually)

I've been able to reproduce and track it down, follows a description of the real reason of the failure:

Extension.jsm has to be imported after the `createAppInfo` helper has been called or the import will fail because Extension.jsm internally imports AddonManager.jsm and AddonManager.jsm will raise a ReferenceError exception because it tries to use an undefined `Services.appinfo` object.
Comment on attachment 8751343 [details]
MozReview Request: Bug 1269342 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51977/diff/3-4/
Attachment #8753348 - Attachment description: MozReview Request: Bug 1269342 - Integrate EmbeddedWebExtensionsUtils helper into XPIProvider. f?aswan → MozReview Request: Bug 1269342 - Integrate EmbeddedWebExtensionsUtils helper into XPIProvider.
Attachment #8753348 - Flags: feedback+
Comment on attachment 8748738 [details]
MozReview Request: Bug 1269342 - [webext] Add EmbeddedWebExtensionsUtils helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50505/diff/7-8/
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/1-2/
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/2-3/
Attachment #8751343 - Attachment is obsolete: true
Attachment #8748738 - Attachment is obsolete: true
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/3-4/
Attachment #8753348 - Flags: review?(aswan)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/4-5/
Blocks: 1269347
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review54790

Sorry this one sat for so long.  Given the nature of it, I think it would be good if Kris takes a look too.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:61
(Diff revision 5)
>  XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
>                                    "resource://gre/modules/AppConstants.jsm");
> +XPCOMUtils.defineLazyGetter(this, "EmbeddedWebExtensionsUtils", () => {
> +  const {
> +    EmbeddedWebExtensionsUtils,
> +  } = Cu.import("resource://gre/modules/ClassicExtensionsUtils.jsm");

nitpick: as I understand it, since you're not passing a second argument to import(), it will put all the exported symbols from ClassicExtensionUtils.jsm into the global scope so you could simplify this to just call defineLazyModuleGetter

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4761
(Diff revision 5)
>            params[key] = aExtraParams[key];
>          }
>        }
>  
> +      if (aAddon.enableEmbeddedWebExtension) {
> +        if (aMethod == "startup") {

As we overload callBootstrapMethod() to do other things for various actions, I think it would make sense to refactor here so we have methods like `startupExtension()`, `shutdownExtension()` etc.  Those methods would invoke `callBootstrapMethod()` as well as doing other related things (eg, load/unload chrome manifest, start/stop embedded webextension) and existing calls to `callBootstrapMethod()` would change to calls to `startupExtension()` etc.
I don't think you need to do that in this patch, just making the observation...

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:140
(Diff revision 5)
> +  for (let prop of EXPECTED_API_PROPS) {
> +    do_check_true(actualProps.includes(prop),
> +                  `Got the expected ${prop} property in the embeddedWebExtensionAPI object`);
> +  }
> +
> +  // Uninstall the addon and test the params of the shutdown and uninstall bootstrap method.

also yield promiseWebExtensionShutdown() here?
Attachment #8753348 - Flags: review?(aswan) → review+
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/5-6/
Attachment #8753348 - Attachment description: MozReview Request: Bug 1269342 - Integrate EmbeddedWebExtensionsUtils helper into XPIProvider. → Bug 1269342 - Integrate EmbeddedWebExtensionsUtils helper into XPIProvider.
https://reviewboard.mozilla.org/r/53230/#review54790

> nitpick: as I understand it, since you're not passing a second argument to import(), it will put all the exported symbols from ClassicExtensionUtils.jsm into the global scope so you could simplify this to just call defineLazyModuleGetter

I double-checked this and the ClassicExtensionsUtils (which is the only symbol explicitly exported from the jsm) is only available inside the closure (and it is not available globally).

I tried in this lazy import in different ways before reaching this current version, and I was unable to achieve it in a reasonable way using the XPCOMUtils.defineLazyModuleGetter, and that is why I've changed it to use the XPCOMUtils.defineLazyGetter.

> As we overload callBootstrapMethod() to do other things for various actions, I think it would make sense to refactor here so we have methods like `startupExtension()`, `shutdownExtension()` etc.  Those methods would invoke `callBootstrapMethod()` as well as doing other related things (eg, load/unload chrome manifest, start/stop embedded webextension) and existing calls to `callBootstrapMethod()` would change to calls to `startupExtension()` etc.
> I don't think you need to do that in this patch, just making the observation...

I fully agree with this observation.

> also yield promiseWebExtensionShutdown() here?

added in the last version of this test file.
https://reviewboard.mozilla.org/r/53230/#review55718

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:28
(Diff revision 6)
> +function promiseWebExtensionStartup() {
> +  return new Promise(resolve => {
> +    let listener = (event, extension) => {
> +      Management.off("startup", listener);
> +      // Fake the content process "Extension:StartupComplete" so that the
> +      // WebExtensions startup promise could be resolved in the XPCShell tests.
> +      Services.cpmm.sendAsyncMessage("Extension:StartupComplete");
> +      resolve(extension);
> +    };
> +
> +    Management.on("startup", listener);
> +  });
> +}

In the last version I've tweaked the `promiseWebExtensionStartup()` test helper so that the WebExtension startup method doesn't get stuck because it will never receive the "Extension:StartupComplete" message from the content process in an xpcshell test.

NOTE: In a Firefox instance the promise returned by the WebExtension startup method will resolved when the promise returned by the broadcast method is resolved, references:

- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1311
- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#808
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Added Kris as an additional reviewer for this patch.
Attachment #8753348 - Flags: review?(kmaglione+bmo)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/6-7/
Attachment #8753348 - Attachment description: Bug 1269342 - Integrate EmbeddedWebExtensionsUtils helper into XPIProvider. → Bug 1269342 - Integrate EmbeddedExtensionsUtils helper into XPIProvider.
Attachment #8753348 - Flags: review?(kmaglione+bmo)
Attachment #8753348 - Flags: review?(kmaglione+bmo)
(In reply to Luca Greco [:rpl] from comment #43)
> Comment on attachment 8753348 [details]
> Bug 1269342 - Integrate EmbeddedExtensionsUtils helper into XPIProvider.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/53230/diff/6-7/

Applied minor changes based on the last review comments on the patches attached to Bug 1252215, and I've renamed the RDF property to "embeddedWebExtension" as suggested during the presentation and the related brief discussion during MozLondon.
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/7-8/
Attachment #8753348 - Attachment description: Bug 1269342 - Integrate EmbeddedExtensionsUtils helper into XPIProvider. → Bug 1269342 - Integrate ClassicExtensionsUtils helpers into XPIProvider.
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/8-9/
Attachment #8753348 - Attachment description: Bug 1269342 - Integrate ClassicExtensionsUtils helpers into XPIProvider. → Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.
The updated patch contains changes which are related to the last changes applied to Bug 1252215 based on the review comments:

- the new JSM module has been renamed to LegacyExtensionUtils.jsm (and the exported classes renamed accordingly)

- the EmbeddedExtension class is exported by the LegacyExtensionsUtils.jsm and it is used in the XPIProvider to create the embedded extension instances

- the XPIProvider keeps track of the existent embedded extension instances, and manage their lifecycle (using the startup/shutdown method of the embedded extension instance and adding/removing the embedded extension instance from a `Map` of the embedded extensions associated to their add-on id)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53230/diff/9-10/
tracking-e10s: --- → ?
tracking-e10s: ? → +
Priority: -- → P1
(In reply to Luca Greco [:rpl] from comment #50)
> Comment on attachment 8753348 [details]
> Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/53230/diff/10-11/

Rebased and updated patch (based on the last version of the patches attached to Bug 1252215)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review73686

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:182
(Diff revision 11)
>  
>  const TOOLKIT_ID                      = "toolkit@mozilla.org";
>  
>  const XPI_SIGNATURE_CHECK_PERIOD      = 24 * 60 * 60;
>  
>  XPCOMUtils.defineConstant(this, "DB_SCHEMA", 17);

This needs to be bumped.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4903
(Diff revision 11)
> +        embeddedExtension.startup()
> +          .then(api => {
> +            params.embeddedWebExtensionAPI = api;

Can we just pass the promise or the startup method? Some extensions rely on being able to run code as early as possible from their startup methods.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4905
(Diff revision 11)
> +
> +        // This is an hybrid addon startup, try to start the embedded extension and if
> +        // there are no loading errors, then call the bootstrap method asynchronously.
> +        embeddedExtension.startup()
> +          .then(api => {
> +            params.embeddedWebExtensionAPI = api;

This name is a bit verbose.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4907
(Diff revision 11)
> +            callBootstrap(() => {
> +              // Shutdown the embedded extension if the container has failed to start.
> +              embeddedExtension.shutdown();
> +            });

I'm not sure this is a good idea.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:7279
(Diff revision 11)
>  
>    get seen() {
>      return addonFor(this).seen;
>    },
>  
> +  get embeddedWebExtension() {

`hasEmbeddedWebExtension`?

::: toolkit/mozapps/extensions/test/addons/test_embedded_webext2/bootstrap.js:1
(Diff revision 11)
> +Components.utils.import("resource://xpcshell-data/BootstrapMonitor.jsm").monitor(this);

Please generate test extensions on the fly for new tests.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:5
(Diff revision 11)
> +// Enable loading extensions from the user scopes
> +Services.prefs.setIntPref("extensions.enabledScopes",
> +                          AddonManager.SCOPE_PROFILE + AddonManager.SCOPE_USER);

This isn't necessary.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:15
(Diff revision 11)
> +
> +const profileDir = gProfD.clone();
> +profileDir.append("extensions");
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "49");
> +startupManager();

This isn't necessary.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:24
(Diff revision 11)
> +  LegacyExtensionsUtils,
> +  EmbeddedExtensionManager,

Please keep sorted.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:35
(Diff revision 11)
> +  return new Promise(resolve => {
> +    let listener = (event, extension) => {
> +      Management.off("startup", listener);
> +      // Fake the content process "Extension:StartupComplete" so that the
> +      // WebExtensions startup promise could be resolved in the XPCShell tests.
> +      Services.cpmm.sendAsyncMessage("Extension:StartupComplete");

This shouldn't be necessary.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:61
(Diff revision 11)
> +  writeInstallRDFForExtension({
> +    id: ID,
> +    version: "1.0",
> +    name: "Embedded WebExtension Test 1",
> +    embeddedWebExtension: true,
> +    targetApplications: [{
> +      id: "xpcshell@tests.mozilla.org",
> +      minVersion: "1",
> +      maxVersion: "49"
> +    }]
> +  }, profileDir);
> +
> +  yield promiseRestartManager();

This doesn't seem like a good idea, unless we're doing it to check error handling when we have `embeddedWebExtension="true"` but no embedded webextension. We don't seem to be checking the error handling, though.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:77
(Diff revision 11)
> +
> +  yield promiseRestartManager();
> +
> +  let addon = yield promiseAddonByID(ID);
> +
> +  do_check_neq(addon, null, "Got an addon object as expected");

Nit: Please use `equal`, `notEqual`, `ok`, ... in new tests.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:88
(Diff revision 11)
> +  do_check_false(addon.userDisabled);
> +  do_check_true(addon.isActive);
> +
> +  addon.uninstall();
> +
> +  yield promiseRestartManager();

This shouldn't be necessary. Nor the restarts in the other tasks.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:111
(Diff revision 11)
> +
> +  let addon = yield promiseAddonByID(ID);
> +
> +  do_check_neq(addon, null, "Got an addon object as expected");
> +  do_check_eq(addon.version, "1.0", "Got the expected version");
> +  do_check_true(addon.embeddedWebExtension,

We should be explicitly checking this against the exact value `true`.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:140
(Diff revision 11)
> +  for (let prop of EXPECTED_API_PROPS) {
> +    do_check_true(actualProps.includes(prop),
> +                  `Got the expected ${prop} property in the embeddedWebExtensionAPI object`);
> +  }

`Assert.deepEqual`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:151
(Diff revision 11)
> +  // test the params of the shutdown and uninstall bootstrap method.
> +  let waitForWebExtensionShutdown = promiseWebExtensionShutdown();
> +  let waitUninstall = promiseAddonEvent("onUninstalled");
> +  addon.uninstall();
> +  yield waitForWebExtensionShutdown;
> +  yield waitUninstall;

We should check that this has been cleared from the embedded extension map at this point.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:156
(Diff revision 11)
> +  yield waitUninstall;
> +
> +  BootstrapMonitor.checkAddonNotStarted(ID, "1.0");
> +
> +  let shutdownInfo = BootstrapMonitor.stopped.get(ID);
> +  do_check_eq(shutdownInfo.data.embeddedWebExtensionAPI, null,

This should be a `!(... in shutdownInfo.data)` check.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:174
(Diff revision 11)
> + */
> +add_task(function* reload_embedded_webext_bootstrap() {
> +  const ID = "embedded-webextension-addon2@tests.mozilla.org";
> +
> +  // No embedded webextension should be currently around.
> +  do_check_eq(EmbeddedExtensionManager.embeddedExtensionsByAddonId.size, 0);

Please include messages in all assertions.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:200
(Diff revision 11)
> +  for (let i = 0; i < NUM_FAST_RESTARTS; i++) {
> +    let waitNextShutdown = promiseWebExtensionShutdown();
> +    let waitNextStartup = promiseWebExtensionStartup();
> +
> +    addon.userDisabled = true;
> +    addon.userDisabled = false;
> +
> +    yield waitNextShutdown;
> +    yield waitNextStartup;
> +  }
> +
> +  for (let i = 0; i < NUM_FAST_RESTARTS; i++) {
> +    let waitNextShutdown = promiseWebExtensionShutdown();
> +    let waitNextStartup = promiseWebExtensionStartup();
> +
> +    addon.reload();
> +
> +    yield waitNextShutdown;
> +    yield waitNextStartup;
> +  }

I'm not sure these checks are useful. Given that we're explicitly waiting for startup and shutdown between restarts, it's not an especially realistic scenario. And there are definitely places where we don't handle this properly.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:222
(Diff revision 11)
> +    yield waitNextShutdown;
> +    yield waitNextStartup;
> +  }
> +
> +  // No leaked embedded extension after the previous reloads.
> +  do_check_eq(EmbeddedExtensionManager.embeddedExtensionsByAddonId.size, 1);

This map is keyed by ID, so this check doesn't actually tell us very much.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:232
(Diff revision 11)
> +  yield waitUninstalled;
> +
> +  // No leaked embedded extension after uninstalling.
> +  do_check_eq(EmbeddedExtensionManager.embeddedExtensionsByAddonId.size, 0);
> +
> +  yield promiseRestartManager();

This isn't necessary.
Attachment #8753348 - Flags: review?(kmaglione+bmo)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review73686

> This needs to be bumped.

Are you sure?

it looks like that when we added the ["multiprocessCompatible" RDF field](https://hg.mozilla.org/mozilla-central/rev/5ea30521f56bfa7f0b4e8361b40a702a90088f56) (which should be stored in the same way of the new one) we didn't upgraded the DB_SCHEMA, and so I was assuming that this fields are not stored in the sqlite storage.

But it is completely possible that I was missing something, can you give me some details about when the db schema should be bumped and when it should not?

> Can we just pass the promise or the startup method? Some extensions rely on being able to run code as early as possible from their startup methods.

I know, "turning callBootstrapMethod from synchronous to asynchronous" is something that worry me as well (and that was the main reason why I was keep asking why you were suggesting to reverse the startup order of the embedded and container addons).

For the same reason, this patch is calling the addon bootstrap method asynchronously only for addons that have choosen explicitly to embed a WebExtension and synchronously (like it was) for all the other addons.

Can you add more details about the above "Some extensions rely on being able to run code as early as possible" and how it will be affected by this change?

Anyway, in the updated version of this patch, I'm exploring how the hybrid addons APIs will look if we pass the startup method instead (e.g. it means that it needs to be explicitly called from the container addon to make the embedded extension to be started, on the contrary the shutdown is still automatically done when the addon is disabled/uninstalled, and the real api can be retrieved only after the startup promise has been resolved, which looks a bit weird for an Addon SDK api)

> This name is a bit verbose.

reduced to embeddedWebExtension 
(this name is going to be used directly only from addons based on a bootstrap file, Addon SDK addons will never see this name, they will just require the "sdk/webextension" module).

> I'm not sure this is a good idea.

I removed it, but I'm still interested to the reason why you think that it is wrong to shutdown the embedded extension if calling the bootstrap startup method has raised an exception.

> `hasEmbeddedWebExtension`?

Sounds good to me, but after renaming it here, I preferred to rename it in the other places to avoid any confusion
(basically the `hasEmbeddedWebExtension` is always a boolean, `embeddedWebExtension` is how the object that the bootstrap method receive is named)

> Please generate test extensions on the fly for new tests.

Sounds great. I actually prefer when the test file is self contained, I just wrote these tests before that helpers were introduced here.

> This isn't necessary.

Are you sure about it?
every one of the tests defined in this directory that I used as reference is using it
(and if I remove it I get this pretty clear exception raised:
```
 0:01.04 LOG: Thread-7 ERROR Unexpected exception NS_ERROR_NOT_INITIALIZED: AddonManager is not initialized
AddonManagerInternal.installTemporaryAddon@resource://gre/modules/AddonManager.jsm:2323:13
this.AddonManager.installTemporaryAddon@resource://gre/modules/AddonManager.jsm:3505:12
run_embedded_webext_bootstrap@/zfs-mordor/mozlab/mozilla-central-webext/objdir-frontend/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:56:5
_run_next_test@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:1566:9
do_execute_soon/<.run@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:713:9
_do_main@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:210:5
_execute_test@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:545:5
@-e:1:1

```
)

> This shouldn't be necessary.

Yeah, it was necessary when I wrote this tests, but you are right, apparently is not needed anymore.

> This doesn't seem like a good idea, unless we're doing it to check error handling when we have `embeddedWebExtension="true"` but no embedded webextension. We don't seem to be checking the error handling, though.

Actually, it is just that this generated addon doesn't have a bootstrap file, and so it doesn't raise any error.

I'm in doubt if it worth to raise an explicit exception if the RDF flag is set in a install.rdf which doesn't have the bootstrap RDF flag set to true, what do you think? (I can file a follow up bugzilla issue if we prefer to discuss this scenario separately)

Anyway, I agree that the test like it is doesn't add a lot, and so I'm cutting it out in the meantime.

> Nit: Please use `equal`, `notEqual`, `ok`, ... in new tests.

Sure, unfortunately I wrote this test when nothing of that was yet introduced in our xpcshell-test provided test helpers conventions.

> We should check that this has been cleared from the embedded extension map at this point.

Agree, added an additional assertion here.

> Please include messages in all assertions.

Yeah, agree, I followed the pattern used in most of the other tests in this directory because at the time it was first test written using the xpcshell test suite, but the lack of the assertion messages makes harder to follow the test or debug a failure.

> I'm not sure these checks are useful. Given that we're explicitly waiting for startup and shutdown between restarts, it's not an especially realistic scenario. And there are definitely places where we don't handle this properly.

This tests has been helpful so far, to catch mistakes and race conditions while I was changing Bug 1252215 based on the review comments or after a rebase.

But I agree that is is not very realistic like it was, I reworked it in the last version, let me know how it looks now.

> This map is keyed by ID, so this check doesn't actually tell us very much.

That's true. I reworked the test to check that the instances are different after an addon has been disabled and re-enabled or it has been reloaded.
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review73686

> Are you sure?
> 
> it looks like that when we added the ["multiprocessCompatible" RDF field](https://hg.mozilla.org/mozilla-central/rev/5ea30521f56bfa7f0b4e8361b40a702a90088f56) (which should be stored in the same way of the new one) we didn't upgraded the DB_SCHEMA, and so I was assuming that this fields are not stored in the sqlite storage.
> 
> But it is completely possible that I was missing something, can you give me some details about when the db schema should be bumped and when it should not?

Yes, I'm sure. The fact that it hasn't been bumped for other similar changes is a bug.

> I know, "turning callBootstrapMethod from synchronous to asynchronous" is something that worry me as well (and that was the main reason why I was keep asking why you were suggesting to reverse the startup order of the embedded and container addons).
> 
> For the same reason, this patch is calling the addon bootstrap method asynchronously only for addons that have choosen explicitly to embed a WebExtension and synchronously (like it was) for all the other addons.
> 
> Can you add more details about the above "Some extensions rely on being able to run code as early as possible" and how it will be affected by this change?
> 
> Anyway, in the updated version of this patch, I'm exploring how the hybrid addons APIs will look if we pass the startup method instead (e.g. it means that it needs to be explicitly called from the container addon to make the embedded extension to be started, on the contrary the shutdown is still automatically done when the addon is disabled/uninstalled, and the real api can be retrieved only after the startup promise has been resolved, which looks a bit weird for an Addon SDK api)

It varies by extension, but there are any number of things they may do that depend on being called early in the startup sequence.

I'm not sure what the SDK API has to do with any of this...

> I removed it, but I'm still interested to the reason why you think that it is wrong to shutdown the embedded extension if calling the bootstrap startup method has raised an exception.

We don't take any other action based on errors thrown by the startup method, and since the embedded extension is essentially a separate entity, I don't see why it should be shut down because of errors in the startup method, rather than at normal extension shutdown.

> Are you sure about it?
> every one of the tests defined in this directory that I used as reference is using it
> (and if I remove it I get this pretty clear exception raised:
> ```
>  0:01.04 LOG: Thread-7 ERROR Unexpected exception NS_ERROR_NOT_INITIALIZED: AddonManager is not initialized
> AddonManagerInternal.installTemporaryAddon@resource://gre/modules/AddonManager.jsm:2323:13
> this.AddonManager.installTemporaryAddon@resource://gre/modules/AddonManager.jsm:3505:12
> run_embedded_webext_bootstrap@/zfs-mordor/mozlab/mozilla-central-webext/objdir-frontend/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:56:5
> _run_next_test@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:1566:9
> do_execute_soon/<.run@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:713:9
> _do_main@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:210:5
> _execute_test@/zfs-mordor/mozlab/mozilla-central-webext/testing/xpcshell/head.js:545:5
> @-e:1:1
> 
> ```
> )

Sorry, you're right.

> Actually, it is just that this generated addon doesn't have a bootstrap file, and so it doesn't raise any error.
> 
> I'm in doubt if it worth to raise an explicit exception if the RDF flag is set in a install.rdf which doesn't have the bootstrap RDF flag set to true, what do you think? (I can file a follow up bugzilla issue if we prefer to discuss this scenario separately)
> 
> Anyway, I agree that the test like it is doesn't add a lot, and so I'm cutting it out in the meantime.

I don't think it's worth worrying about.
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review75246
Attachment #8753348 - Flags: review?(kmaglione+bmo)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review73686

> Yes, I'm sure. The fact that it hasn't been bumped for other similar changes is a bug.

Schema Bump (from 17 to 18) added to this patch.

Thanks a lot Kris (for catching it and for the confirmation and additional info). 

I wanted to know for sure, mostly because it isn't a secondary detail (and knowing about it surely help to reduce the chances of forgetting to bump the db schema in future patches, which it is probably a very annoying issue to fix once it reached the users and their profiles)

> It varies by extension, but there are any number of things they may do that depend on being called early in the startup sequence.
> 
> I'm not sure what the SDK API has to do with any of this...

Sorry, that part of my comment was definitely missing some additional information to be understandable:

what I mean is that, in an addon based on plain boostrap.js file, the developer is directly exposed to the internal APIs, and these APIs are already potentially different from each other (or, at least, no one expect them to follow a common API style).

On the contrary the SDK exposes the APIs with similar interfaces and approaches, and by exposing the startup method (or the startup promise) to an SDK addon, the way the developer is going to use the API will change from:

```
const { browser } = require("sdk/webextension");

browser.runtime.onConnect.addListener((...) => { ... });
```

to something like the following:

```
const webext = require("sdk/webextension");

webext.startup().then(({browser}) => {
  browser.runtime.onConnect.addListener((...) => { ... });
});
```

Anyway, I'm completely ok with the above (which, to be fair, is not that bad), especially if it is the best (and safer) strategy to follow.

> I don't think it's worth worrying about.

Thanks for the confirmation.

> This tests has been helpful so far, to catch mistakes and race conditions while I was changing Bug 1252215 based on the review comments or after a rebase.
> 
> But I agree that is is not very realistic like it was, I reworked it in the last version, let me know how it looks now.

The new version of this test is changed, I'm clearing this issue in the meantime (because it is referred to a different test of the current one).

Let me know if the new test is better and more realistic of this one.
(In reply to Luca Greco [:rpl] from comment #57)
> Comment on attachment 8753348 [details]
> Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/53230/diff/12-13/

This update contains:

- a rebase of this patch on a slightly more recent fx-team tip, where the patch from its dependency (Bug 1252215) has been already landed

- the remaining suggested changes from the previous review (in particular the bump of the XPIProvider DB_SCHEMA version number)
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review76424

r=me with corrections.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4899
(Diff revision 13)
> +      function callBootstrap() {
> -      logger.debug("Calling bootstrap method " + aMethod + " on " + aAddon.id + " version " +
> +        logger.debug("Calling bootstrap method " + aMethod + " on " + aAddon.id + " version " +
> -                   aAddon.version);
> +                     aAddon.version);
> -      try {
> +        try {
> -        method(params, aReason);
> +          method(params, aReason);
> -      }
> +        }
> -      catch (e) {
> +        catch (e) {
> -        logger.warn("Exception running bootstrap method " + aMethod + " on " + aAddon.id, e);
> +          logger.warn("Exception running bootstrap method " + aMethod + " on " + aAddon.id, e);
> -      }
> +        }
> -    }
> +      }

Please just move this code after the if-else statement rather than making it a function.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4913
(Diff revision 13)
> +        // This is an hybrid addon startup, try to start the embedded extension and if
> +        // there are no loading errors, then call the bootstrap method asynchronously.

This comment isn't accurate.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4915
(Diff revision 13)
> +      if (aAddon.hasEmbeddedWebExtension && aMethod == "startup") {
> +        const embeddedExtension = LegacyExtensionsUtils.getEmbeddedExtensionFor(params);
> +
> +        // This is an hybrid addon startup, try to start the embedded extension and if
> +        // there are no loading errors, then call the bootstrap method asynchronously.
> +        params.embeddedWebExtension = {

Let's just call this `webExtension`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4916
(Diff revision 13)
> +        const embeddedExtension = LegacyExtensionsUtils.getEmbeddedExtensionFor(params);
> +
> +        // This is an hybrid addon startup, try to start the embedded extension and if
> +        // there are no loading errors, then call the bootstrap method asynchronously.
> +        params.embeddedWebExtension = {
> +          startup: () => embeddedExtension.startup()

Please add trailing comma.

::: toolkit/mozapps/extensions/test/xpcshell/data/BootstrapMonitor.jsm:15
(Diff revision 13)
>        resourceURI: data.resourceURI.spec,
>      }),
>      reason
>    };
>  
> -  Services.obs.notifyObservers(null, "bootstrapmonitor-event", JSON.stringify(info));
> +  let subject = {};

`{wrappedJSObject: data}`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_embedded.js:103
(Diff revision 13)
> +
> +  let startupInfo = BootstrapMonitor.started.get(ID);
> +
> +  ok(("embeddedWebExtension" in startupInfo.data),
> +    "Got an embeddedWebExtension property in the startup bootstrap method params"
> +  );

Move to previous line.
Attachment #8753348 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8753348 [details]
Bug 1269342 - Integrate LegacyExtensionsUtils helpers into XPIProvider.

https://reviewboard.mozilla.org/r/53230/#review76424

> Please just move this code after the if-else statement rather than making it a function.

agree, it is not needed now that it is always called synchronously.

updated.

> This comment isn't accurate.

True, as the above. removed.

> Let's just call this `webExtension`

+1

it has been renamed in the updated patch.

> `{wrappedJSObject: data}`

I'm assuming that you mean: "just always use the wrapped data object as the subject", am I right?

Changed as suggested in the last version of this comment.
Please clear the unresolved issues in MozReview so this can autoland.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #64)
> Please clear the unresolved issues in MozReview so this can autoland.

My apologies.
 
Issues cleared and checkin-needed re-newed.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc699a478f74
Integrate LegacyExtensionsUtils helpers into XPIProvider. r=aswan,kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc699a478f74
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
This patch contains the changes applied by Attachment 8753348 [details] on mozilla-central rebased on aurora tip.

This patch completes the patches related to Bug 1252215 (recently uplifted to aurora), by letting the XPIProvider to create  instances of the embedded webextension for the addons that has enabled the feature in their install.rdf (if the `hasEmbeddedWebExtension` option is set to `true`), and then shut down the embedded webextension when the container addon is going to shut down.

Follows a brief recap of the minor adaptations that this patch needed, so that it can be cleanly applied on aurora:

- changes to the XPIProvider modules (nothing has been really changed here, the auto-merge fails only because of some minor unrelated changes that are on mozilla-central but not on aurora)
- changes to the test file (because some of the helpers that are currently used by this test file on mozilla-central are not in aurora, e.g. the helper to generate the test addon on the fly)
Comment on attachment 8791928 [details] [diff] [review]
[rebased to aurora] 1-Bug_1269342___Integrate_LegacyExtensionsUtils_helpers_into_XPIProvider__r_aswan_kmag.patch

Approval Request Comment

[Feature/regressing bug #]: 
Bug 1269342

[User impact if declined]:
This patch provides one of the remaining components needed to provide to legacy add-ons a strategy to transition to WebExtensions.

Without a strategy to create transitional addons, developers of existing legacy addon are going to put the transition to WebExtensions on hold, resort to riskier migration strategies, or skipping user data migration steps.
 
[Describe test coverage new/current, TreeHerder]:
This patch introduces a new test file to specifically test that the XPIProvider provides (through the `startup` bootstrap method parameters) an "embedded webextension" to addons which have the "hasEmbeddedWebExtension" install.rdf property set to true, and then ensures that the lifecycle of the embedded webextension is managed correctly when the "container" legacy addon has been disable, enabled, reloaded and uninstalled.

Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc1850aa09e

[Risks and why]:
These changes have no effect unless an add-on explicitly chooses to use this feature by setting "hasEmbeddedWebExtension" to true in their install.rdf.
For the above reason the risks of these changes should be low, because their impact is going to be restricted to the users that have installed add-ons which are directly (and explicitly) using this feature.

[String/UUID change made/needed]:
None
Attachment #8791928 - Flags: approval-mozilla-aurora?
Comment on attachment 8791928 [details] [diff] [review]
[rebased to aurora] 1-Bug_1269342___Integrate_LegacyExtensionsUtils_helpers_into_XPIProvider__r_aswan_kmag.patch

50 moved to Beta today.
Attachment #8791928 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
I'm not sure this should be uplifted to beta. Add-on developers that really need this in 50 can use the changes landed in bug 1252215 with a little extra work. These changes are riskier.
Comment on attachment 8791928 [details] [diff] [review]
[rebased to aurora] 1-Bug_1269342___Integrate_LegacyExtensionsUtils_helpers_into_XPIProvider__r_aswan_kmag.patch

Based on Kris' comment if there is another way of working around this problem, we should use that instead of uplifting this risker patch. I'd prefer to let this one ride the 51 train.
Attachment #8791928 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Blocks: 1306037
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: