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.

Comment on attachment 8786125 [details] Bug 1298979 - Decouple ProxyContext from ExtensionContext https://reviewboard.mozilla.org/r/75092/#review73316

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.

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.

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

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.

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

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.

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?

Comment on attachment 8786126 [details] Bug 1298979 - Add ProxyMessenger, change message managers and getSender https://reviewboard.mozilla.org/r/75094/#review74914

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); ```

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?

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"); }); ```

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

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.

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.

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.

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.

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)

Comment on attachment 8788421 [details] Bug 1298979 - Fix LegacyExtensionContext https://reviewboard.mozilla.org/r/76928/#review76050

Comment on attachment 8788420 [details] Bug 1298979 - Test sender equality by contextId https://reviewboard.mozilla.org/r/76926/#review76052

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.

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

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