Provide to other addon types a way to communicate with WebExtensions addons

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla51
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(7 attachments, 7 obsolete attachments)

58 bytes, text/x-review-board-request
kmag
: review+
Details | Review
58 bytes, text/x-review-board-request
aswan
: review+
kmag
: review+
Details | Review
58 bytes, text/x-review-board-request
kmag
: review+
Details | Review
19.37 KB, patch
kmag
: review+
Details | Diff | Splinter Review
14.76 KB, patch
kmag
: review+
Details | Diff | Splinter Review
1.93 KB, patch
Details | Diff | Splinter Review
6.32 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
This issue is about providing a subset of the communication channel API available to WebExtensions-based add-ons ("chrome.runtime.onConnect", and its Port object API) to other add-on types which need to exchange messages with them.
(Assignee)

Updated

2 years ago
Blocks: 1252227
(Assignee)

Comment 1

2 years ago
This issue is part of the "WebExtension TransitionAPI Proposal"[1]

[1]: https://docs.google.com/document/d/18KCEIohJdQBLsHW2A9HkaZKsXhVEIhk8qvSSzU2GkzA

Updated

2 years ago
Blocks: 1252272
(Assignee)

Updated

2 years ago
Depends on: 1258360
(Assignee)

Comment 2

2 years ago
Added dependency on Bug 1258360, that will introduce the "runtime.onMessageExternal/onConnectExternal" methods that are needed to be able to receive messages and ports from a different sender addon.
(Assignee)

Comment 3

2 years ago
Created attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

- this new module contains helpers to be able to receive connections
  originated from a webextension context from a classic extension context
  (implemented by the `ClassicExtensionContext` class exported from
  this new jsm module)

Review commit: https://reviewboard.mozilla.org/r/49277/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49277/
(Assignee)

Comment 4

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

This is the first piece of the SDK Transition feature, extracted and cleaned up from the previous prototype.

The component (currently named "ClassicExtensionContext") in this first patch is going to provide just the "webextension messaging" part and a group of test cases for it. 
(the "webextension embedding" part will be provided by another helper exported from the same JSM module)
Attachment #8746147 - Flags: feedback?(aswan)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/49277/#review46515

