Rob Wu [:robwu] Assignee
	Description • 8 years ago

MessageChannel and runtime.sendMessage/connect and tabs.sendMessage/connect assume that they can directly talk with other parts of the extensions such as content scripts.

With webext-oop this assumption is false.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8786125 [details]
Bug 1298979 - Decouple ProxyContext from ExtensionContext

https://reviewboard.mozilla.org/r/75092/#review73316

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8786126 [details]
Bug 1298979 - Add ProxyMessenger, change message managers and getSender

https://reviewboard.mozilla.org/r/75094/#review73358

Generally this looks good. I would like to see the port forwarding stuff again before r+.

::: toolkit/components/extensions/Extension.jsm:207
(Diff revision 1)
>  
>      // Properties in |filter| must match those in the |recipient|
>      // parameter of sendMessage.
>      let filter = {extensionId: extension.id};
> -    this.messenger = new Messenger(this, [Services.mm, Services.ppmm], sender, filter, delegate);
> +    // Addon-generated messages (from a content script or from the addon process
> +    // itself) are sent to all frames in an addon process. So we have to listen

This is confusing. Addon-generated messages are sent to the parent process, which forwards them via the parent process message manager. It's clearer to say that.

::: toolkit/components/extensions/Extension.jsm:254
(Diff revision 1)
>        this.extension.views.delete(this);
>      }
>    }
>  };
>  
> +class ProxyListener {

I think it would be cleaner if this class were bidirectional. Let's call it BidirectionalPortForwarder. We'll pass it the port ID and the two message managers. It will take care of the rest. I think it will be cleaner this way.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8786127 [details]
Bug 1298979 - Check whether ProxyContext exists before using it

https://reviewboard.mozilla.org/r/75096/#review73364

::: toolkit/components/extensions/Extension.jsm:520
(Diff revision 1)
>    },
>  
>    createProxyContext(data, target) {
>      let {extensionId, childId, principal} = data;
> +    if (this.proxyContexts.has(childId)) {
> +      Cu.reportError("A context with the given ID already exists!");

These messages are pretty generic. Please use the word WebExtension in each one so it's clearer what is causing them.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 9 • 8 years ago
mozreview-review

Comment on attachment 8786128 [details]
Bug 1298979 - Add test to verify that sending a message and unloading works

https://reviewboard.mozilla.org/r/75098/#review73366

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 10 • 8 years ago
mozreview-review

Comment on attachment 8786129 [details]
Bug 1298979 - move tabs.sendMessage/connect to child process

https://reviewboard.mozilla.org/r/75100/#review73372

::: browser/components/extensions/extensions-browser.manifest:14
(Diff revision 1)
>  category webextension-scripts tabs chrome://browser/content/ext-tabs.js
>  category webextension-scripts utils chrome://browser/content/ext-utils.js
>  category webextension-scripts windows chrome://browser/content/ext-windows.js
>  
> +# scripts that must run in the same process as addon code.
> +category webextension-scripts-addon tabs chrome://browser/content/ext-c-tabs.js

Could we call this ext-a-tabs.js? I guess it's confusing because ext-c-* stuff can be loaded into an add-on process. But I still think it would be nice to distinguish stuff that only loads in addon processes from stuff that can be loaded into content or addon processes.

	Rob Wu [:robwu] Assignee
	Comment 18 • 8 years ago

I've rebased the Port implementation on MessageChannel because that reduces the complexity in proxying, and because of an issue with <browser>.messageManager being undefined.

(In reply to Bill McCloskey (:billm) from comment #10)code.
> > +category webextension-scripts-addon tabs chrome://browser/content/ext-c-tabs.js
> 
> Could we call this ext-a-tabs.js? I guess it's confusing because ext-c-*
> stuff can be loaded into an add-on process. But I still think it would be
> nice to distinguish stuff that only loads in addon processes from stuff that
> can be loaded into content or addon processes.

The "c" in ext-c-*.js used to stand for content, but now it is "child". To distinguish addon code from content code, just check whether the file contains a registerSchemaAPI call with the right envType - see the docs that I added last week at https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process#Execution_environments

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 19 • 8 years ago
mozreview-review

Comment on attachment 8786643 [details]
Bug 1298979 - Properly dispose EventManager

https://reviewboard.mozilla.org/r/75578/#review73698

::: toolkit/components/extensions/ExtensionUtils.jsm:961
(Diff revision 1)
>  
>    close() {
>      if (this.callbacks.size) {
>        this.unregister();
>      }
> -    this.callbacks = Object.freeze([]);
> +    this.callbacks = null;

The purpose of the Object.freeze([]) change was to ensure that if fire is called after close, nothing will happen. (I guess additionally we want to make sure we throw if you call addListener after close.) Your change will make fire throw after close since we can't iterate over null (I think--JS can be weird sometimes).

I suggest setting callbacks to an empty set. If we call addListener after close, then we'll throw because register is null. So I think that guarantees what you need.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 27 • 8 years ago
mozreview-review

Comment on attachment 8786644 [details]
Bug 1298979 - Use MessageChannel to implement runtime.Port

https://reviewboard.mozilla.org/r/75580/#review74908

::: toolkit/components/extensions/ExtensionUtils.jsm:1164
(Diff revision 2)
> + * @param {object} recipient The recipient of messages sent from this port.
> + */
> +function Port(context, senderMM, receiverMM, name, id, sender, recipient) {
>    this.context = context;
> -  this.messageManager = messageManager;
> +  this.senderMM = senderMM;
> +  this.receiverMM = receiverMM;

This should be receiverMMs.

::: toolkit/components/extensions/ExtensionUtils.jsm:1187
(Diff revision 2)
> +  };
> +
> +  this.disconnectHandler = Object.assign({
> +    receiveMessage: () => this.disconnectByOtherEnd(),
> +  }, this.handlerBase);
> +  MessageChannel.addListener(this.receiverMM, "Extension:Port.disconnect", this.disconnectHandler);

Please use Extension:Port:Disconnect.

::: toolkit/components/extensions/ExtensionUtils.jsm:1205
(Diff revision 2)
>        postMessage: json => {
>          if (this.disconnected) {
> -          throw new this.context.contentWindow.Error("Attempt to postMessage on disconnected port");
> +          throw new this.context.cloneScope.Error("Attempt to postMessage on disconnected port");
>          }
> -        this.messageManager.sendAsyncMessage(this.listenerName, json);
> +
> +        this._sendMessage("Extension:Port.postMessage", json);

Extension:Port:PostMessage.

::: toolkit/components/extensions/MessageChannel.jsm:517
(Diff revision 2)
>  
>      let channelId = `${gChannelId++}-${Services.appinfo.uniqueProcessID}`;
>      let message = {messageName, channelId, sender, recipient, data, responseType};
>  
> +    if (responseType == this.RESPONSE_NONE) {
> +      target.sendAsyncMessage(MESSAGE_MESSAGE, message);

Should this be wrapped in try/catch like the one below?

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 28 • 8 years ago
mozreview-review

Comment on attachment 8786126 [details]
Bug 1298979 - Add ProxyMessenger, change message managers and getSender

https://reviewboard.mozilla.org/r/75094/#review74914

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 44 • 8 years ago
mozreview-review

Comment on attachment 8788420 [details]
Bug 1298979 - Test sender equality by contextId

https://reviewboard.mozilla.org/r/76926/#review75078

::: toolkit/components/extensions/ExtensionUtils.jsm:1318
(Diff revision 1)
>      return new SingletonEventManager(this.context, name, callback => {
>        let listener = {
>          messageFilterPermissive: this.filter,
>  
>          filterMessage: (sender, recipient) => {
> -          // Ignore the message if it was sent by this Messenger.
> +          // Ignore the port if it was created by this Messenger.

This 'inline comment' has been probably changed by mistake (this filter still ignores messages and not ports)

::: toolkit/components/extensions/ExtensionUtils.jsm:1379
(Diff revision 1)
>        let listener = {
>          messageFilterPermissive: this.filter,
>  
>          filterMessage: (sender, recipient) => {
>            // Ignore the port if it was created by this Messenger.
> -          return !MessageChannel.matchesFilter(this.sender, sender);
> +          return sender.contextId !== this.context.contextId;

I briefly looked into the changes that this set of patches are going to introduce, how they impact on the legacy extension context, and the proposal fixes to the issues with the legacy context.

One thing that I'm wondering is:
can we keep the previous version of the filter (`return !MessageChannel.matchesFilter(...)`) instead, and add the `contextId` as a field in the `sender` object in the `LegacyExtensionContext` to make the filter to behave correctly with the legacy contexts?

e.g. in the "LegacyExtensionsUtils.jsm":

```
var LegacyExtensionContext = class extends BaseContext {
  ...
  constructor(targetExtension) {
    ...
    let sender = {id: targetExtension.uuid, contextId: this.contextId};
    let filter = {extensionId: targetExtension.id};
    // Legacy addons live in the main process. Messages from other addons are
    // Messages from WebExtensions are sent to the main process and forwarded via
    // the parent process manager to the legacy extension.
    this.messenger = new Messenger(this, [Services.cpmm], sender, filter);

```

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 45 • 8 years ago
mozreview-review

Comment on attachment 8788421 [details]
Bug 1298979 - Fix LegacyExtensionContext

https://reviewboard.mozilla.org/r/76928/#review75080

::: browser/components/extensions/test/browser/browser_ext_legacy_extension_context_contentscript.js:107
(Diff revision 1)
>        sendReply("legacy_extension -> webextension reply");
>        resolve({singleMsg, msgSender});
>      });
>    });
>  
> -  is(legacyContext.type, "legacy_extension",
> +  is(legacyContext.envType, "legacy_extension",

how about using the string "legacy_extension" as both the `type` and `envType` of the legacy context instances?

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 46 • 8 years ago
mozreview-review-reply

Comment on attachment 8788421 [details]
Bug 1298979 - Fix LegacyExtensionContext

https://reviewboard.mozilla.org/r/76928/#review75080

There is also another test (the test file " toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js") that needs to be fixed before it can complete correctly with the patches attached to this issue applied.

I've currently found three issues that seem to need further changes in this test:

- the assertion that check the context type should be changed to assert the envType instead (or we can set the type in the legacy context)

- when the extension is unloaded (and all the patches from this issue are applied changes applies), the port disconnect event handler is sent from the legacy context and it is never received by the handler registered by the waitForDisconnect promise (on the contrary if we call `port.disconnect` explicitly from the background page, the disconnect
event is received by the legacy context (as it should)   

- `legacyContext.shutdown` should now be changed to `legacyContext.unload` (and to better match what really happen when this component is used internally by the EmbeddedExtension component, the `unload` method should be called when the embedded extension instance is closing (by registering an "onClose" handler to the extension with `extension.callOnClose`)

I've applied this patch locally and with the following changes applied, this test is passing:

```
diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js b/toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js
--- a/toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js
@@ -24,31 +24,35 @@ add_task(function* test_legacy_extension
     let extensionInfo = {
       bgURL,
       // Extract the assigned uuid from the background page url.
       uuid: window.location.hostname,
     };

     browser.test.sendMessage("webextension-ready", extensionInfo);

+    let port;
+
     browser.test.onMessage.addListener(msg => {
       if (msg == "do-send-message") {
         browser.runtime.sendMessage("webextension -> legacy_extension message").then(reply => {
           browser.test.assertEq("legacy_extension -> webextension reply", reply,
                                 "Got the expected message from the LegacyExtensionContext");
           browser.test.sendMessage("got-reply-message");
         });
       } else if (msg == "do-connect") {
-        let port = browser.runtime.connect();
+        port = browser.runtime.connect();

         port.onMessage.addListener(msg => {
           browser.test.assertEq("legacy_extension -> webextension port message", msg,
                                 "Got the expected message from the LegacyExtensionContext");
           port.postMessage("webextension -> legacy_extension port message");
         });
+      } else if (msg == "do-disconnect") {
+        port.disconnect();
       }
     });
   }

   let extensionData = {
     background: "new " + backgroundScript,
   };

@@ -60,25 +64,33 @@ add_task(function* test_legacy_extension
         reject(new Error(`Got an unexpected test-message: ${msg}`));
       } else {
         extension.off("test-message", testMessageListener);
         resolve(args[0]);
       }
     });
   });

+  // Connect to the target extension.id as an external context
+  // using the given custom sender info.
+  let legacyContext;
+
+  extension.on('startup', function onStartup() {
+    extension.off('startup', onStartup);
+    legacyContext = new LegacyExtensionContext(extension);
+    extension.callOnClose({
+      close: () => legacyContext.unload(),
+    });
+  });
+
   yield extension.startup();

   let extensionInfo = yield waitForExtensionInfo;

-  // Connect to the target extension.id as an external context
-  // using the given custom sender info.
-  let legacyContext = new LegacyExtensionContext(extension);
-
-  equal(legacyContext.type, "legacy_extension",
+  equal(legacyContext.envType, "legacy_extension",
      "LegacyExtensionContext instance has the expected type");

   ok(legacyContext.api, "Got the expected API object");
   ok(legacyContext.api.browser, "Got the expected browser property");

   let waitMessage = new Promise(resolve => {
     const {browser} = legacyContext.api;
     browser.runtime.onMessage.addListener((singleMsg, msgSender) => {
@@ -141,16 +153,16 @@ add_task(function* test_legacy_extension

   equal(msg, "webextension -> legacy_extension port message",
      "LegacyExtensionContext received the expected message from the webextension");

   let waitForDisconnect = new Promise(resolve => {
     port.onDisconnect.addListener(resolve);
   });

-  extension.shutdown();
+  extension.testMessage("do-disconnect");

   yield waitForDisconnect;

-  do_print("Got the disconnect event on unload");
+  extension.shutdown();

-  legacyContext.shutdown();
+  do_print("Got the disconnect event on unload");
 });
```

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 47 • 8 years ago

Here is a try build with the changes described in Comment 44 and Comment 46 applied on top of the patches attached to this bugzilla issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3196bb92e4dc

	Rob Wu [:robwu] Assignee
	Comment 48 • 8 years ago
mozreview-review

Comment on attachment 8788420 [details]
Bug 1298979 - Test sender equality by contextId

https://reviewboard.mozilla.org/r/76926/#review75196

::: toolkit/components/extensions/ExtensionUtils.jsm:1379
(Diff revision 1)
>        let listener = {
>          messageFilterPermissive: this.filter,
>  
>          filterMessage: (sender, recipient) => {
>            // Ignore the port if it was created by this Messenger.
> -          return !MessageChannel.matchesFilter(this.sender, sender);
> +          return sender.contextId !== this.context.contextId;

The `contextId` is automatically assigned to the sender by `BaseContext`'s common `sendMessage` method - http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/components/extensions/ExtensionUtils.jsm#318

The issue is briefly summarized in the commit message. Note that in the past, contextId was not unique across processes, so back then a full comparison was needed. Now contextId is sufficiently unique so there is no need for checking the other parameters.

	Rob Wu [:robwu] Assignee
	Comment 49 • 8 years ago
mozreview-review-reply

Comment on attachment 8788421 [details]
Bug 1298979 - Fix LegacyExtensionContext

https://reviewboard.mozilla.org/r/76928/#review75080

> how about using the string "legacy_extension" as both the `type` and `envType` of the legacy context instances?

In `ExtensionContext` the `type` value is mainly used by `extension.getViews` and others to identify specific view types, here in `legacy_extension` there is no need for it.

Note that content scripts also have a "type" "content_script", which is also unused. In another patchset, I've removed the content_script type and renamed type to viewType to be more explicit.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 58 • 8 years ago
mozreview-review-reply

Comment on attachment 8788420 [details]
Bug 1298979 - Test sender equality by contextId

https://reviewboard.mozilla.org/r/76926/#review75196

> The `contextId` is automatically assigned to the sender by `BaseContext`'s common `sendMessage` method - http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/components/extensions/ExtensionUtils.jsm#318
> 
> The issue is briefly summarized in the commit message. Note that in the past, contextId was not unique across processes, so back then a full comparison was needed. Now contextId is sufficiently unique so there is no need for checking the other parameters.

Great, thanks for these additional background info.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 59 • 8 years ago

Comment on attachment 8788420 [details]
Bug 1298979 - Test sender equality by contextId

From a "LegacyExtensionContext" point of view, this patch looks good to me, I'm setting the f+ flag.

I'm also changing the r? flag and set it to Bill, he has followed your work of this patches in much more details and can give it you the final sign off.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 60 • 8 years ago
mozreview-review

Comment on attachment 8788421 [details]
Bug 1298979 - Fix LegacyExtensionContext

https://reviewboard.mozilla.org/r/76928/#review75418

From a "LegacyExtensionContext" point of view, this patch looks great.

f+ (with just a small number of nits that would be nice to fix)

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:23
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "ExtensionContext",
>                                    "resource://gre/modules/Extension.jsm");

`ExtensionContext` seems to be unused now the `LegacyExtensionContext` extends `BaseContext` (and it is not exported anymore from the Extension.jsm module)

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:25
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
>                                    "resource://gre/modules/NetUtil.jsm");

`NetUtil` seems to be unused with the applied changes.

::: toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js:72
(Diff revision 2)
>          resolve(args[0]);
>        }
>      });
>    });
>  
> +  // Connect to the target extension.id as an external context

Nit, I forgot to change "extension.id" into "extension" in this inline comment (in the patch that landed this test).

::: toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_context.js:167
(Diff revision 2)
>  
>    yield waitForDisconnect;
>  
> -  do_print("Got the disconnect event on unload");
> +  extension.shutdown();
>  
> -  legacyContext.shutdown();
> +  do_print("Got the disconnect event on unload");

Nit, this printed log line should be probably moved to line 164 (my apologies, I'm pretty sure that it is coming from my patch attached in one of the above comments)

	Andrew Swan [:aswan]
	Comment 65 • 8 years ago
mozreview-review

Comment on attachment 8788421 [details]
Bug 1298979 - Fix LegacyExtensionContext

https://reviewboard.mozilla.org/r/76928/#review76050

	Kris Maglione [:kmag]
	Comment 66 • 8 years ago
mozreview-review

Comment on attachment 8788420 [details]
Bug 1298979 - Test sender equality by contextId

https://reviewboard.mozilla.org/r/76926/#review76052

	Rob Wu [:robwu] Assignee
	Comment 67 • 8 years ago

Ready to land :)
Try from two days ago looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2418dcd49a4
... but just in case I send another try request.

	Pulsebot
	Comment 68 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1de65e9d7ed3
Properly dispose EventManager r=billm
https://hg.mozilla.org/integration/autoland/rev/d074142f6055
Test sender equality by contextId r=kmag
https://hg.mozilla.org/integration/autoland/rev/f99427fa29f0
Use MessageChannel to implement runtime.Port r=billm
https://hg.mozilla.org/integration/autoland/rev/c476c4fbe3a3
Decouple ProxyContext from ExtensionContext r=billm
https://hg.mozilla.org/integration/autoland/rev/329102a4c682
Add ProxyMessenger, change message managers and getSender r=billm
https://hg.mozilla.org/integration/autoland/rev/6d7fe1431644
Fix LegacyExtensionContext r=aswan
https://hg.mozilla.org/integration/autoland/rev/f9f94cf30f53
Check whether ProxyContext exists before using it r=billm
https://hg.mozilla.org/integration/autoland/rev/2fa53669115d
Add test to verify that sending a message and unloading works r=billm
https://hg.mozilla.org/integration/autoland/rev/76ded056a9de
move tabs.sendMessage/connect to child process r=billm

	Carsten Book [:Tomcat]
	Comment 69 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1de65e9d7ed3
https://hg.mozilla.org/mozilla-central/rev/d074142f6055
https://hg.mozilla.org/mozilla-central/rev/f99427fa29f0
https://hg.mozilla.org/mozilla-central/rev/c476c4fbe3a3
https://hg.mozilla.org/mozilla-central/rev/329102a4c682
https://hg.mozilla.org/mozilla-central/rev/6d7fe1431644
https://hg.mozilla.org/mozilla-central/rev/f9f94cf30f53
https://hg.mozilla.org/mozilla-central/rev/2fa53669115d
https://hg.mozilla.org/mozilla-central/rev/76ded056a9de