I commented on a few little stylistic things but my larger question is about the way this is intended to be used -- you're using runtime.connect() but that surprises me since I understand that as being for different components within the same extension to communicate with each other, but this is about communicating between two different extensions (a webextension and a classic extension)?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:25
(Diff revision 1)
> +const CC = Components.Constructor;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +const {

style nit, you can merge this line with the one above (adding a {} argument to Cu.import)

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:44
(Diff revision 1)
> +/**
> + * Instances created from this class provides to a classic extension
> + * a simple API to exchange messages with a webextension.
> + */
> +var ClassicExtensionContext = class extends BaseContext {
> +  constructor({id, url}) {

what is url?  it doesn't seem to be used anywhere?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:55
(Diff revision 1)
> +    this[ns].url = url;
> +
> +    let sender = {id: getExtensionUUID(id), url};
> +
> +    let filter = {extensionId: id};
> +    let delegate = {getSender() {}};

from a quick read of ExtensionUtils.jsm it looks like you can just pass null for delegate instead of having no-op methods
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/49277/#review46515

This component is mainly one of the pieces that will provide to a classic extension an API to communicate to its embedded webextension, and given that it is something useful during the transition, I would prefer to do not create a completely new API for it, if it is possible (but I'm obviously open to discuss it, and change the plan accordingly if we have very good reason for doing it differently).

In the "classic extension with an embedded webextension" scenario, the classic extension is for the any of the embedded webextension contexts like another extension context (even if it is a "fake" context), which has the same privileges available to the normal classic extension (but this should not open it to any new privilege that that classic extension doesn't already have).

Another reason to present the exactly same API available to communicate between the various webextension contexts is that, as an example, it will be possible to connect from content script directly to a classic extension context (which I think is nice because an addon developer can switch from the SDK content scripts to the webextensions content scripts if our content scripts works better of the SDK ones even if the entire addon cannot still be rewritten into a real webextension, e.g. because we still lack a feature that the addon uses)

Incidentally this component could be enough by the test pilot main addon to hook itself into the messaging of an "test pilot experiment addon" created using the webextension framework (which is another possible, and requested, use case but it is not the main one)
(Assignee)

Comment 7

2 years ago
https://reviewboard.mozilla.org/r/49277/#review46523

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:44
(Diff revision 1)
> +/**
> + * Instances created from this class provides to a classic extension
> + * a simple API to exchange messages with a webextension.
> + */
> +var ClassicExtensionContext = class extends BaseContext {
> +  constructor({id, url}) {

It is something that is not probably used but anyone, but it can be useful if we log the messages and we want to recognize who is the sender (in the Extension.jsm the regular extension contexts set it here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#263)

I remember that at some point I renamed it to containerUrl to be more explicit about what it is, but it is probably in one of the other experimental draft commit that are built on this one.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:55
(Diff revision 1)
> +    this[ns].url = url;
> +
> +    let sender = {id: getExtensionUUID(id), url};
> +
> +    let filter = {extensionId: id};
> +    let delegate = {getSender() {}};

I tried to stay consistent to what the regular extension context does internally, but I agree that it doesn't seems to be of any value, I'm going take a look and remove it if we doesn't create any issue.

Comment 8

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Discussed connect vs connectExternal on IRC, connect() does sound like the right thing here.
Attachment #8746147 - Flags: feedback?(aswan) → feedback+

Updated

2 years ago
Whiteboard: triaged
(Assignee)

Comment 9

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/1-2/
Attachment #8746147 - Attachment description: MozReview Request: Bug 1252215 - [webext] add ClassicExtensionsUtils JSM module. f?aswan → MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r?aswan
Attachment #8746147 - Flags: feedback+ → review?(aswan)
(Assignee)

Updated

2 years ago
Blocks: 1269342
(Assignee)

Comment 10

2 years ago
I've just updated this patch, based on the previous feedback comment and the related discussion on irc.

This new version has:

- more inline comments (and one of those explains the `delegate.getSender` purpose)

- export Management from Extension.jsm (needed to get access to the `Management.emit` helper needed to be able
  to fill the `sender.tab` property when the port is received from a tab (e.g. a content script)

- an additional test case which ensures that the new ClassicExtensionContext can receive a connection from
  a content script (I put this new scenario in a different test file because currently it cannot work on android,
  and the other test case with the background page works correctly on Android like it is)

Updated

2 years ago
Attachment #8746147 - Flags: review?(aswan) → review+
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review47339

It looks good to me.  A few nitpicks about comments...

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:12
(Diff revision 2)
> +this.EXPORTED_SYMBOLS = ["ClassicExtensionContext"];
> +
> +/* exported ClassicExtensionContext */
> +
> +/**
> + *  This file exports helpers for Classic Extensions that wants to embed a webextensions

grammar nitpick, drop the s from wants and then make webextension singular (drop the s, change the apostrophe on the next line and drop the s from contexts)

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:45
(Diff revision 2)
> +const systemPrincipal = CC("@mozilla.org/systemprincipal;1", "nsIPrincipal")();
> +
> +const ns = Symbol();
> +
> +/**
> + *  Instances created from this class provides to a classic extension

grammar nitpick: drop the s from provides

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:73
(Diff revision 2)
> +  constructor({targetAddonId, targetExtension, senderId, senderURL}) {
> +    if (!targetAddonId) {
> +      throw new Error("targetAddonId parameter is mandatory");
> +    }
> +
> +    super(targetAddonId);

shouldn't this actually be the id of the addon that is creating the context and not the target?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:138
(Diff revision 2)
> +  shutdown() {
> +    this.unload();
> +  }
> +
> +  /**
> +   *  This method is called when an the extension shuts down or unloaded.

grammar nitpick: take out "an"

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:161
(Diff revision 2)
> +  /**
> +   *  The custom type of this context (that will always be
> +   *  "classic_extension").
> +   */
> +  get type() {
> +    // Instead of the usually set extension context type, always return

you have a comment outside the method, this one isn't needed

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:58
(Diff revision 2)
> +  yield extension.awaitMessage("webextension-ready");
> +
> +  let classicContext = new ClassicExtensionContext({
> +    targetAddonId: extension.id,
> +    sourceAddonId: "@fake-source-addon-id",
> +    sourceContextURL: "about:blank",

what is this?  (and also the line above?)

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context_contentscript.html:92
(Diff revision 2)
> +
> +  let port = yield waitConnectPort;
> +
> +  ok(port, "Got the Port API object");
> +  ok(port.sender, "The port has a sender property");
> +  ok(port.sender.id, "The port sender has an id property");

can we validate the contents of these properties?
(Assignee)

Comment 12

2 years ago
Created attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review commit: https://reviewboard.mozilla.org/r/50803/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50803/
Attachment #8749196 - Flags: review?(aswan)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/2-3/
(In reply to Andrew Swan [:aswan] from comment #5)
> 
> I commented on a few little stylistic things but my larger question is about
> the way this is intended to be used -- you're using runtime.connect() but
> that surprises me since I understand that as being for different components
> within the same extension to communicate with each other, but this is about
> communicating between two different extensions (a webextension and a classic
> extension)?

Just to chime in as a Test Pilot team member:

The way we're building Test Pilot is that there's a main add-on built using the Add-on SDK. That add-on communicates with the Test Pilot website and manages experimental Firefox features implemented as add-ons. We want to support experimental features built as both classic extensions and as webextensions.

Two aspects of what the main add-on offers to experiment add-ons:

1) An API for experiments to send metrics data back to Test Pilot servers.
2) Feature flipping configuration data pulled from Test Pilot servers (e.g. for a/b testing, incremental roll-out, etc).

The main add-on manages user authentication and access to profile data, so individual experiments don't have to. So far, we've been doing this via nsIObserverService events. To support webextensions, though, we need a different channel. We're hoping the solution to this bug gives us that.
(Assignee)

Comment 15

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/3-4/
Attachment #8746147 - Attachment description: MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r?aswan → MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r=aswan
(Assignee)

Comment 16

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/1-2/
(Assignee)

Updated

2 years ago
Assignee: nobody → lgreco
(Assignee)

Comment 17

2 years ago
(In reply to Luca Greco [:rpl] from comment #15)
> Comment on attachment 8746147 [details]
> MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module
> and ClassicExtensionContext helper. r=aswan
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/49277/diff/3-4/

This new revision of the first patch applies the following tweaks:

- removed the sourceAddonId parameter from the ClassicExtensionContext, mainly because 
  ClassicExtensionContext shouldn't be perceived as a component to directly 
  achieve cross-addon messaging, that will be better achieved from a webextension
  (and an embedded webextension in case of a classic extension, Bug 1252227 and Bug 1269342) 
  using the upcoming "onConnectExternal/onMessageExternal" API methods (Bug 1258360)

- minor tweaks to inline comments and the test cases
(Assignee)

Comment 18

2 years ago
(In reply to Luca Greco [:rpl] from comment #16)
> Comment on attachment 8749196 [details]
> MozReview Request: Bug 1252215 - [webext] Add xpcshell test for
> ClassicExtensionContext params validation. r?aswan
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/50803/diff/1-2/

Minor fix (fixed indentation in the xpcshell test case)
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

https://reviewboard.mozilla.org/r/50803/#review48177
Attachment #8749196 - Flags: review?(aswan) → review+
(Assignee)

Comment 20

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

This patch has been reviewed by Andrew before, but as discussed on irc, it is related to some of our internals and so it worth giving it a second look.

r? assigned to Kris.
Attachment #8746147 - Flags: review+ → review?(kmaglione+bmo)
(Assignee)

Comment 21

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

This patch is an incremental over the above (it adds an additional xpcshell-based test unit).

r? assigned to Kris.
Attachment #8749196 - Flags: review+ → review?(kmaglione+bmo)
(Assignee)

Comment 22

2 years ago
Created attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review commit: https://reviewboard.mozilla.org/r/52517/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52517/
Attachment #8749196 - Attachment description: MozReview Request: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. r?aswan → MozReview Request: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. r=aswan
Attachment #8752300 - Flags: review?(kmaglione+bmo)
Attachment #8752300 - Flags: review?(aswan)
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Attachment #8749196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 23

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/4-5/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/2-3/
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 25

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Set r? to Kris to double-check the previous review from Andrew (this patch is unchanged, a new patch has been added to the mozreview queue)
Attachment #8746147 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 26

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Set r? to Kris to double-check the previous review from Andrew (this patch is unchanged, a new patch has been added to the mozreview queue)
Attachment #8749196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 27

2 years ago
(In reply to Luca Greco [:rpl] from comment #22)
> Created attachment 8752300 [details]
> MozReview Request: Bug 1252215 - [webext] add sendMessage support and
> mochitest to ClassicExtensionContext. r?aswan,kmag
> 
> Review commit: https://reviewboard.mozilla.org/r/52517/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/52517/

Added a new patch into this mozreview queue:

during the test cases creation I started to think that it can be reasonable to give access to both onConnect and
onMessage in the exposed API, mainly because onMessage can be nicer for short living data exchanges.
(Assignee)

Comment 28

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/1-2/
(Assignee)

Comment 29

2 years ago
(In reply to Luca Greco [:rpl] from comment #28)
> Comment on attachment 8752300 [details]
> MozReview Request: Bug 1252215 - [webext] add sendMessage support and
> mochitest to ClassicExtensionContext. r?aswan,kmag
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/52517/diff/1-2/

Pushed a small change on the last patch in the review queue: fixed two eslint errors.
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

https://reviewboard.mozilla.org/r/52517/#review50152

looks good to me
Attachment #8752300 - Flags: review?(aswan) → review+
(Assignee)

Comment 31

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/5-6/
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Attachment #8749196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 32

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/3-4/
(Assignee)

Comment 33

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/2-3/
(Assignee)

Updated

2 years ago
Attachment #8746147 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8749196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 34

2 years ago
Created attachment 8754934 [details]
Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.

Review commit: https://reviewboard.mozilla.org/r/54308/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54308/
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Attachment #8749196 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 35

2 years ago
Created attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

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/54310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54310/
(Assignee)

Comment 36

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/6-7/
(Assignee)

Comment 37

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/4-5/
(Assignee)

Comment 38

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/3-4/
(Assignee)

Updated

2 years ago
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Attachment #8749196 - Flags: review?(kmaglione+bmo)
Attachment #8754935 - Flags: review?(kmaglione+bmo)
Attachment #8754935 - Flags: review?(aswan)
(Assignee)

Updated

2 years ago
Attachment #8754934 - Flags: review?(felipc)
Comment on attachment 8754934 [details]
Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.

https://reviewboard.mozilla.org/r/54308/#review50994
Attachment #8754934 - Flags: review?(felipc) → review+
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review51284

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:19
(Diff revision 7)
> + */
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +const Cu = Components.utils;
> +const Cr = Components.results;

`const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components`

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:37
(Diff revision 7)
> +const {
> +  ExtensionUtils: {
> +    Messenger,
> +    BaseContext,
> +  },
> +} = Cu.import("resource://gre/modules/ExtensionUtils.jsm", {});

Please don't try to retrive exports from the return value of `Cu.import`. Aside from the issues with lexically scoped variables, it gives access to non-public symbols that may not intentionally be exported.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:40
(Diff revision 7)
> +    BaseContext,
> +  },
> +} = Cu.import("resource://gre/modules/ExtensionUtils.jsm", {});
> +
> +
> +const systemPrincipal = CC("@mozilla.org/systemprincipal;1", "nsIPrincipal")();

Please don't use `Components.Constructor` for one-off instance creation. 

And for this, you should use `Services.scriptSecurityManager.getSystemPrincipal()`

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:42
(Diff revision 7)
> +} = Cu.import("resource://gre/modules/ExtensionUtils.jsm", {});
> +
> +
> +const systemPrincipal = CC("@mozilla.org/systemprincipal;1", "nsIPrincipal")();
> +
> +const ns = Symbol();

Please use a WeakMap instead.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:45
(Diff revision 7)
> +const systemPrincipal = CC("@mozilla.org/systemprincipal;1", "nsIPrincipal")();
> +
> +const ns = Symbol();
> +
> +/**
> + *  Instances created from this class provide to a classic extension

Only one space after `*`. Same for the other doc comments.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:48
(Diff revision 7)
> +
> +/**
> + *  Instances created from this class provide to a classic extension
> + *  a simple API to exchange messages with a webextension.
> + */
> +var ClassicExtensionContext = class extends BaseContext {

`class ClassicExtensionContext extends BaseContext`

And, can we make this `Legacy` rather than `Classic`?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:53
(Diff revision 7)
> +var ClassicExtensionContext = class extends BaseContext {
> +  /**
> +   *  Create a new ClassicExtensionContext given an addon id and an optional
> +   *  url (which can be used to recognized the messages of container context).
> +   *
> +   *  @param targetAddonId: (String)

`@param {string} targetAddonId`

Same for the others.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:57
(Diff revision 7)
> +   *
> +   *  @param targetAddonId: (String)
> +   *    The Addon id of the target webextension.
> +   *  @param optionalParams: (Object)
> +   *    An object with the following properties:
> +   *    - targetExtension: (Extension [optional])

`@param {Extension} [optionalParams.targetExtension]`

Same for the other properties.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:83
(Diff revision 7)
> +      }
> +    }
> +
> +    super(targetAddonId);
> +
> +    this.isClassicExtensionContext = true;

Do we really need this?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:93
(Diff revision 7)
> +    //  - does not have any permission on its own
> +    this.extension = targetExtension || {
> +      id: targetAddonId,
> +      uuid: targetAddonId,
> +      hasPermission: () => false,
> +    };

This seems like a recipe for trouble. Why would we need a fake extension instance?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:117
(Diff revision 7)
> +    // be able to resolve the tab object when the connection is originated
> +    // from a tab (e.g. a content script).
> +    Management.emit("page-load", this, {type: this.type}, sender, delegate);
> +
> +    // Create private instance data.
> +    this[ns] = {};

This isn't a pattern that we use outside of the SDK. If we really need public and private versions of an object, then there should be a public wrapper that forwards to the private class.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:130
(Diff revision 7)
> +
> +    this[ns].cloneScope = Cu.Sandbox(this[ns].addonPrincipal, {});
> +    Cu.setSandboxMetadata(this[ns].cloneScope, {addonId: targetAddonId});
> +
> +    this[ns].api = {
> +      onConnect: this[ns].messenger.onConnect("runtime.onConnect"),

Is there any particular reason to only support `onConnect`?

::: toolkit/components/extensions/Extension.jsm:7
(Diff revision 7)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["Extension", "ExtensionData"];
> +this.EXPORTED_SYMBOLS = ["Extension", "ExtensionData", "Management"];

Why do we need to export `Management`?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:18
(Diff revision 7)
> +<script type="text/javascript">
> +"use strict";
> +
> +const {
> +  ClassicExtensionContext,
> +} = SpecialPowers.Cu.import("resource://gre/modules/ClassicExtensionsUtils.jsm", {});

We shouldn't be importing this from a content process, which a plain mochitest will be running in.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:21
(Diff revision 7)
> +const {
> +  ClassicExtensionContext,
> +} = SpecialPowers.Cu.import("resource://gre/modules/ClassicExtensionsUtils.jsm", {});
> +
> +/**
> + *  This test case ensures that ClassicExtensionContext instances:

Only one leading space.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:30
(Diff revision 7)
> + *     Port created in the webextension's background page
> + *   - the received Port instance can exchange messages with the background page
> + *   - the received Port receive a disconnect event when the webextension is
> + *     shutting down
> + */
> +add_task(function* classic_extension_context() {

Please start all test function names with `test_`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:32
(Diff revision 7)
> + *   - the received Port receive a disconnect event when the webextension is
> + *     shutting down
> + */
> +add_task(function* classic_extension_context() {
> +  function backgroundScript() {
> +    let bgURL = String(window.location);

`window.location.href`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:37
(Diff revision 7)
> +    let bgURL = String(window.location);
> +
> +    let extensionInfo = {
> +      bgURL,
> +      // Extract the assigned uuid from the background page url.
> +      uuid: bgURL.match("://(.*)/")[1],

`.+?`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:44
(Diff revision 7)
> +
> +    browser.test.sendMessage("webextension-ready", extensionInfo);
> +
> +    browser.test.onMessage.addListener(msg => {
> +      if (msg == "do-connect") {
> +        let port = chrome.runtime.connect();

s/chrome/browser/

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:88
(Diff revision 7)
> +
> +  let port = yield waitConnectPort;
> +
> +  ok(port, "Got the Port API object");
> +  ok(port.sender, "The port has a sender property");
> +  is(port.sender && port.sender.id, extensionInfo.uuid,

No need for the `port.sender &&`
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

https://reviewboard.mozilla.org/r/50803/#review51288

Please roll this into the previous commit.

::: toolkit/components/extensions/test/xpcshell/test_classic_extension_utils.js:21
(Diff revision 5)
> + */
> +add_task(function* classic_extension_context_params_validation() {
> +  // Missing mandatory targetAddonId
> +  Assert.throws(() => {
> +    new ClassicExtensionContext();
> +  }, "targetAddonId parameter is mandatory");

Please add a regexp to test the exception's actual message. For the others too.
Attachment #8749196 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

https://reviewboard.mozilla.org/r/52517/#review51290

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:131
(Diff revision 4)
>      this[ns].cloneScope = Cu.Sandbox(this[ns].addonPrincipal, {});
>      Cu.setSandboxMetadata(this[ns].cloneScope, {addonId: targetAddonId});
>  
>      this[ns].api = {
>        onConnect: this[ns].messenger.onConnect("runtime.onConnect"),
> +      onMessage: this[ns].messenger.onMessage("runtime.onMessage"),

Why not `sendMessage` as well?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_context.html:95
(Diff revision 4)
> +
> +  let {singleMsg, msgSender} = yield waitMessage;
> +  is(singleMsg, "webextension -> classic_extension message",
> +     "Got the expected message");
> +  ok(msgSender, "Got a message sender object");
> +  is(msgSender && msgSender.id, extensionInfo.uuid,

No need for the `msgSender &&`
Attachment #8752300 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 43

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/7-8/
Attachment #8746147 - Attachment description: MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r=aswan → MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r?kmag,aswan
Attachment #8749196 - Attachment description: MozReview Request: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. r=aswan → MozReview Request: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. r=kmag,aswan
Attachment #8752300 - Attachment description: MozReview Request: Bug 1252215 - [webext] add sendMessage support and mochitest to ClassicExtensionContext. r?aswan,kmag → MozReview Request: Bug 1252215 - [webext] add sendMessage support and mochitest to ClassicExtensionContext. r=aswan,kmag
Attachment #8754934 - Attachment description: MozReview Request: Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals. → MozReview Request: Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals. r=Felipe
Attachment #8754935 - Attachment description: MozReview Request: Bug 1252215 - [webext] Add EmbeddedWebExtensionsUtils helper. → MozReview Request: Bug 1252215 - [webext] Add EmbeddedWebExtensionsUtils helper. r?kmag,aswan
Attachment #8746147 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 44

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/5-6/
(Assignee)

Comment 45

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/4-5/
(Assignee)

Comment 46

2 years ago
Comment on attachment 8754934 [details]
Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54308/diff/1-2/
(Assignee)

Comment 47

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/1-2/
(Assignee)

Comment 48

2 years ago
https://reviewboard.mozilla.org/r/49277/#review51284

> `class ClassicExtensionContext extends BaseContext`
> 
> And, can we make this `Legacy` rather than `Classic`?

To be honest, in the very first version I named them "Legacy\*", but I didn't like how they sound and I opted for Classic.

Do you think it would be better to rename all the files and names to "Legacy\*"?

(besides the above "taste" issue, I'm not totally against to use Legacy instead, but I'm not sure that the renaming worth the time spent to rename everything)

> Is there any particular reason to only support `onConnect`?

There are 3 reasons why I chose to support communication only in one direction (started from the webextension, and with the legacy context as the target), follows a brief description (in order of importance, at least in my opinion):

- timing reasons: there is an high chance that the addon developer will try to use the API during the container addon startup, and the webextension context that should receive the message (or the port connection) will not be ready to receive it
(but we can, and we are going to, garantee that the opposite is true from the XPIProvider that is the orchestrator of the startup/shutdown of both the legacy container and the embedded webextension parts of the addon) 

- review reasons: if the communication needs to be started by the embedded webextension, it should be easier for a reviewer to figure out what the exchange of messages is doing and where is the sending/receiving of the messages

- transition reasons: by being the passive part of the communication, the legacy part better fits in the role of a service, which (I hope) will help the perception of the embedded webextension as the main part of the addon

Nevertheless, we can change that in followups if we have reasons to.

> Why do we need to export `Management`?

The reason is explained in the inline comment above its usage in the ClassicExtensionContext.jsm:

```
    // This page-load event is handled synchronously by ext-tabs.js
    // and will put a getSender method in the delegate object, that will
    // be able to resolve the tab object when the connection is originated
    // from a tab (e.g. a content script).
```

basically, we need it to get the sender info for port and messages that are coming from a tab.
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review54472

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:264
(Diff revision 2)
> +
> +  /**
> +   * Start the embedded webextension (and report any loading error in the Browser console).
> +   */
> +  startup() {
> +    if (this.started || this.pendingStartup) {

initialize started, pendingStartup, and pendingShutdown to false in the constructor?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:20
(Diff revision 2)
> +
> +const {
> +  ClassicExtensionsUtils: {
> +    EmbeddedWebExtensionsUtils,
> +  },
> +} = SpecialPowers.Cu.import("resource://gre/modules/ClassicExtensionsUtils.jsm", {});

since you're in chrome i don't think you need SpecialPowers?
Attachment #8754935 - Flags: review?(aswan) → review+
(Assignee)

Comment 50

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/8-9/
Attachment #8746147 - Attachment description: MozReview Request: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. r?kmag,aswan → Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper.
Attachment #8749196 - Attachment description: MozReview Request: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. r=kmag,aswan → Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation.
Attachment #8752300 - Attachment description: MozReview Request: Bug 1252215 - [webext] add sendMessage support and mochitest to ClassicExtensionContext. r=aswan,kmag → Bug 1252215 - [webext] add sendMessage support and mochitest to ClassicExtensionContext.
Attachment #8754934 - Attachment description: MozReview Request: Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals. r=Felipe → Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.
Attachment #8754935 - Attachment description: MozReview Request: Bug 1252215 - [webext] Add EmbeddedWebExtensionsUtils helper. r?kmag,aswan → Bug 1252215 - [webext] Add EmbeddedWebExtensionsUtils helper.
Attachment #8746147 - Flags: review?(lgreco)
(Assignee)

Comment 51

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/6-7/
(Assignee)

Comment 52

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/5-6/
(Assignee)

Comment 53

2 years ago
Comment on attachment 8754934 [details]
Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54308/diff/2-3/
(Assignee)

Comment 54

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/2-3/
(Assignee)

Comment 55

2 years ago
https://reviewboard.mozilla.org/r/54310/#review54472

> initialize started, pendingStartup, and pendingShutdown to false in the constructor?

yep, sounds reasonable, applied in the last update.

> since you're in chrome i don't think you need SpecialPowers?

Right! in the last update I've removed all the SpecialPowers usage from this test file.
(Assignee)

Updated

2 years ago
Attachment #8746147 - Flags: review?(lgreco)
Adding this as a Test Pilot dependency as we have a WebExtension active in our pipeline that we'd like to ship through Test Pilot.
Blocks: 1257690
(Assignee)

Comment 57

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/9-10/
(Assignee)

Comment 58

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/7-8/
(Assignee)

Comment 59

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/6-7/
(Assignee)

Comment 60

2 years ago
Comment on attachment 8754934 [details]
Bug 1252215 - Add isDeeply to mochitest.eslintrc configured globals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54308/diff/3-4/
(Assignee)

Comment 61

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/3-4/
(Assignee)

Comment 62

2 years ago
In the last push of updated patches:

- rebased on a recent fx-team tip
- fixed eslint errors related to the valid-jsdoc rule
- applied minor tweaks on the shutdown method defined in the last patch (related to last review comment on the XPIProvider patch in Bug 1269342)
(Assignee)

Comment 63

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/10-11/
(Assignee)

Comment 64

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/8-9/
(Assignee)

Comment 65

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/7-8/
(Assignee)

Comment 66

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/4-5/
(Assignee)

Updated

2 years ago
Attachment #8754934 - Attachment is obsolete: true
(Assignee)

Comment 67

2 years ago
Just pushed a small update on this patch queue:

- removed the patch that adds isDeeply to the mochitest eslintrc (because it landed as part of the connectNative patches)
- small fix on a test (due to a change in the expected validation error message)
https://reviewboard.mozilla.org/r/54310/#review51296

Urgh. Apparently I didn't manage to submit this properly when I wrote it weeks ago...

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:157
(Diff revision 1)
> +   *    - resolve: (Function)
> +   *      to be called to resolve the pending startup promise.
> +   *    - reject: (Function)
> +   *      to be called to reject the pending startup promise.
> +   */
> +  setupStartupPromise() {

This doesn't need its own method. It also doesn't need to be nearly this complicated.

    this.startupPromise = new Promise((resolve, reject) => {
      this.startupDeferred = {resolve, reject};
    });

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:262
(Diff revision 1)
>      return this[ns].api;
>    }
>  };
> +
> +/**
> + *  Instances of this class are used internally by the exported EmbeddedWebExtensionsUtils

One leading space.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:266
(Diff revision 1)
> +/**
> + *  Instances of this class are used internally by the exported EmbeddedWebExtensionsUtils
> + *  to manage the embedded webextension instance and the related ClassicExtensionContext
> + *  instance used to exchange messages with it.
> + */
> +class EmbeddedWebExtension {

Should be `EmbeddedExtension` for consistency.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:274
(Diff revision 1)
> +   *  container addon (the webextension resources will be loaded from the "webextension/"
> +   *  subdir of the base resource URI for the classic extension addon).
> +   *
> +   *  @param containerAddonParams: (Object)
> +   *    An object with the following properties:
> +   *    - id: (String)

`@param {string} options.id`

Same for the others.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:275
(Diff revision 1)
> +   *  subdir of the base resource URI for the classic extension addon).
> +   *
> +   *  @param containerAddonParams: (Object)
> +   *    An object with the following properties:
> +   *    - id: (String)
> +   *      The Addon id of the Classic Extension which will contain the embedded webextension.

*add-on

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:282
(Diff revision 1)
> +   *      The nsIURI of the Classic Extension container addon.
> +   */
> +  constructor({id, resourceURI}) {
> +    this.addonId = id;
> +
> +    let webextensionURI = Services.io.newURI(resourceURI.resolve("webextension/"), null, null);

`Services.io.newURI("webextension/", null, resourceURI)`

Is there some reason we don't just use the root directory for this?

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:284
(Diff revision 1)
> +  constructor({id, resourceURI}) {
> +    this.addonId = id;
> +
> +    let webextensionURI = Services.io.newURI(resourceURI.resolve("webextension/"), null, null);
> +
> +    this.webextension = new Extension({

Should be `this.extension` for consistency.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:289
(Diff revision 1)
> +    this.webextension = new Extension({
> +      id,
> +      resourceURI: webextensionURI,
> +    });
> +
> +    this.classicExtensionContext = new ClassicExtensionContext(id, {

Should be `this.context` for consistency.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:297
(Diff revision 1)
> +    });
> +
> +    // Setup the startup promise.
> +    this.classicExtensionContext.setupStartupPromise();
> +
> +    // destroy the ClassicExtensionContext cloneScope when

Capitalize.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:345
(Diff revision 1)
> +        // (with the error object as parameter).
> +        let id = this.addonId;
> +
> +        // Adjust the error message to nicely handle both the exception and
> +        // the validation errors scenarios.
> +        let msg = err.errors ? JSON.stringify(err.errors, null, 2) : err.message;

Please expand to a multi-line `if`.

::: toolkit/components/extensions/ClassicExtensionsUtils.jsm:444
(Diff revision 1)
> +      embeddedWebExtension.shutdown();
> +      embeddedWebExtensionsMap.delete(id);
> +    } else {
> +      Cu.reportError(`No embedded WebExtension found for addonId ${id}`);
> +    }
> +  },

There are too many levels of indirection here. You have 4 levels of indirection for the `api` property. To quote a friend of mine, "it just doesn't seem to me like some of the abstractions are earning their keep."

Please just create one method which returns an object with startup and shutdown methods, along with the APIs that should be available to it.

Also, it would help to see how this is intended to be used.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:25
(Diff revision 1)
> +const {
> +  Extension,
> +} = SpecialPowers.Cu.import("resource://gre/modules/Extension.jsm", {});
> +
> +/**
> + *  This test case ensures that the EmbeddedWebExtensionsUtils:

Please remove extra leading space.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:39
(Diff revision 1)
> + *   - The port object receive a disconnect event when the embedded webextension is
> + *     shutting down
> + */
> +add_task(function* test_embedded_webextension_utils() {
> +  function backgroundScript() {
> +    let port = chrome.runtime.connect();

s/chrome/browser/

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:50
(Diff revision 1)
> +      }
> +    });
> +  }
> +
> +  const id = "@test.embedded.web.extension";
> +  let xpi = Extension.generateXPI(id, {

I don't understand why you're using `generateXPI` for this.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:52
(Diff revision 1)
> +  }
> +
> +  const id = "@test.embedded.web.extension";
> +  let xpi = Extension.generateXPI(id, {
> +    files: {
> +      "webextension/manifest.json": `{

Just make this an object literal and it will automatically be converted to JSON.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_classic_extension_embedding.html:72
(Diff revision 1)
> +  SimpleTest.registerCleanupFunction(() => {
> +    SpecialPowers.Services.obs.notifyObservers(xpi, "flush-cache-entry", null);
> +    xpi.remove(false);
> +  });
> +
> +  let fileURI = SpecialPowers.Services.io.newFileURI(xpi);

No `SpecialPowers` in chrome tests.
(Assignee)

Comment 69

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/11-12/
Attachment #8754935 - Attachment description: Bug 1252215 - [webext] Add EmbeddedWebExtensionsUtils helper. → Bug 1252215 - [webext] Add EmbeddedExtensionsUtils helper.
(Assignee)

Comment 70

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/9-10/
(Assignee)

Comment 71

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/8-9/
(Assignee)

Comment 72

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/5-6/
(Assignee)

Comment 73

2 years ago
https://reviewboard.mozilla.org/r/54310/#review51296

> This doesn't need its own method. It also doesn't need to be nearly this complicated.
> 
>     this.startupPromise = new Promise((resolve, reject) => {
>       this.startupDeferred = {resolve, reject};
>     });

Honestly, I'm sure about this, it looks like an over-semplification, Andrew and I have been iterating over the handling of startup/shutdown promise and the related error property for a while (take into account that you commented the revision 1 and this patch is at its revision 6th revision and this entire set of patches is at its 13th).

This kind of change doesn't cover what we discussed in the previous comments with Andrew, and I'd like to hear his opinion before trying any further change on it (and all the testing coverage other this part of this patches), if we finally agreed that it is a real issue I'm open to re-look at it.

> `Services.io.newURI("webextension/", null, resourceURI)`
> 
> Is there some reason we don't just use the root directory for this?

Like discussed during MozLondon (and like I wrote in the design document and in the brief presentation), the reason is that we want:

- to be able to identify what the addon developer has been already able to move into the pure webextension embedded into the hybrid addon and what is still in the container based on the legacy technologies
- to prevent the webextension part to load any asset that is outside the `webextension/` subdir

In my vision this is very important, and I'm not going to give up on it ;-)

> Should be `this.extension` for consistency.

I think that this variable is not the same we that we usually name `this.extension`, and so I would prefer if we can name it differently, but at the same time I don't think that continuing a "battle for naming" is of any help (especially giving that these are all names that are private to this JSM and are not going to be visible or used outside of it).

Here is my proposal (that I think is in the middle of our two opinions about it):

- this will be called the embedded extension is going to be `this.embeddedExtension`
- the context related to the container legacy part is going to be `this.containerContext`

> There are too many levels of indirection here. You have 4 levels of indirection for the `api` property. To quote a friend of mine, "it just doesn't seem to me like some of the abstractions are earning their keep."
> 
> Please just create one method which returns an object with startup and shutdown methods, along with the APIs that should be available to it.
> 
> Also, it would help to see how this is intended to be used.

I'm a great fan of Ron Gilbert, but I'm not going to fight a "Monkey Island"-styled "quotes battle" with you... it is a battle that clearly I'll lose :-)

This methods are just the simplified exported interface that the XPIProvider.jsm is going to use to integrate the embedded webextension lifecycle management into `callBootstrapMethod`, and to keep the impact of the XPIProvider as low as possible.

I've changed this patch so that they are directly exported instead of exporting another object which contains this methods, that makes the change on the XPIProvider even a bit nicer and makes more clear what is their role (and to look how it is going to be used you can look at the patch attached on Bug 1269342, which should be already in your review queue, from a pretty long time now)

Besides the above applied change, I think that it is not a real issue, and there are not 4 level of indirection because the lifecycle methods are not part of the API (and so the levels of indirection are just 2 if we really want to count them)

I'm clearing this as an issue because in my opinion it looks more like a matter of taste than a real issue.

We can discuss it further if you feel that it is really important and if we are able to point out the exact "reasons why", but like one of my most important mentors inside Mozilla said to me once: "the perfect is the enemy of the good" and this patch is in the review queue from a long time now, and in my opinion this feature cannot miss Firefox 50, absolutely (or I'm going to lie down on the railroad of the "release train", joking-not-joking :-)).
(Assignee)

Comment 74

2 years ago
I had to drop a good part of the comments marked as issues in Comment 68, because they were related to the revision 1 of the patch and they were already been fixed long time ago.

I understand that these patches are not exactly small, but I've tried to keep them as small as possible (and most of the code is actually tests), and they are in reviews from both Andrew and Kris, if it can be helpful I'm always available to briefly discuss the patches in review over irc so that we are sure that we're looking at the same page (because "teleporting" the issues pointed out from a past revision of the patch to the last one is a painful, and potentially error prone, process for both the reviewer and the author).
(Assignee)

Comment 75

2 years ago
Andrew, may I ask you your opinion about the issue related to the setup of the startup promise? (which has been commented by Kris as part of Comment 68)

As I'm pointing out in Comment 73, I'm a bit worried  that the proposed simplification could oversimplify an issue that we have discussed and tweaked for a long time now, and before making any change of it (and on the part where the change will have an impact) and like to hear you opinion about it.
Flags: needinfo?(aswan)
(Assignee)

Comment 76

2 years ago
Updated push to try (including of all the patches related to this features, which are Bug 1252215, Bug 1269342, Bug 1269347 and Bug 1252227):

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

Comment 77

2 years ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/12-13/
Attachment #8754935 - Attachment description: Bug 1252215 - [webext] Add EmbeddedExtensionsUtils helper. → Bug 1252215 - [webext] Add Embedded Extensions helper to ClassicExtensionsUtils.
(Assignee)

Comment 78

2 years ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/10-11/
(Assignee)

Comment 79

2 years ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/9-10/
(Assignee)

Comment 80

2 years ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/6-7/
(Assignee)

Comment 81

2 years ago
Small recap of the last changes applied based on the last review comments:

- simplification of the startupPromise & abstraction levels: I've briefly discussed with Andrew over irc (and so I'm clearing the needinfo for him) and we agreed on some changes to apply the requested simplification without losing anything of the previously requested changes (and the simplified version passes the tests written for the previous version, as expected by an internal refactoring that should keep the behavior mostly unmodified), diff at
https://reviewboard.mozilla.org/r/54310/diff/6-7/

- remove "WebExtension"/"webextension" from class names, var names etc.
  e.g.: "this.webextension" -> "this.embeddedExtension", and other similar
  changes) and other minor tweaks, diff at https://reviewboard.mozilla.org/r/54310/diff/5-6/

Small recap of the status of all the patches attached to this issue:
- the 2nd and the 3rd patches in the queue are already r+ by both Kris and Andrew
- the first and the last patches are already r+ from Andrew, they both need the review confirmation from Kris on the last changes applied

New try build:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f20f046f2d
Flags: needinfo?(aswan)
(Assignee)

Comment 82

a year ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/13-14/
Attachment #8746147 - Attachment description: Bug 1252215 - [webext] ClassicExtensionsUtils JSM module and ClassicExtensionContext helper. → Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.
Attachment #8749196 - Attachment description: Bug 1252215 - [webext] Add xpcshell test for ClassicExtensionContext params validation. → Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.
Attachment #8752300 - Attachment description: Bug 1252215 - [webext] add sendMessage support and mochitest to ClassicExtensionContext. → Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.
Attachment #8754935 - Attachment description: Bug 1252215 - [webext] Add Embedded Extensions helper to ClassicExtensionsUtils. → Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.
(Assignee)

Comment 83

a year ago
Comment on attachment 8749196 [details]
Bug 1252215 - [webext] Add xpcshell test for LegacyExtensionContext params validation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50803/diff/11-12/
(Assignee)

Comment 84

a year ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/10-11/
(Assignee)

Comment 85

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/7-8/
(Assignee)

Comment 86

a year ago
Push to try of the updated patches:

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

Comment 87

a year ago
As per discussion over IRC with Kris, related to the remaining changes needed to this patch set, I've applied the following changes:

- renamed files, class names, inline comments and hg summary messages (changing "classic" into "legacy")

- directly export the EmbeddedExtension class from the LegacyExtensionsUtils.jsm module and removed the helpers that keeps track of the existent embedded extension inside the new JSM module, the tracking, startup and shutdown of the embedded extensions has been moved into the XPIProvider patch attached to Bug 1269342.

Unfortunately the renamed files are not recognized by mozreview, because they were added in these patches and so "hg mv" cannot help mozreview to produce a better diff.

The applied changes are minimal (just the renames and the removal of a couple of helper function exported by the new jsm module defined in these patches), and the actual changes can be more easily recognized by diff the previous version of the file with the renamed one.
(Assignee)

Updated

a year ago
No longer depends on: 1258360
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review62970

::: toolkit/components/extensions/Extension.jsm:7
(Diff revision 14)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["Extension", "ExtensionData"];
> +this.EXPORTED_SYMBOLS = ["Extension", "ExtensionData", "Management"];

I don't think we should be exporting `Management` by default, if at all.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:31
(Diff revision 14)
> +                                  "resource://gre/modules/Extension.jsm");
> +
> +// Import Messenger and BaseContext from ExtensionUtils.
> +Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +
> +const {Messenger, BaseContext} = ExtensionUtils;

Please sort imported symbol names.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:48
(Diff revision 14)
> +   *   The webextension instance associated with this context. This will be the
> +   *   instance of the newly created embedded webextension when this class is
> +   *   used through the EmbeddedWebExtensionsUtils.
> +   * @param {Object} [optionalParams]
> +   *   An object with the following properties:
> +   * @param {string}  [optionalParams.senderURL]

This should just be `url`

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:61
(Diff revision 14)
> +      if (!(targetExtension instanceof Extension)) {
> +        throw new Error("targetExtension is not an Extension class instance");
> +      }
> +    } else {
> +      throw new Error("targetExtension parameter is mandatory");
> +    }

These type checks shouldn't be necessary. This presumably shouldn't be created by code outside this file.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:87
(Diff revision 14)
> +
> +    // This page-load event is handled synchronously by ext-tabs.js
> +    // and will put a getSender method in the delegate object, that will
> +    // be able to resolve the tab object when the connection is originated
> +    // from a tab (e.g. a content script).
> +    Management.emit("page-load", this, {type: this.type}, sender, delegate);

This shouldn't be necessary.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:91
(Diff revision 14)
> +    // from a tab (e.g. a content script).
> +    Management.emit("page-load", this, {type: this.type}, sender, delegate);
> +
> +    // Legacy Extensions (xul overlays, bootstrap restartless and Addon SDK)
> +    // runs with a systemPrincipal.
> +    this.addonPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

There's no need to have separate `principal` and `addonPrincipal` properties.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:96
(Diff revision 14)
> +    this.addonPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> +
> +    this.messenger = new Messenger(this, [Services.mm, Services.ppmm],
> +                                       sender, filter, delegate);
> +
> +    this._cloneScope = Cu.Sandbox(this.addonPrincipal, {});

There's no need for separate `_cloneScope` and `cloneScope` properties.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:148
(Diff revision 14)
> +    return this.addonPrincipal;
> +  }
> +}
> +
> +this.LegacyExtensionsUtils = {
> +  LegacyExtensionContext,

Why do we need to export this? I can't think of an instance where we'd want an external caller to create an instance of this.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:16
(Diff revision 14)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +const {Management} = SpecialPowers.Cu.import("resource://gre/modules/Extension.jsm");

No `SpecialPowers`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:21
(Diff revision 14)
> +const {Management} = SpecialPowers.Cu.import("resource://gre/modules/Extension.jsm");
> +const {
> +  LegacyExtensionsUtils: {
> +    LegacyExtensionContext,
> +  },
> +} = SpecialPowers.Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");

No `SpecialPowers`. And please do this in two steps rather than trying to destructure the return value of `Cu.import`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:40
(Diff revision 14)
> +    let bgURL = window.location.href;
> +
> +    let extensionInfo = {
> +      bgURL,
> +      // Extract the assigned uuid from the background page url.
> +      uuid: bgURL.match("://(.+?)/")[1],

`location.hostname`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:70
(Diff revision 14)
> +      resolve(extension);
> +    };
> +    Management.on("startup", startupListener);
> +  });
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);

Using `ExtensionTestUtils` here doesn't make any sense. If you need an Extension instance, just use `Extension.generate` and skip the management observers and so forth.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:78
(Diff revision 14)
> +  let extensionInstance = yield waitForExtensionInstance;
> +
> +  // Connect to the target extension.id as an external context
> +  // using the given custom sender info.
> +  let legacyContext = new LegacyExtensionContext(extensionInstance, {
> +    sourceContextURL: "about:blank",

What is the point of `sourceContextURL`?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:21
(Diff revision 14)
> +const {Management} = SpecialPowers.Cu.import("resource://gre/modules/Extension.jsm");
> +const {
> +  LegacyExtensionsUtils: {
> +    LegacyExtensionContext,
> +  },
> +} = SpecialPowers.Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");

Same issues here as the other test.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:69
(Diff revision 14)
> +    manifest: {
> +      "content_scripts": [
> +        {
> +          "matches": ["http://example.com/*"],
> +          "js": ["content-script.js"],
> +          "run_at": "document_idle",

This is not necessary.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:86
(Diff revision 14)
> +      resolve(extension);
> +    };
> +    Management.on("startup", startupListener);
> +  });
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);

Same issues as the other test. Please don't use ExtensionTestUtils for this.
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review62986

Incidentally, all of these tests should probably be xpcshell tests (including for the other patch).

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:190
(Diff revision 8)
> +
> +    // Setup the startup promise.
> +    this.startupPromise = new Promise((resolve, reject) => {
> +      this.startupResolve = resolve;
> +      this.startupReject = reject;
> +    });

Please just return a promise from `.startup()` rather than storing this and its resolution functions on the object.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:193
(Diff revision 8)
> +    this.started = false;
> +    this.pendingStartup = false;
> +    this.pendingShutdown = false;

These state flags are too difficult to follow. If we need to keep track of state, please just create one state property that tells us the current state.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:197
(Diff revision 8)
> +    // Setup status flags.
> +    this.started = false;
> +    this.pendingStartup = false;
> +    this.pendingShutdown = false;
> +
> +    this.containerContext = new LegacyExtensionContext(this.embeddedExtension, {

`context`

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:199
(Diff revision 8)
> +    this.pendingStartup = false;
> +    this.pendingShutdown = false;
> +
> +    this.containerContext = new LegacyExtensionContext(this.embeddedExtension, {
> +      senderURL: resourceURI.resolve("/"),
> +      startupPromise: this.startupPromise,

We shouldn't even create a context until the extension has successfully started up, so there should be no need to pass this promise to the context, or expose anything related to it via its API.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:297
(Diff revision 8)
> +  }
> +}
> +
>  this.LegacyExtensionsUtils = {
>    LegacyExtensionContext,
> +  EmbeddedExtension,

I don't think that we should be exporting this directly. We should export a function that loads the embedded extension for an add-on, and returns a promise that resolves when it's ready. It should probably also keep track of active instances, and make sure we don't start duplicates.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:22
(Diff revision 8)
> +  utils: Cu,
> +} = Components;
> +
> +const {
> +  Services,
> +} = Cu.import("resource://gre/modules/Services.jsm");

Please don't access properties from the return value of `Cu.import` except that private globals that we need for testing.
Attachment #8754935 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 90

a year ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/14-15/
Attachment #8746147 - Flags: review?(kmaglione+bmo)
Attachment #8754935 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 91

a year ago
Comment on attachment 8752300 [details]
Bug 1252215 - [webext] add sendMessage support and mochitest to LegacyExtensionContext.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52517/diff/11-12/
(Assignee)

Comment 92

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/8-9/
(Assignee)

Updated

a year ago
Attachment #8749196 - Attachment is obsolete: true
(Assignee)

Comment 93

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/9-10/
(Assignee)

Comment 94

a year ago
https://reviewboard.mozilla.org/r/49277/#review62970

> I don't think we should be exporting `Management` by default, if at all.

We were already discussing about Management 2 months ago as part of a previous review comment (https://reviewboard.mozilla.org/r/49277/#comment67142), I never cleared that issue because I added commented about the reason, which by the way it is specified in an inline comment right above the only usage of Management itself inside the new module, and I was waiting to know it was a blocker or not.

Given that this is still considered a blocker for this patch, I put together a different proposal which I explain in more detail in the issue comment related to the `Management.emit("page-load", ...)` line.

> Please sort imported symbol names.

Both imported symbols removed in the last update.

> This should just be `url`

renamed.

> These type checks shouldn't be necessary. This presumably shouldn't be created by code outside this file.

removed (and dropped the patch that contains the related xpcshell test).

> This shouldn't be necessary.

As discussed over IRC (as described in comment https://reviewboard.mozilla.org/r/49277/#comment67142 and in the inline comment above this line), this is necessary, because we need to emit "page-load" so that "ext-tabs.js" can help us to retrieve the tab data when the messages/ports are coming from a content script.

So, given that it is necessary, but exporting `Management` is still a blocker for this patch, I looked into possible alternative strategies.

In my opinion, the best strategy, one that give us a way to emit "page-load" without using `Management` directly and without breaking anything or start a big refactoring just to achieve it, is to export `ExtensionContext` from the "Extension.jsm" module (instead of `Management`) and to make the change needed to make `LegacyExtensionContext` to inherit much more of the needed behavior (included the above `Management.emit("page-load, ...)`) from the `ExtensionContext`.

> There's no need to have separate `principal` and `addonPrincipal` properties.

done.

> There's no need for separate `_cloneScope` and `cloneScope` properties.

done.

> Why do we need to export this? I can't think of an instance where we'd want an external caller to create an instance of this.

This was exported because it was intended as the component that TestPilot would have used to build the communication channel needed to get telemetry data from a WebExtensions-based add-on. 

I ended up to propose a different strategy that doesn't depend from this module, mostly because the timing to be able to use this module was not compatible with the timing of the first planned TestPilot WebExtensions-based experiment add-ons.
The new strategy is now merged into TestPilot, and so this module doesn't need to be exported anymore.

The only other external direct usage of it is testing, and so in the last change it is not exported by default but the tests extract it from the JSM module to be able to test it.

> No `SpecialPowers`.

done.

> No `SpecialPowers`. And please do this in two steps rather than trying to destructure the return value of `Cu.import`

done

> `location.hostname`

done

> Using `ExtensionTestUtils` here doesn't make any sense. If you need an Extension instance, just use `Extension.generate` and skip the management observers and so forth.

"doesn't make any sense" makes it sounds like that I choose a random jsm module and then I tried to use it for something that it wasn't meant to :-)

and so, assuming that I didn't choose to use `ExtensionTestUtils` by launching a dice ;-), 
let's talk about the reasons:

One of the initial versions of this test was based on `Extension.generate` but then I couldn't use the `browser.test` APIs that are useful (like in all the other tests that we have in the WebExtensions mochitest) to get a different communication channel that is not the one that this test file is testing (which is exactly the reason why we have the `browser.test` messaging API, am I wrong?).

Given that `Management` is now not exported by default from these patches as requested, and that it is now just used for testing reasons as we are already doing for testing purpose in other tests (the XPIProvider tests as an example), and that there are the reasons to actually use ExtensionTestUtils, I have to ask before trying anything:

what is the reason of the request? and how strong you feel about the need of this particular change in these tests?

> What is the point of `sourceContextURL`?

obviously it is the url, after all the "renaming" phases this one has  slipped out.

removed.

> Same issues here as the other test.

some changes applied (and same questions) of the previous related comment.

> This is not necessary.

removed.

> Same issues as the other test. Please don't use ExtensionTestUtils for this.

related question added in one of the above comment (related to the same issue review comment on the other test file).
(Assignee)

Comment 95

a year ago
https://reviewboard.mozilla.org/r/52517/#review51290

> Why not `sendMessage` as well?

This was answered as part of the comment https://bugzilla.mozilla.org/show_bug.cgi?id=1252215#c48
(and I forgot to mention it explicitly here too)
(Assignee)

Comment 96

a year ago
https://reviewboard.mozilla.org/r/54310/#review62986

I'm not sure about this, in the XPIProvider patch there are xpcshell tests for the startup/shutdown/restart part, 
on the contrary these tests are testing that the messaging is working correctly (and that is not currently tested in any of our xpcshell tests).

> Please just return a promise from `.startup()` rather than storing this and its resolution functions on the object.

I disagree, this cannot be done because the container addon needs the API (and the startup promise to chain, if it wants to) before the startup of the embedded extension, the reasons are outlined in the comments that follows.

> These state flags are too difficult to follow. If we need to keep track of state, please just create one state property that tells us the current state.

done. now `this.started` is the only flag defined and used by this class.

> `context`

done.

> We shouldn't even create a context until the extension has successfully started up, so there should be no need to pass this promise to the context, or expose anything related to it via its API.

This is not actually true:
we need the context (and the startup promise) before the embedded extension startup (the EmbeddedExtension instance is created by the XPIProvider just before the addon bootstrap startup method has been called, and destroyed after the shutdown bootstrap method has been called), 
because we need to give the API object to the container addon as part of the bootstrap startup method parameters, because the container addon should be able to subscribe the startupPromise (if it wants to),
and (even more important) subscribe its onMessage/onConnect listeners to be able to receive any message that the background page of the embedded webextension can send to the legacy part during its startup.

Basically when an hybrid addon is started, the XPIProvider's callBootstrapMethod will:

1) create (or get) an instance of this class when it is calling the startup bootstrap method,
2) call the startup bootstrap method with the API object in the the parameters, 
3) startup the embedded webextension, after that the add-on startup bootstrap method has been called.

then, when an hybrid addon is stopped, the XPIProvider's callBootstrapMethod will shutdown the embedded webextension and call the shutdown bootstrap method of the container addon.

> I don't think that we should be exporting this directly. We should export a function that loads the embedded extension for an add-on, and returns a promise that resolves when it's ready. It should probably also keep track of active instances, and make sure we don't start duplicates.

Let's again again then, the updated version offers another alternative.

I have to point out that in any case the XPIProvider doesn't need the promise (at least not as it currently works), and that the container addon needs to receive the API object before the startup of the embedded extension for the reasons pointed out in the above comment.

> Please don't access properties from the return value of `Cu.import` except that private globals that we need for testing.

it wasn't intended. fixed.
(Assignee)

Comment 97

a year ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/15-16/
(Assignee)

Comment 98

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/10-11/
(Assignee)

Updated

a year ago
Attachment #8752300 - Attachment is obsolete: true
(Assignee)

Comment 99

a year ago
https://reviewboard.mozilla.org/r/49277/#review62970

> "doesn't make any sense" makes it sounds like that I choose a random jsm module and then I tried to use it for something that it wasn't meant to :-)
> 
> and so, assuming that I didn't choose to use `ExtensionTestUtils` by launching a dice ;-), 
> let's talk about the reasons:
> 
> One of the initial versions of this test was based on `Extension.generate` but then I couldn't use the `browser.test` APIs that are useful (like in all the other tests that we have in the WebExtensions mochitest) to get a different communication channel that is not the one that this test file is testing (which is exactly the reason why we have the `browser.test` messaging API, am I wrong?).
> 
> Given that `Management` is now not exported by default from these patches as requested, and that it is now just used for testing reasons as we are already doing for testing purpose in other tests (the XPIProvider tests as an example), and that there are the reasons to actually use ExtensionTestUtils, I have to ask before trying anything:
> 
> what is the reason of the request? and how strong you feel about the need of this particular change in these tests?

As discussed over IRC, in the last updated version "no `Management` or `ExtensionTestUtils` were harmed during the making of this patches" :-)

I'll file a follow up to turn as much as possible of these tests to xpcshell tests after Bug 1288885 lands.

> related question added in one of the above comment (related to the same issue review comment on the other test file).

fixed, same as above.
(Assignee)

Comment 100

a year ago
Created attachment 8778405 [details]
proposal: provide all the embedded webextension APIs to the legacy context. f?kmag,aswan

Review commit: https://reviewboard.mozilla.org/r/69712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69712/
(Assignee)

Comment 101

a year ago
Created attachment 8778406 [details]
proposal-followup - minor changes on ProxyContext and always use context.cloneScope as the context sandbox. f?kmag,aswan

Review commit: https://reviewboard.mozilla.org/r/69714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69714/
(Assignee)

Comment 102

a year ago
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49277/diff/16-17/
(Assignee)

Comment 103

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/11-12/
(Assignee)

Comment 104

a year ago
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54310/diff/12-13/
(Assignee)

Comment 105

a year ago
Comment on attachment 8778405 [details]
proposal: provide all the embedded webextension APIs to the legacy context. f?kmag,aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69712/diff/1-2/
(Assignee)

Comment 106

a year ago
Comment on attachment 8778406 [details]
proposal-followup - minor changes on ProxyContext and always use context.cloneScope as the context sandbox. f?kmag,aswan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69714/diff/1-2/
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

r=me with the given changes.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:85
(Diff revisions 14 - 15)
> -
>      super.unload();
> -    Cu.nukeSandbox(this._cloneScope);
> -  }
>  
> -  /**
> +    if (this.cloneScope) {

No need for this check.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:87
(Diff revisions 14 - 15)
> -   */
> -  get cloneScope() {
> -    return this._cloneScope;
> +      Object.defineProperty(
> +        this, "cloneScope",
> +        {value: null, enumerable: true, configurable: true}
> +      );

No need for `defineProperty`. Just assign to it.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:21
(Diff revision 17)
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");

Please keep sorted. This should be at the end of the list.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:82
(Diff revision 17)
> +          onMessage: this.messenger.onMessage("runtime.onMessage"),
> +        },
> +      },
> +    };
> +
> +    this.api.chrome = this.api.browser;

Let's not do this. We have no need for chrome compatibility here.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:94
(Diff revision 17)
> +      Object.defineProperty(
> +        this, "cloneScope",
> +        {value: null, enumerable: true, configurable: true}
> +      );

We can just do `this.cloneScope = null` at this point.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:1
(Diff revision 17)
> +<!DOCTYPE HTML>

This should be an xpcshell test, which would also allow you to use the `ExtensionTestUtils` helper rather than handling everything yourself.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:23
(Diff revision 17)
> +
> +const {
> +  LegacyExtensionContext,
> +} = Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Please keep imports sorted. This should go just after the Extension.jsm import.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:52
(Diff revision 17)
> +
> +    browser.test.sendMessage("webextension-ready", extensionInfo);
> +
> +    browser.test.onMessage.addListener(msg => {
> +      if (msg == "do-send-message") {
> +        browser.runtime.sendMessage("webextension -> legacy_extension message", (reply) => {

`sendMessage("webext...").then(reply => ...)`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:74
(Diff revision 17)
> +  let extensionData = {
> +    background: "new " + backgroundScript,
> +  };
> +
> +  let id = uuidGenerator.generateUUID().number;
> +  let extension = Extension.generate(id, extensionData);

This will conflict with bug 1286908. We just need to not pass the ID.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:95
(Diff revision 17)
> +
> +  // Connect to the target extension.id as an external context
> +  // using the given custom sender info.
> +  let legacyContext = new LegacyExtensionContext(extension);
> +
> +  ok(legacyContext, "Got a LegacyExtensionContext instance");

This doesn't seem necessary.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:100
(Diff revision 17)
> +  ok(
> +    legacyContext.api && legacyContext.api.browser && legacyContext.api.chrome,
> +    "Got the expected API object"
> +  );

Separate tests for each, please. `is(typeof legacyContext.api, "object", ...)`, `is(typeof legacyContext.api.browser, "object", ...)`.

Also, there's no reason to spread this over more than two lines.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:108
(Diff revision 17)
> +  );
> +
> +  let waitMessage = new Promise(resolve => {
> +    const {browser} = legacyContext.api;
> +    browser.runtime.onMessage.addListener((singleMsg, msgSender, sendReply) => {
> +      sendReply("legacy_extension -> webextension reply");

`return Promise.resolve("legacy...");`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:128
(Diff revision 17)
> +  }
> +  // Wait confirmation that the reply has been received.
> +  yield new Promise((resolve) => {
> +    extension.on("test-message", function testMessageListener(kind, msg, ...args) {
> +      if (msg != "got-reply-message") {
> +        return;

We should probably treat this as an error.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context.html:150
(Diff revision 17)
> +  let port = yield waitConnectPort;
> +
> +  ok(port, "Got the Port API object");
> +  ok(port.sender, "The port has a sender property");
> +
> +  if (port.sender) {

No need to check for this. If this doesn't exist, the rest of the test doesn't matter. And if we convert it to an xpcshell test, the failed `ok` will throw anyway.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:23
(Diff revision 17)
> +
> +const {
> +  LegacyExtensionContext,
> +} = Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Again, please keep sorted.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:41
(Diff revision 17)
> +        browser.tabs.create({url: "http://example.com/"})
> +          .then(tab => browser.test.sendMessage("get-expected-sender-info", {
> +            uuid, tab,
> +          }));

Tests that use `browser.tabs` should be in browser/, and should browser rather than chrome mochitests.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:87
(Diff revision 17)
> +  let id = uuidGenerator.generateUUID().number;
> +  let extension = Extension.generate(id, extensionData);

Same issues as other test.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:90
(Diff revision 17)
> +  let waitForExtensionReady = new Promise((resolve) => {
> +    extension.on("test-message", function testMessageListener(kind, msg, ...args) {
> +      if (msg != "ready") {
> +        return;
> +      }
> +
> +      extension.off("test-message", testMessageListener);
> +      resolve();
> +    });
> +  });

This can't be an xpcshell test for now, but I'd still rather use ExtensionTestUtils. We can get the extension instance that we need by just listening for a "startup" event on the management channel.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:107
(Diff revision 17)
> +
> +  // Connect to the target extension.id as an external context
> +  // using the given custom sender info.
> +  let legacyContext = new LegacyExtensionContext(extension);
> +
> +  ok(legacyContext, "Got a LegacyExtensionContext instance");

No need for this.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:149
(Diff revision 17)
> +  let {singleMsg, msgSender} = yield waitMessage;
> +  is(singleMsg, "webextension -> legacy_extension message",
> +     "Got the expected message");
> +  ok(msgSender, "Got a message sender object");
> +
> +  if (msgSender) {

No need for thischeck.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:154
(Diff revision 17)
> +  if (msgSender) {
> +    is(msgSender.id, uuid, "The sender has the expected id property");
> +    is(msgSender.url, "http://example.com/", "The sender has the expected url property");
> +    ok(msgSender.tab, "The sender has a tab property");
> +
> +    if (msgSender.tab) {

No need for this check.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:176
(Diff revision 17)
> +  let port = yield waitConnectPort;
> +
> +  ok(port, "Got the Port API object");
> +  ok(port.sender, "The port has a sender property");
> +
> +  if (port.sender) {

No need.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_context_contentscript.html:181
(Diff revision 17)
> +  if (port.sender) {
> +    is(port.sender.id, uuid, "The port sender has an id property");
> +    is(port.sender.url, "http://example.com/", "The port sender has the expected url property");
> +    ok(port.sender.tab, "The port sender has a tab property");
> +
> +    if (port.sender.tab) {

No need.
Attachment #8746147 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review68872

Mostly down to nits at this point.

::: toolkit/components/extensions/Extension.jsm:1055
(Diff revision 13)
>    }),
>  };
>  
>  // We create one instance of this class per extension. |addonData|
>  // comes directly from bootstrap.js when initializing.
> -this.Extension = function(addonData) {
> +this.Extension = function(addonData, {onBeforeStarted} = {}) {

Destructuring and default values for the same argument is a bit weird. I'd rather `options = {}` and then use the options object below.

Also, I'm sorry, but you're going to have to do a hell of a rebase since bug 1263011 merged.

::: toolkit/components/extensions/Extension.jsm:1061
(Diff revision 13)
>    ExtensionData.call(this, addonData.resourceURI);
>  
> +  // The onBeforeStarted is a callback that is called after the
> +  // extension is initialized (and the manifest fully read)
> +  // but before starting any of the Extension contexts.
> +  this.callOnBeforeStarted = () => {

Is there any particular reason not to just listen for the "startup" event?

::: toolkit/components/extensions/Extension.jsm:1061
(Diff revision 13)
> +  this.callOnBeforeStarted = () => {
> +    if (typeof onBeforeStarted == "function") {
> +      try {
> +        onBeforeStarted();
> +      } catch (e) {
> +        Cu.reportError(e);
> +      }
> +    }
> +  };

This is a bit overkill. If we need an `onBeforeStarted` function, just assign it to a property, and use `if (this.onBeforeStarted)` before the one place you call it.

Also, we have `callSafeSync` to encapsulate this try-catch logic.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:29
(Diff revision 13)
> +XPCOMUtils.defineLazyModuleGetter(this, "AddonManager",
> +                                  "resource://gre/modules/AddonManager.jsm");

Please keep imports sorted.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:138
(Diff revision 13)
> +    // Setup status flag.
> +    this.started = false;
> +  }
> +
> +  /**
> +   * Start the embedded webextension (and report any loading error in the Browser console).

I think that errors reported to the console can be taken as a given.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:140
(Diff revision 13)
> +  }
> +
> +  /**
> +   * Start the embedded webextension (and report any loading error in the Browser console).
> +   *
> +   * @returns {Promise<LegacyContextAPI>} a promise which resolve to the API exposed to the

Please capitalize first word of sentence.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:145
(Diff revision 13)
> +   * @returns {Promise<LegacyContextAPI>} a promise which resolve to the API exposed to the
> +   *                                      legacy context.
> +   */
> +  startup() {
> +    if (this.started) {
> +      return Promise.resolve(this.context.api);

We should probably reject if this is called more than once. Or at least return the stored rejection information if startup failed after the first call.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:150
(Diff revision 13)
> +      this.startupResolve = resolve;
> +      this.startupReject = reject;

I'd rather something like `startupDeferred = {resolve, reject}` if storing these is necessary. But I don't think that it is, given that these are never used outside of this scope.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:203
(Diff revision 13)
> +        let id = this.addonId;
> +
> +        // Adjust the error message to nicely handle both the exception and
> +        // the validation errors scenarios.
> +        let msg;
> +        if (err.errors) {

These will already have been reported to the console.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:209
(Diff revision 13)
> +          msg = JSON.stringify(err.errors, null, 2);
> +        } else {
> +          msg = err.message;
> +        }
> +
> +        let startupError = `Embedded WebExtension startup failed for addonId ${id}: ${msg}`;

No need for the variable. Probably no need to report the error at all, really. If the rejected promise doesn't get handled, the unchecked rejection will be reported automatically.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:230
(Diff revision 13)
> +  shutdown() {
> +    if (!this.extension || this.extension.hasShutdown) {
> +      return Promise.resolve();
> +    }
> +
> +    if (!this.started) {

What if `(this.started && this.startupPromise == undefined)`?

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:237
(Diff revision 13)
> +    // Run shutdown now if the embedded webextension has been started
> +    return new Promise(resolve => {
> +      this.extension.shutdown();
> +      resolve();
> +    });

No need to handle this separately from the above. If there's a `startupPromise`, we can do this from its resolution handler.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:255
(Diff revision 13)
> +    AddonManager.addAddonListener(this);
> +    this.initialized = true;

Please check `this.initialized` here rather than from the caller.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:290
(Diff revision 13)
> +  // AddonManager event listener callbacks
> +
> +  onDisabling(addon) {
> +    // Clean the embedded extension install when the legacy container add-on
> +    // is going to be disabled.
> +    this.cleanupEmbeddedExtensionFor(addon);
> +  },
> +
> +  onUninstalling(addon) {
> +    // Clean the embedded extension install when the legacy container add-on
> +    // is going to be uninstalled.
> +    this.cleanupEmbeddedExtensionFor(addon);
> +  },
> +
> +  onInstalling(addon) {
> +    // Clean the embedded extension install when the legacy container add-on
> +    // is going to be upgraded/reloaded.
> +    this.cleanupEmbeddedExtensionFor(addon);
> +  },

We should just have the add-on manager notify us after it calls the bootstrap shutdown method instead of trying to track all of these. There are definitely corner cases we're missing here.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:312
(Diff revision 13)
> +  getEmbeddedExtensionFor: EmbeddedExtensionManager
> +    .getEmbeddedExtensionFor.bind(EmbeddedExtensionManager),

I think this would be easier to read as

  getEmbeddedExtensionFor(...args) {
    return EmbeddedExtensionManager.getEmbeddedExtensionFor(...args);
  }

The `.bind()` just obfuscates things.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:1
(Diff revision 13)
> +<!DOCTYPE HTML>

This should also be an xpcshell test now.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:19
(Diff revision 13)
> +const {
> +  LegacyExtensionsUtils,
> +} = Cu.import("resource://gre/modules/LegacyExtensionsUtils.jsm");
> +
> +const {
> +  Extension,
> +} = Cu.import("resource://gre/modules/Extension.jsm");

Please use these as regular imports rather than destructuring the return value. And keep sorted.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:56
(Diff revision 13)
> +
> +  // Extensions.generateXPI is used here (and in the other hybrid addons tests in this same
> +  // test dir) to be able to generate an xpi with the directory layout that we expect from
> +  // an hybrid legacy+webextension addon (where all the embedded webextension resources are
> +  // loaded from a 'webextension/' directory).
> +  let xpi = Extension.generateXPI(id, {

Please use `generateZipFile` instead, now that it exists.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:123
(Diff revision 13)
> +
> +  info("Wait for the disconnect port event");
> +  yield waitForDisconnect;
> +  info("Got the disconnect port event");
> +
> +  embeddedExtension.shutdown();

`yield`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:143
(Diff revision 13)
> +  try {
> +    yield embeddedExtension.startup();
> +  } catch (e) {
> +    startupError = e;
> +  }

`yield Assert.rejects(...)`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:169
(Diff revision 13)
> +    info(`Got Validation Errors: ${JSON.stringify(startupError.errors, null, 2)}`);
> +  }
> +
> +  // Shutdown a "never-started" addon with an embedded webextension should not
> +  // raise any exception, and if it does this test will fail.
> +  embeddedExtension.shutdown();

`yield`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:173
(Diff revision 13)
> +  const isExceptionError = true;
> +  const isValidationErrors = true;

This is kind of confusing.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_legacy_extension_embedding.html:210
(Diff revision 13)
> +  for (let {id, files, expected} of testCases) {
> +    let xpi = Extension.generateXPI(id, {files});
> +    yield createManifestErrorTestCase(id, xpi, expected);
> +  }

It would be better to have these as separate tasks that just call `createManifestErrorTestCase` with the right params.
Attachment #8754935 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 109

a year ago
mozreview-review-reply
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

> No need for this check.

We need to check if cloneScope has been already invalidated or Cu.nukeSandbox will raise an exception.
(and as an additional confirmation, this is consistent to the behavior of the `unload` method of the base class, `ExtensionContext`, which protects the instance from executing its unloading logic twice: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#314)

> No need for `defineProperty`. Just assign to it.

Actually, `defineProperty` is needed here, because `cloneScope` is a getter in the base class:

- https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#290

> We can just do `this.cloneScope = null` at this point.

Same as above, `defineProperty` is needed here, because `cloneScope` is a getter in the base class.

> This can't be an xpcshell test for now, but I'd still rather use ExtensionTestUtils. We can get the extension instance that we need by just listening for a "startup" event on the management channel.

This comment confuses me a bit, what happened to the previous review comment related to this topic:

- "Using ExtensionTestUtils here doesn't make any sense. If you need an Extension instance, just use Extension.generate and skip the management observers and so forth." (as in https://reviewboard.mozilla.org/r/49277/#comment81248 )

On the contrary, this new review comment seems to suggest to rewrite this test to how it was before that previous review comment, is this what you are suggesting?

I've not problem to rewrite it, but I want to be sure that it is what the comment is actually suggesting.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8778405 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8778406 - Attachment is obsolete: true
(Assignee)

Comment 112

a year ago
mozreview-review-reply
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

> This should be an xpcshell test, which would also allow you to use the `ExtensionTestUtils` helper rather than handling everything yourself.

This test have been converted to an xpcshell test.

> This will conflict with bug 1286908. We just need to not pass the ID.

Patches rebased and changed accordingly to the changes introduced by Bug 1286908.

> Tests that use `browser.tabs` should be in browser/, and should browser rather than chrome mochitests.

This test has been moved in browser/ and coverted to a browser mochitest.

> This comment confuses me a bit, what happened to the previous review comment related to this topic:
> 
> - "Using ExtensionTestUtils here doesn't make any sense. If you need an Extension instance, just use Extension.generate and skip the management observers and so forth." (as in https://reviewboard.mozilla.org/r/49277/#comment81248 )
> 
> On the contrary, this new review comment seems to suggest to rewrite this test to how it was before that previous review comment, is this what you are suggesting?
> 
> I've not problem to rewrite it, but I want to be sure that it is what the comment is actually suggesting.

This test has been converted into a mochitest browser as suggested in one of the previous comments, and the test has been rewritten to use ExtensionTestUtils and the "startup" event on the mangement channel.
(Assignee)

Comment 113

a year ago
mozreview-review-reply
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review68872

> Destructuring and default values for the same argument is a bit weird. I'd rather `options = {}` and then use the options object below.
> 
> Also, I'm sorry, but you're going to have to do a hell of a rebase since bug 1263011 merged.

In the updated patch the onBeforeStarted is extracted from the options object in the constructor body as suggested.

"Hell of a rebase related to bug 1263011" done ;-)

> Is there any particular reason not to just listen for the "startup" event?

Mainly because we need to run the onBeforeStarted callback synchronously to be sure to reduce further risks of race conditions (and even if the `emit` can be currently synchronous it is better to don't assume that it will always be).

> This is a bit overkill. If we need an `onBeforeStarted` function, just assign it to a property, and use `if (this.onBeforeStarted)` before the one place you call it.
> 
> Also, we have `callSafeSync` to encapsulate this try-catch logic.

Agree, I rewrote it using one of our helpers, and now it is a lighter and nicer change.

> We should just have the add-on manager notify us after it calls the bootstrap shutdown method instead of trying to track all of these. There are definitely corner cases we're missing here.

Sounds good, I opted to export the cleanup helper and defer this to the changes to the XPIProvider itself.

> This should also be an xpcshell test now.

Converted into an xpcshell test.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 117

a year ago
mozreview-review
Comment on attachment 8782972 [details]
Bug 1252215 - [webext] Fix promiseAddonStartup defined in xpcshell tests.

https://reviewboard.mozilla.org/r/72972/#review70720

::: toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:12
(Diff revision 1)
> -    let listener = (extension) => {
> +    let listener = (evt, extension) => {
>        Management.off("startup", listener);
>        resolve(extension);
>      };

I just noticed that the promiseAddonStartup helper defined in the test_ext_experiments xpcshell test is resolving as `extension` the wrong parameter (the first one is actually the event name).

The resolved value is not currently used anywhere in the test case, and so there is no rush to fix it, nevertheless we should fix it before someone is going to try to use the resolved value, and spare to him the time of debugging a known issue.
Comment on attachment 8782972 [details]
Bug 1252215 - [webext] Fix promiseAddonStartup defined in xpcshell tests.

https://reviewboard.mozilla.org/r/72972/#review71472

This should be fixed in the other test files that define this helper too.
Attachment #8782972 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

> We need to check if cloneScope has been already invalidated or Cu.nukeSandbox will raise an exception.
> (and as an additional confirmation, this is consistent to the behavior of the `unload` method of the base class, `ExtensionContext`, which protects the instance from executing its unloading logic twice: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#314)

We should never get here twice. If we want to protect against that, we should either throw or return early (with the former being preferable).

> Actually, `defineProperty` is needed here, because `cloneScope` is a getter in the base class:
> 
> - https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#290

But it's an ordinary value property on this object, since you've already called defineProperty.

> Same as above, `defineProperty` is needed here, because `cloneScope` is a getter in the base class.

But it's not a getter in this instance.
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

> This test has been converted into a mochitest browser as suggested in one of the previous comments, and the test has been rewritten to use ExtensionTestUtils and the "startup" event on the mangement channel.

Sorry, you're right. That was before ExtensionTestUtils existed for xpcshell tests, where the implementation isn't as thorny. It was probably fine to use it here to begin with, so I probably shouldn't have complained, but if we're using it in xpcshell tests, I think we should definitely do the same here.
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review68872

> Mainly because we need to run the onBeforeStarted callback synchronously to be sure to reduce further risks of race conditions (and even if the `emit` can be currently synchronous it is better to don't assume that it will always be).

Management events are meant to be synchronous, and a lot of our code relies on them being so.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 126

a year ago
mozreview-review-reply
Comment on attachment 8746147 [details]
Bug 1252215 - [webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper.

https://reviewboard.mozilla.org/r/49277/#review63234

> We should never get here twice. If we want to protect against that, we should either throw or return early (with the former being preferable).

sounds good, in the updated version the unload method raises an exception when called twice.

> But it's an ordinary value property on this object, since you've already called defineProperty.

yes, it would be writable, but only if it is set to "`writable: true`" where it is defined the first time in the constructor.

from the review comment above it seems that we prefer to make it writable, and so I'm going to change it accordingly.
(Assignee)

Comment 127

a year ago
mozreview-review-reply
Comment on attachment 8782972 [details]
Bug 1252215 - [webext] Fix promiseAddonStartup defined in xpcshell tests.

https://reviewboard.mozilla.org/r/72972/#review70720

> I just noticed that the promiseAddonStartup helper defined in the test_ext_experiments xpcshell test is resolving as `extension` the wrong parameter (the first one is actually the event name).
> 
> The resolved value is not currently used anywhere in the test case, and so there is no rush to fix it, nevertheless we should fix it before someone is going to try to use the resolved value, and spare to him the time of debugging a known issue.

The updated version on this patch introduces the same fix in the other xpcshell test that define a similar helper.
(Assignee)

Comment 128

a year ago
mozreview-review-reply
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review68872

> Management events are meant to be synchronous, and a lot of our code relies on them being so.

In the last update of this patch, I rewrote this using the Management events.

NOTE: to make it work as it is supposed to, I defined a new event (which is obviously called "before-startup"), because the listeners are called in the order of subscription and if we call the `onBeforeStartup` from the startup event, then some of the xpcshell tests in the patch that I wrote above this one fails, the reason is that these tests subscribe the Management startup event before the code in Extension constructor, and then they assume wrongly  that the addon is fully started, but it is not because the `onBeforeStarted` it has not been called yet.

> These will already have been reported to the console.

sounds good, I've removed this error reporting code from the updated version of this patch.

but Extension.jsm currently does a very poor job on reporting them into the console (e.g. if the error object is a collection of validation error the code that should report the error will raises an error because it assumes that the object is an Error instance: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm?q=file%3AExtension.jsm&redirect_type=single#1621).

I've created a separate patch to propose some changes on it (https://reviewboard.mozilla.org/r/73916/diff/1#index_header).

(I'm open to move it into a new bugzilla issue if we prefer to handle it separately)

> No need for the variable. Probably no need to report the error at all, really. If the rejected promise doesn't get handled, the unchecked rejection will be reported automatically.

removed.
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review72098

::: toolkit/components/extensions/Extension.jsm:1252
(Diff revision 16)
> +    // Optional listener called after loading the manifest, but before
> +    // creating any of the defined extension contexts.
> +    if (options.onBeforeStarted) {
> +      let onExtensionStartup = (evt, extension) => {
> +        Management.off("before-startup", onExtensionStartup);
> +        if (extension == this) {
> +          runSafeSyncWithoutClone(options.onBeforeStarted);
> +        }
> +      };
> +      Management.on("before-startup", onExtensionStartup);
> +    }
> +    this.onBeforeStarted = options.onBeforeStarted;

Please just have the caller listen for the startup event rather than passing this callback. It would probably simplify things if we emitted it on the Extension as well as the management channel.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:201
(Diff revision 16)
> +          msg = JSON.stringify(err.errors, null, 2);
> +
> +          // Normalize rejected error type (which should always be an Error instance)
> +          err = new Error(`Manifest Validation Errors ${msg}`);
> +        } else {
> +          msg = err.message;

This isn't used.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:269
(Diff revision 16)
> +  cleanupEmbeddedExtensionFor: (addon) => {
> +    return EmbeddedExtensionManager.cleanupEmbeddedExtensionFor(addon);
> +  },

Why do we need this rather than just having the caller shutdown the extension?

::: toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js:161
(Diff revision 16)
> +for (let {id, files, desc, expectedError} of testCases) {
> +  add_task(function* () {
> +    do_print(`Test "${desc}" Manifest Error`);
> +    let fakeHybridAddonFile = Extension.generateZipFile(files);
> +    yield createManifestErrorTestCase(id, fakeHybridAddonFile, expectedError);
> +  });
> +}

There's no need for a loop. Just call `add_task` once for each testcase, and use a proper function name for each one.
Attachment #8754935 - Flags: review?(kmaglione+bmo)
Comment on attachment 8784470 [details]
Bug 1252215 - [webext] Fix startup and manifest validation errors reporting in Extension.jsm.

https://reviewboard.mozilla.org/r/73916/#review72100

::: toolkit/components/extensions/Extension.jsm:1612
(Diff revision 1)
> -      dump(`Extension error: ${e.message} ${e.filename || e.fileName}:${e.lineNumber} :: ${e.stack || new Error().stack}\n`);
> +      if (e instanceof Error) {
> +        dump(`Extension '${this.id}' error: ${e.message} ${e.filename || e.fileName}:${e.lineNumber} :: ${e.stack || new Error().stack}\n`);
> -      Cu.reportError(e);
> +        Cu.reportError(e);
> +      } else if (e.errors) {
> +        // This is a validation error, create a proper error message to report in the console.
> +        const validationErrors = JSON.stringify(e.errors, null, 2);
> +        const msg = `Extension '${this.id}' Manifest Validation errors: ${validationErrors}`;
> +        dump(msg);
> +        Cu.reportError(msg);
> +      }

Why is this necessary?
Attachment #8784470 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 135

a year ago
mozreview-review-reply
Comment on attachment 8784470 [details]
Bug 1252215 - [webext] Fix startup and manifest validation errors reporting in Extension.jsm.

https://reviewboard.mozilla.org/r/73916/#review72100

> Why is this necessary?

Reporting the validation errors to the console doesn't seem to be necessary (because they are already been reported at this time).

I can be wrong but, still, this Promise chain (at least in my opinion) could use some minor changes, the bare minimum would be:
do not try to report an object with validation errors messages (which is the value of the reject when the manifest contains validation errors that prevents the addon from being fully started) as an instance of Error.

(and in the last push, I have updated this patch to do just the bare minimum described above)

It would be even nice if we always raise instances of Error from here, so that a caller doesn't need to "guess" what kind of object it can receive from the Promise returned by `startup`.

None of this is strictly necessary for the other patches in this queue (but if we change idea of the format of the errors threw from this Promise chain, then we need to tweak the "test_ext_legacy_extension_embedding.js" xpcshell test accordingly.
(Assignee)

Comment 136

a year ago
mozreview-review-reply
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review72098

> Please just have the caller listen for the startup event rather than passing this callback. It would probably simplify things if we emitted it on the Extension as well as the management channel.

Sounds good to me, I've updated this patch to:

- emit the "before-startup" event on both Management and the Extension instance itself
- the caller (in LegacyExtensionsUtils.jsm) subscribes the "before-startup" event on the Extension instance,
  and unsubscribe it once the event has been received or if the extension startup promise has been rejected.

> This isn't used.

That's true, leftover from the previous version.

Removed in the last update.

> Why do we need this rather than just having the caller shutdown the extension?

Given that we are already tracking the created instances inside "LegacyExtensionsUtils.jsm" (to prevent these helpers from creating more then one EmbeddedExtension instance for a defined addonId), it would be nice (at least in my opinion) if we don't have to manually track these instances again in the XPIProvider as well:


```
 if (aAddon.embeddedWebExtension && aMethod == "startup") {
   const embeddedExtension = LegacyExtensionsUtils.getEmbeddedExtensionFor(params);

   // Startup the embedded extension and then, once the embedded extension startup promise has been resolved,
   // call the bootstrap method on the container addon.
   ...
 } else {
   if (aAddon.embeddedWebExtension && aMethod == "shutdown") {
     LegacyExtensionsUtils.cleanupEmbeddedExtensionFor(params);
   }

   // Call the bootstrap method as usual.
   ...
 }
```

In the previous versions of this patches I've already tried a number of different approaching to the shutdown/cleanup of the EmbeddedExtension instances, but none of which seem to match your vision related to this.

may I ask you some additional details about how you prefer these patches to handle the the shutdown/cleanup handling of the EmbeddedExtension instance?

just enough to be sure that I'm going to rewrite it in a way that we can agree on.

> There's no need for a loop. Just call `add_task` once for each testcase, and use a proper function name for each one.

In the updated version this `for` loop has been rewritten into three statically defined test functions as suggested.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 141

a year ago
mozreview-review-reply
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review72098

> Sounds good to me, I've updated this patch to:
> 
> - emit the "before-startup" event on both Management and the Extension instance itself
> - the caller (in LegacyExtensionsUtils.jsm) subscribes the "before-startup" event on the Extension instance,
>   and unsubscribe it once the event has been received or if the extension startup promise has been rejected.

As per brief discussion with Kris, in this last update the "startup" event is emitted on the extension instance before of emitting it on the Management object, and the LegacyExtensionUtils now subscribe "startup" (instead of a new "before-startup" like in the previous version) to be able to run its own startup code at the right time.

> Given that we are already tracking the created instances inside "LegacyExtensionsUtils.jsm" (to prevent these helpers from creating more then one EmbeddedExtension instance for a defined addonId), it would be nice (at least in my opinion) if we don't have to manually track these instances again in the XPIProvider as well:
> 
> 
> ```
>  if (aAddon.embeddedWebExtension && aMethod == "startup") {
>    const embeddedExtension = LegacyExtensionsUtils.getEmbeddedExtensionFor(params);
> 
>    // Startup the embedded extension and then, once the embedded extension startup promise has been resolved,
>    // call the bootstrap method on the container addon.
>    ...
>  } else {
>    if (aAddon.embeddedWebExtension && aMethod == "shutdown") {
>      LegacyExtensionsUtils.cleanupEmbeddedExtensionFor(params);
>    }
> 
>    // Call the bootstrap method as usual.
>    ...
>  }
> ```
> 
> In the previous versions of this patches I've already tried a number of different approaching to the shutdown/cleanup of the EmbeddedExtension instances, but none of which seem to match your vision related to this.
> 
> may I ask you some additional details about how you prefer these patches to handle the the shutdown/cleanup handling of the EmbeddedExtension instance?
> 
> just enough to be sure that I'm going to rewrite it in a way that we can agree on.

As per brief discussion with Kris, in the last updated patch the `cleaupEmbeddedExtensionFor` exported helper has been removed and the caller will call the shutdown method directly on the instance retrieved using the same `getEmbeddedExtensionFor` helper used during the startup of the hybrid addons (and the `EmbeddedExtension` `shutdown` method is now responsible of removing the instance from the Map of the tracked EmbeddedExtension instances).
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review73666

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:153
(Diff revision 18)
> +      this.extension = new Extension({
> +        id: this.addonId,
> +        resourceURI: embeddedExtensionURI,
> +      });
> +
> +            // This callback is register to the "before-startup" event, emitted by the Extension

Nit: Fix indentation and s/before-startup/startup/

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:155
(Diff revision 18)
> +      // but before starting any of the defined
> +      // contexts (which give the legacy part a chance to subscribe
> +      // runtime.onMessage/onConnect listener before the background page has been
> +      // loaded).

Nit: Please fix line wrapping.

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:211
(Diff revision 18)
> +   * Shuts down the embedded webextension.
> +   *
> +   * @returns {Promise<void>} a promise that is resolved when the shutdown has been done
> +   */
> +  shutdown() {
> +    EmbeddedExtensionManager.untrackEmbeddedExtensionFor({id: this.addonId});

We should only do this if this object is the one in the map, so we don't wind up in an inconsistent state if there are races.
Attachment #8754935 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8784470 [details]
Bug 1252215 - [webext] Fix startup and manifest validation errors reporting in Extension.jsm.

https://reviewboard.mozilla.org/r/73916/#review72100

> Reporting the validation errors to the console doesn't seem to be necessary (because they are already been reported at this time).
> 
> I can be wrong but, still, this Promise chain (at least in my opinion) could use some minor changes, the bare minimum would be:
> do not try to report an object with validation errors messages (which is the value of the reject when the manifest contains validation errors that prevents the addon from being fully started) as an instance of Error.
> 
> (and in the last push, I have updated this patch to do just the bare minimum described above)
> 
> It would be even nice if we always raise instances of Error from here, so that a caller doesn't need to "guess" what kind of object it can receive from the Promise returned by `startup`.
> 
> None of this is strictly necessary for the other patches in this queue (but if we change idea of the format of the errors threw from this Promise chain, then we need to tweak the "test_ext_legacy_extension_embedding.js" xpcshell test accordingly.

My real question is, when is this not an Error object? If we're going to change this, we should change it at the place where the rejection is generated, rather than here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 146

a year ago
mozreview-review-reply
Comment on attachment 8754935 [details]
Bug 1252215 - [webext] Add Embedded Extensions helper to LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/54310/#review73666

> Nit: Please fix line wrapping.

my bad, I don't really know how it ended up formatted like this :-(

fixed in the last update.

> We should only do this if this object is the one in the map, so we don't wind up in an inconsistent state if there are races.

Yeah, it would be definitely better to check the instance before remove the item, I've updated this helper to check that the instance is the one that we want to remove before removing it.
(Assignee)

Comment 147

a year ago
mozreview-review
Comment on attachment 8782972 [details]
Bug 1252215 - [webext] Fix promiseAddonStartup defined in xpcshell tests.

https://reviewboard.mozilla.org/r/72972/#review74104

::: toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js:34
(Diff revision 5)
>  function promiseWebExtensionStartup() {
>    return new Promise(resolve => {
> -    let listener = (extension) => {
> +    let listener = (event, extension) => {
>        Management.off("startup", listener);
>        resolve(extension);
>      };
>  
>      Management.on("startup", listener);
>    });
>  }

While I was tweaking the patch attached to Bug 1269342 based on the last review comments, I noticed that there was another one of these test helpers that was resolving the wrong parameter.

Fixed it with the rest of these helpers in the updated version of this patch.
(Assignee)

Comment 148

a year ago
mozreview-review-reply
Comment on attachment 8784470 [details]
Bug 1252215 - [webext] Fix startup and manifest validation errors reporting in Extension.jsm.

https://reviewboard.mozilla.org/r/73916/#review72100

> My real question is, when is this not an Error object? If we're going to change this, we should change it at the place where the rejection is generated, rather than here.

Like the described in the previous comment, it is not an error when the rejection is coming from the validation errors rejected here (just ~15 lines before these checks): http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1504

Sounds good to me to change it where the rejection is generated, but it is probably better to change it in a separate bugzilla issue, is it ok for you if I remove this patch from this patch queue and I file a separate issue for it?
Comment on attachment 8784470 [details]
Bug 1252215 - [webext] Fix startup and manifest validation errors reporting in Extension.jsm.

https://reviewboard.mozilla.org/r/73916/#review72100

> Like the described in the previous comment, it is not an error when the rejection is coming from the validation errors rejected here (just ~15 lines before these checks): http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1504
> 
> Sounds good to me to change it where the rejection is generated, but it is probably better to change it in a separate bugzilla issue, is it ok for you if I remove this patch from this patch queue and I file a separate issue for it?

Fine with me.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8784470 - Attachment is obsolete: true
Attachment #8784470 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 153

a year ago
Last try push (none of the oranges seems to be related to this patches):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=62ab479b5182
Keywords: checkin-needed
(Assignee)

Comment 154

a year ago
In the last update:
the three fully reviewed patches have been rebased on a recent fx-team, and  Attachment 8784470 [details] dropped from this patch queue (because it is going to be attached and discussed separately in a new bugzilla issue).

Comment 155

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/edeae4a294a7
[webext] LegacyExtensionsUtils JSM module and LegacyExtensionContext helper. r=aswan,kmag
https://hg.mozilla.org/integration/autoland/rev/9c1339f30764
[webext] Add Embedded Extensions helper to LegacyExtensionsUtils. r=aswan,kmag
https://hg.mozilla.org/integration/autoland/rev/b57551bae673
[webext] Fix promiseAddonStartup defined in xpcshell tests. r=kmag
Keywords: checkin-needed

Comment 156

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edeae4a294a7
https://hg.mozilla.org/mozilla-central/rev/9c1339f30764
https://hg.mozilla.org/mozilla-central/rev/b57551bae673
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Comment 157

a year ago
Please back out these patches...!

Legacy extensions that use the introduced API will be notified for EVERY extension message from anywhere. This is killing for performance, not to mention a security issue (legacy addons can already do anything, but this change makes it very easy to eavesdrop on extension messages).

A target extension ID should be mandatory. The sendMessge/connect API already support eztensionId, but it is unconditionally overwritten in BaseContext's sendMessage - http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/toolkit/components/extensions/ExtensionUtils.jsm#314

You should first implement onMessageExternal/onConnectExternal before continuing with this bug.

Please get in touch for more implementation details.
Flags: needinfo?(lgreco)

Comment 158

a year ago
Never mind, I misunderstood the meaning of the test.

At first I thought that the WebExtension in the test case is a *different* addon than the legacy extension, but I just learned that it is supposed to be the same addon. So the absence of an explicit extension ID does not mean that the extension broadcasts to a legacy addon, it merely means that the legacy addon is supposed to be a part of the WebExtension.
Flags: needinfo?(lgreco)
Keywords: dev-doc-needed
(Assignee)

Comment 159

a year ago
Created attachment 8789919 [details] [diff] [review]
[rebased to aurora] 1-Bug_1252215____webext__LegacyExtensionsUtils_JSM_module_and_LegacyExtensionContext_helper__r_kmag_aswan.patch

This patch contains the changes from attachment 8746147 [details] rebased and adapted so that it can be applied on aurora.

The only change applied on this patch is related to the included xpcshell test case:

- the extension id is the first parameter of the Extension.generate helper available on aurora
Attachment #8789919 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 160

a year ago
Created attachment 8789920 [details] [diff] [review]
[rebased to aurora] 2-Bug_1252215____webext__Add_Embedded_Extensions_helper_to_LegacyExtensionsUtils__r_kmag_aswan.patch

This patch contains the changes from attachment 8754935 [details] rebased and adapted so that it can be applied on aurora.

The only changes applied on this patch are related to the include xpcshell test cases:

- there is no Extension.generateZipFile helper available on aurora, the test can be adapted by using Extension.generateXPI instead
- the extension id is the first parameter of the Extension.generateXPI helper available on aurora
Attachment #8789920 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 161

a year ago
Created attachment 8789922 [details] [diff] [review]
[rebased on aurora] 3-Bug_1252215___Integrate_changes_to_LegacyExtensionUtils_jsm_into_the_patch_rebased_for_aurora__r_kmag_aswan.patch

This patch contains part of the changes applied by Bug 1298979 on the files landed with these patches.

It seems to be reasonable to apply at least the changes that can be applied without introducing additional risks.

The changes applied by this patch are the following:

- remove the optional url (and the extra parameters) from the LegacyExtensionContext constructor (like the related constructor currently on mozilla-central) and adapt the EmbeddedExtension class accordingly.
- minor tweaks to the test case (so that it is almost equal to the one currently on mozilla-central)
Attachment #8789922 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 162

a year ago
I'm not currently attaching a rebase on aurora of the patch from attachment 8782972 [details].

The main reason is that, besides not being directly related to the goal of this bugzilla issue, it only contains small tweaks to a small number of unrelated test cases and their uplift to aurora would not probably add any real value.

Anyway, I can prepare and attach the patch rebased on aurora if we decide that it is be better to do it (one of those test files doesn't actually exists on aurora, but on the other files the patch can be applied like it is)
Flags: needinfo?(kmaglione+bmo)
Please request uplift first. I'll review if it's approved.

Attachment 8782972 [details] is a test-only change, and should probably be uplifted either way.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 164

a year ago
Created attachment 8790229 [details] [diff] [review]
[rebased to aurora] 4-Bug_1252215____webext__Fix_promiseAddonStartup_defined_in_xpcshell_tests__r_kmag.patch

This patch contains part of the changes applied Attachment 8782972 [details] rebased to be applied to aurora (and it doesn't include only the changes that were applied to test files that don't exist in aurora).
(Assignee)

Comment 165

a year ago
Comment on attachment 8789919 [details] [diff] [review]
[rebased to aurora] 1-Bug_1252215____webext__LegacyExtensionsUtils_JSM_module_and_LegacyExtensionContext_helper__r_kmag_aswan.patch

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
Addon developers of existent legacy addons cannot prepare their addons to transition from the legacy technologies to a webextension.
This patch provides one of the components needed to optionally provide to a legacy add-on a strategy to transition their user to a webextension, by embedding a webextension inside a legacy container addon.
[Describe test coverage new/current, TreeHerder]:
This patch introduces a new JSM module and two new test files to test the features provided by the components defined in the new JSM module.
Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef0c6d5be39
[Risks and why]:
This patch has a low impact and it has a low risk, the provided components could only impact users which have installed add-ons which directly (and explicitly) use the new components provided by the JSM module.
The new LegacyExtensionsUtils.jsm module tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Attachment #8789919 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 166

a year ago
Comment on attachment 8789920 [details] [diff] [review]
[rebased to aurora] 2-Bug_1252215____webext__Add_Embedded_Extensions_helper_to_LegacyExtensionsUtils__r_kmag_aswan.patch

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
Addon developers of existent legacy addons cannot prepare their addons to transition from the legacy technologies to a webextension.
This patch provides one of the components needed to optionally provide to a legacy add-on a strategy to transition their user to a webextension, by embedding a webextension inside a legacy container addon.
[Describe test coverage new/current, TreeHerder]:
This patch introduces in the new JSM module added in attachment 8789919 [details] [diff] [review] the remaining components, and their related
tests.
Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=b98b99dcc1cd
[Risks and why]:
This patch has a low impact and it has a low risk, the provided components could only impact users which have installed add-ons which directly (and explicitly) use the new components provided by the JSM module.
The new LegacyExtensionsUtils.jsm module tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Attachment #8789920 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 167

a year ago
Comment on attachment 8789922 [details] [diff] [review]
[rebased on aurora] 3-Bug_1252215___Integrate_changes_to_LegacyExtensionUtils_jsm_into_the_patch_rebased_for_aurora__r_kmag_aswan.patch

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
None, this patch completes the changes introduced by attachment 8789919 [details] [diff] [review] and attachment 8789920 [details] [diff] [review], on which it applies part of the changes applied by Bug 1298979 on the files landed with these patches.
[Describe test coverage new/current, TreeHerder]:
This patch applies minor changes to the new JSM module added and their related tests.
Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c097569ec32
[Risks and why]:
This patch has a low impact and it has a low risk, the provided components could only impact users which have installed add-ons which directly (and explicitly) use the new components provided by the JSM module.
The new LegacyExtensionsUtils.jsm module tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Attachment #8789922 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 168

a year ago
Comment on attachment 8790229 [details] [diff] [review]
[rebased to aurora] 4-Bug_1252215____webext__Fix_promiseAddonStartup_defined_in_xpcshell_tests__r_kmag.patch

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
None. This patch contains changes applied only on test files.
[Describe test coverage new/current, TreeHerder]:
This patch applies minor changes to fix the resolved value in existent test helpers.
These tests are not currently using the value resolved by these promises, but nonetheless the promise is resolved to the
wrong parameter.
Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c7f4f19ebf5
[Risks and why]:
This patch has a low impact and low risk, because it is a test-only change and these changed tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Attachment #8790229 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 169

a year ago
Comment on attachment 8789922 [details] [diff] [review]
[rebased on aurora] 3-Bug_1252215___Integrate_changes_to_LegacyExtensionUtils_jsm_into_the_patch_rebased_for_aurora__r_kmag_aswan.patch

From the above try build, I noticed that the change in the test file contains a mistake which make this the test to fail consistently when running on a windows 7 VM.
I'm marking obsolete this patch and I'm attaching the new fixed one.
Attachment #8789922 - Attachment is obsolete: true
Attachment #8789922 - Flags: review?(kmaglione+bmo)
Attachment #8789922 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 170

a year ago
Created attachment 8790681 [details] [diff] [review]
[rebased to aurora]  rev2-3-Bug_1252215___Integrate_changes_to_LegacyExtensionUtils_jsm_into_the_patch_rebased_for_aurora__r_kmag_aswan.patch

updated version of the previous attachment 8789922 [details] [diff] [review] and a new try build.

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
None, this patch completes the changes introduced by attachment 8789919 [details] [diff] [review] [diff] [review] and attachment 8789920 [details] [diff] [review] [diff] [review], on which it applies part of the changes applied by Bug 1298979 on the files landed with these patches.
[Describe test coverage new/current, TreeHerder]:
This patch applies minor changes to the new JSM module added and their related tests.
Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d1588c9b905
[Risks and why]:
This patch has a low impact and it has a low risk, the provided components could only impact users which have installed add-ons which directly (and explicitly) use the new components provided by the JSM module.
The new LegacyExtensionsUtils.jsm module tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Attachment #8790681 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 171

a year ago
Comment on attachment 8790229 [details] [diff] [review]
[rebased to aurora] 4-Bug_1252215____webext__Fix_promiseAddonStartup_defined_in_xpcshell_tests__r_kmag.patch

The try build in Comment 168 contains failures due to the obsolete attachment 8789922 [details] [diff] [review], this updated approval request comment contains a new try build which doesn't contain attachment 8789922 [details] [diff] [review] in the queue of the applied patches. 

Approval Request Comment
[Feature/regressing bug #]: 
Bug 1252215
[User impact if declined]:
None. This patch contains changes applied only on test files.
[Describe test coverage new/current, TreeHerder]:
This patch applies minor changes to fix the resolved value in existent test helpers.
These tests are not currently using the value resolved by these promises, but nonetheless the promise is resolved to the
wrong parameter.
[Updated] Try build on aurora:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=d949e992cca5
[Risks and why]:
This patch has a low impact and low risk, because it is a test-only change and these changed tests has been already landed on mozilla-central and they have not shown any intermittent behavior.
[String/UUID change made/needed]:
None
Comment on attachment 8790229 [details] [diff] [review]
[rebased to aurora] 4-Bug_1252215____webext__Fix_promiseAddonStartup_defined_in_xpcshell_tests__r_kmag.patch

Test-only changes are auto-approved and don't need relman review.
Attachment #8790229 - Flags: approval-mozilla-aurora?

Updated

a year ago
status-firefox50: --- → affected
Hi Luca, I was reviewing the changes in the patches and I do see some non-test changes in there. Is there a reason why this cannot ride the 51 train? 50 is a few days away from entering Beta and I would prefer to this one go through an Aurora test cycle before going to Beta.
Flags: needinfo?(lgreco)
(In reply to Ritu Kothari (:ritu) from comment #173)
> Hi Luca, I was reviewing the changes in the patches and I do see some
> non-test changes in there. Is there a reason why this cannot ride the 51
> train? 50 is a few days away from entering Beta and I would prefer to this
> one go through an Aurora test cycle before going to Beta.

A lot of add-on developers are blocked from migrating their legacy extensions to WebExtensions by the lack of support for transitional extensions. The longer it takes for this to be available, the more likely developers are to resort to riskier migration strategies, skip important data migration steps, or put off transitioning to WebExtensions entirely.

Since these patches should have no effect unless extensions explicitly choose to use them, there shouldn't be much risk in skipping most of the Aurora cycle. Any extension which needs to make use of them in Beta is going to need to be specifically developed and tested in the beta channel anyway, so it should be up to their developers to ensure that they work correctly before release.
(Assignee)

Comment 175

a year ago
Clearing the needinfo request, Comment 174 already contains the same informations that I was going to write (thank you Kris for the above recap).
Flags: needinfo?(lgreco)
(In reply to Kris Maglione [:kmag] from comment #174)
> (In reply to Ritu Kothari (:ritu) from comment #173)
> > Hi Luca, I was reviewing the changes in the patches and I do see some
> > non-test changes in there. Is there a reason why this cannot ride the 51
> > train? 50 is a few days away from entering Beta and I would prefer to this
> > one go through an Aurora test cycle before going to Beta.
> 
> A lot of add-on developers are blocked from migrating their legacy
> extensions to WebExtensions by the lack of support for transitional
> extensions. The longer it takes for this to be available, the more likely
> developers are to resort to riskier migration strategies, skip important
> data migration steps, or put off transitioning to WebExtensions entirely.
> 
> Since these patches should have no effect unless extensions explicitly
> choose to use them, there shouldn't be much risk in skipping most of the
> Aurora cycle. Any extension which needs to make use of them in Beta is going
> to need to be specifically developed and tested in the beta channel anyway,
> so it should be up to their developers to ensure that they work correctly
> before release.

Thanks Kris for this detailed justification. I think I better understand the value proposition of getting this out there sooner (in 50 vs 51) and also that the risk of breaking add-ons is non-existent by default unless add-ons explicitly use these new APIs.
Comment on attachment 8789920 [details] [diff] [review]
[rebased to aurora] 2-Bug_1252215____webext__Add_Embedded_Extensions_helper_to_LegacyExtensionsUtils__r_kmag_aswan.patch

Aurora50+
Attachment #8789920 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8789919 [details] [diff] [review]
[rebased to aurora] 1-Bug_1252215____webext__LegacyExtensionsUtils_JSM_module_and_LegacyExtensionContext_helper__r_kmag_aswan.patch

Aurora50+
Attachment #8789919 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8790681 [details] [diff] [review]
[rebased to aurora]  rev2-3-Bug_1252215___Integrate_changes_to_LegacyExtensionUtils_jsm_into_the_patch_rebased_for_aurora__r_kmag_aswan.patch

Aurora50+
Attachment #8790681 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8789919 - Flags: review?(kmaglione+bmo) → review+
Attachment #8789920 - Flags: review?(kmaglione+bmo) → review+

Comment 180

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e9ef4d944309
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfd7e6f5db59
https://hg.mozilla.org/releases/mozilla-aurora/rev/352666cdee72
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3bd6ceb6fc2
status-firefox50: affected → fixed
Flags: in-testsuite+
Covered, I think, in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Embedded_WebExtensions.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.