+++ This bug was initially created as a clone of Bug #1208257 +++ The functions accept a callback argument, but it's never actually called.

I'd love to work on this bug if I could get some more information in terms of what needs to happen here. Thanks!

Essentially, when an extension calls `executeScript` or `insertCSS`, we need to send a message to the content process to execute that script. After the message has been received by the content process, we need to execute any callback passed by the extension code. The executeScript APIs are here: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#466 And the message is processed by the content script here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#554 The tricky part is that we need to store a reference to the callback when `executeScript` is called, and execute it only when we've received the right message from the content process. I can provide some suggestions on how to do that, if you'd like.

I'm planning to eventually extend this to support to-way messaging across an open channel, hence the module name. I'm also trying to keep it as generic as possible, so I can hopefully make it available for use in the rest of the tree once it matures. This overlaps a lot with the Broker/Messenger/Port functionality. It might be nice to merge some more of that functionality into this interface at some point, but I'm not sure. In the mean time, we should probably just update it to use this class to handle the one-way reply handling logic that it implements in a few places. Review commit: https://reviewboard.mozilla.org/r/31529/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31529/

Review commit: https://reviewboard.mozilla.org/r/31531/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31531/

Hi Kris, I'm having some trouble understanding what's going on here. One thing I'm worried about is that it seems like all the messages end up with MessageChannel:Message as the message name. Is there a reason you can't use something more specific? Also, can you explain what the different advantages of this class are over a normal message manager? It seems like it supports filtering and replies. Is there anything I'm missing? It looks like MessageBroker is a wrapper around a message manager that does filtering. Could you name it FilteringMessageManager or something like that? (Sorry, I realize the broker name is my fault.) I also wonder if it would be easier to make message managers wrapper cached than to have to implement this BrokerManager thing. Then we could just have a global weakmap of FilteringMessageManagers in MessageChannel.jsm, which seems easier.

Olli, how hard would it be to make message managers wrapper cached so that we can put them in weakmaps? It might make some stuff in this bug somewhat easier.

(In reply to Bill McCloskey (:billm) from comment #5) > Hi Kris, I'm having some trouble understanding what's going on here. One > thing I'm worried about is that it seems like all the messages end up with > MessageChannel:Message as the message name. Is there a reason you can't use > something more specific? I considered using a separate message listener for every message name for a while. I decided against it because it would have added a lot of complexity to have to manage a separate listener for each message rather than one listener per manager, and didn't seem to have a lot of benefit. > Also, can you explain what the different advantages of this class are over a > normal message manager? It seems like it supports filtering and replies. Is > there anything I'm missing? The main purpose is to route replies back to the sender, and to do it in a way that makes sure replies are either answered, or rejected, without making it too easy to leak. The filtering is there mostly so the broker can make sure that each message has exactly one recipient and one reply, without making it too difficult for multiple extensions to listen for the same message on the same message manager. I could make a good argument for taking it out, though. > It looks like MessageBroker is a wrapper around a message manager that does > filtering. Could you name it FilteringMessageManager or something like that? > (Sorry, I realize the broker name is my fault.) I wasn't too happy with the name either, in the end. I guess FilteringMessageManager works. > I also wonder if it would be easier to make message managers wrapper cached > than to have to implement this BrokerManager thing. Then we could just have > a global weakmap of FilteringMessageManagers in MessageChannel.jsm, which > seems easier. I was thinking about that too. I'm not sure how much it would simplify things, in this case, since the main purpose of the message-manager-disconnect observer is to cancel pending replies. A WeakMap would remove the need for the unload listener and some chances of leaks, though.

crap, midair collision.... ok what did I write. I thought first wrappercache case would be bug 888600, but actually it should be just a small subset, and it applies to parent side only since on child side mm are being another object which is already wrappercache (because of inheriting DOMEventTargetHelper). So, extend nsWrapperCache, add the relevant stuff to traverse and unlink and modify nsDOMClassInfo.* so that wrappercaching is actually enabled when expando properties and such are added. See EventTargetSH there.

(In reply to Kris Maglione [:kmag] from comment #7) > (In reply to Bill McCloskey (:billm) from comment #5) > > Hi Kris, I'm having some trouble understanding what's going on here. One > > thing I'm worried about is that it seems like all the messages end up with > > MessageChannel:Message as the message name. Is there a reason you can't use > > something more specific? > > I considered using a separate message listener for every message name for a > while. I decided against it because it would have added a lot of complexity > to have to manage a separate listener for each message rather than one > listener per manager, and didn't seem to have a lot of benefit. I suppose why it would make it more complicated isn't immediately obvious, though: * If I send a message on a message manager, I want to know if it failed to reach the other side. With a single message name for all listeners, as long as there's any listener on the other side of the message manager, I'll get a reply. If I'm expecting a content script context to have added a listener for a window, and there's a race, I'll at least know not to expect a reply. * When there are multiple listeners on the same message, there needs to be exactly one reply. Or, at least, adding multiple listeners shouldn't result in multiple listeners being called and some of their replies being lost. With multiple message names, that still means two levels of maps, with the added complexity of adding and removing more listeners, and the added risk of messages failing to find a recipient. * The same problems apply to managing replies. With multiple message names, unexpected extra replies are silently ignored. We also need to separately keep track of pending replies so they can be canceled when a message manager dies, or a context is closed. Sharing the multiplexing code between the requests and replies makes this easier, and multiplexing on one message name makes things easier for both.

IIRC, putting things in weakmaps requires full WebIDLification.

Sorry this is taking so long Kris. I didn't realize how big a patch it was when I said I could review it by yesterday. I should have more time next week once the merge happens. Andrew, are you sure about the WebIDL restriction? The code seems to try to work for wrapped natives: https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/WeakMapObject.cpp#114

(In reply to Bill McCloskey (:billm) from comment #11) > Sorry this is taking so long Kris. I didn't realize how big a patch it was > when I said I could review it by yesterday. I should have more time next > week once the merge happens. That's fine. Given how long it took me to write it, I really shouldn't have expected you to review it that quickly. > Andrew, are you sure about the WebIDL restriction? The code seems to try to > work for wrapped natives: > https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/WeakMapObject.cpp#114 I wound up looking into this yesterday. I initially thought it should work on other types of wrapped native too, but the preserveWrapperCallback we use only handles WebIDL objects. I think we may have other options, though. We could always store the message manager owner rather than the message manager itself. For message managers without owners, we could create a stub owner object and store it in a symbol property on the message manager. I'm pretty sure that as long as the message manager has any listeners that are holding a reference to the JS reflector alive, that should keep both the expando and the weak map key alive. Someone can correct me if I'm wrong. That's not as nice as being able to use the message manager as a key directly, but it might be nicer than having to use a Map.

As Kris said, the question is what happens here in the Gecko callback: https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#3197 We used to have a hacked up bit of code in there for nsINode, before they were converted to WebIDL. Something similar could probably be special-cased in for message managers if needed.

This is what the code used to be: https://hg.mozilla.org/mozilla-central/annotate/945ed4328628/js/xpconnect/src/XPCJSRuntime.cpp#l2296

Comment on attachment 8709736 [details] MozReview Request: Bug 1210583: Part 1 - [webext] Add support for cross-process messaging with async responses. r?billm https://reviewboard.mozilla.org/r/31529/#review28417 In my gut I feel like this is maybe a little overkill. I think we would end up with the best design if we kept the sendMessage/Port code separate from this. It needs extra complexity that this code doesn't. For example, it looks like it would still be a good amount of work to handle multiple browser.runtime.onMessage listeners but ensure that only one of them actually sends a reply. That said, it's hard to design something like this when we don't know quite how it's going to be used. I'm fine landing this and seeing how it goes. Once Luca's bug and bug 1197346 land, we'll hopefully have a better picture of what we need here. ::: browser/components/extensions/ext-tabs.js:487 (Diff revision 1) > + MessageChannel.sendMessage(mm, "Extension:Execute", { options }, recipient); I think b2g might break if you don't import MessageChannel in this file. Even if it doesn't, it would be nice to do. ::: toolkit/components/extensions/ExtensionUtils.jsm:789 (Diff revision 1) > + MessageChannel, This is a little unconventional. I guess it's more uniform this way, but I think I'd still rather do direct Cu.import from each module that uses MessageChannel. ::: toolkit/components/extensions/MessageChannel.jsm:7 (Diff revision 1) > +this.EXPORTED_SYMBOLS = ["MessageChannel"]; Please add a big comment at the top of the file with an example of how to use MessageChannel from the perspective of the client. The inline comments are less useful because it's not clear which ones are meant for someone changing this file and which ones are useful for someone using MessageChannel. ::: toolkit/components/extensions/MessageChannel.jsm:32 (Diff revision 1) > +class MessageBroker { Please do rename this to FilteringMessageManager. ::: toolkit/components/extensions/MessageChannel.jsm:33 (Diff revision 1) > + /** The indentation of this comment seems wrong. ::: toolkit/components/extensions/MessageChannel.jsm:92 (Diff revision 1) > + let handlers = this.handlers.get(messageName) || []; Would be clearer to say "|| new Set()". ::: toolkit/components/extensions/MessageChannel.jsm:152 (Diff revision 1) > +class BrokerManager extends Map { FilteringMessageManagerMap? ::: toolkit/components/extensions/MessageChannel.jsm:438 (Diff revision 1) > + abortResponses(sender, reason = this.REASON_DISCONNECTED) { For abstraction reasons, I think it would be nicer if this delegated to FilteringMessageManager. ::: toolkit/components/extensions/MessageChannel.jsm:439 (Diff revision 1) > + for (let broker of this.responseBrokers.values()) { I guess this makes it hard to use a WeakMap. I don't really see a way around that. ::: toolkit/components/extensions/MessageChannel.jsm:459 (Diff revision 1) > + abortBroker(target, reason) { Same here about delegating to FMM.

Comment on attachment 8709737 [details] MozReview Request: Bug 1210583: Part 2 - [webext] Support callbacks in tabs.executeScript/tabs.insertCSS. r=billm https://reviewboard.mozilla.org/r/31531/#review28421 ::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js:25 (Diff revision 1) > + browser.test.assertEq(undefined, result, "Expected callback result"); My understanding is that Chrome returns 27 here. Maybe I'm wrong? ::: toolkit/components/extensions/Extension.jsm:284 (Diff revision 1) > + sender.pageId = this.contextId; Why pageId and not contextId? ::: toolkit/components/extensions/Extension.jsm:315 (Diff revision 1) > + MessageChannel.abortResponses({ extensionId: this.extension.id, Generally I think it's nicer to do: functionCall({ prop1: val1, prop2: val2, }); since you get less indentation. ::: toolkit/components/extensions/ExtensionContent.jsm:156 (Diff revision 1) > + Why the extra line? ::: toolkit/components/extensions/ExtensionContent.jsm:190 (Diff revision 1) > runSafeSyncWithoutClone(Services.scriptloader.loadSubScriptWithOptions, url, options); Shouldn't we catch errors here too? Also, I think Chrome treats the result of the last statement (if it's an expression) as the result in this case. We would need platform support for that. Could you please file a bug? ::: toolkit/components/extensions/ExtensionContent.jsm:198 (Diff revision 1) > + this.deferred.reject(e.message); Return here before also resolving the promise?

(In reply to Bill McCloskey (:billm) from comment #15) > In my gut I feel like this is maybe a little overkill. I'd agree you if it were only for one or two uses, but I keep running into places where we need this kind of functionality: * tabs.executeScript, insertCSS, captureVisibleTab, sendMessage, ... * webNavigation.getFrame, getAllFrames * runtime.sendMessage, onMessage, ... Not to mention, I expect, just about every method call for OOP add-ons. At the moment it already adds up to at least a half dozen cases where we're manually using context unload listeners and ad-hoc response ID tracking to handle replies, and still missing corner cases. > I think we would end up with the best design if we kept the sendMessage/Port > code separate from this. It needs extra complexity that this code doesn't. > For example, it looks like it would still be a good amount of work to handle > multiple browser.runtime.onMessage listeners but ensure that only one of > them actually sends a reply. I don't really have a strong opinion about that either way. I do still think we should probably use it for some parts of that code, I'm just not sure how much. I did consider whether it would be useful for onMessage with multiple listeners. I have a few options, but I was leaning towards adding a flag to allow multiple listeners, and choose the response using Promise.race, which I think would pretty simple. > This is a little unconventional. I guess it's more uniform this way, but I > think I'd still rather do direct Cu.import from each module that uses > MessageChannel. Yeah, I think that probably makes more sense. > ::: toolkit/components/extensions/MessageChannel.jsm:92 > (Diff revision 1) > > + let handlers = this.handlers.get(messageName) || []; > > Would be clearer to say "|| new Set()". Yeah. The property started out as an array rather than a set. > For abstraction reasons, I think it would be nicer if this delegated to > FilteringMessageManager. That gets complicated. I tried to do it at first, but the message manger doesn't know enough about the handlers to cancel them. And canceling only makes sense for response listeners, not message listeners. I'll think about storing a separate set of pending responses outside the brokers. > ::: toolkit/components/extensions/MessageChannel.jsm:439 > (Diff revision 1) > > + for (let broker of this.responseBrokers.values()) { > > I guess this makes it hard to use a WeakMap. I don't really see a way around > that. We only need to know about pending responses. If I store those in a separate set, we can still use WeakMaps for the message managers.

(In reply to Bill McCloskey (:billm) from comment #16) > > + browser.test.assertEq(undefined, result, "Expected callback result"); > > My understanding is that Chrome returns 27 here. Maybe I'm wrong? It does. I was going to document it as an incompatibility because I didn't want to have to use evalInSandbox for script files. It looks like loadSubScript does return the result of the evaluation, though, so I guess it's easy enough to fix. > ::: toolkit/components/extensions/Extension.jsm:284 > (Diff revision 1) > > + sender.pageId = this.contextId; > > Why pageId and not contextId? Honestly, I spent too much time thinking about what to name it, and eventually gave up. The class calls itself ExtensionPage, we call the variable `context`, and the API calls them views. It should probably be contextId, though, yeah. > ::: toolkit/components/extensions/ExtensionContent.jsm:156 > (Diff revision 1) > > + > > Why the extra line? Merge botch. > ::: toolkit/components/extensions/ExtensionContent.jsm:190 > (Diff revision 1) > > runSafeSyncWithoutClone(Services.scriptloader.loadSubScriptWithOptions, url, options); > > Shouldn't we catch errors here too? Probably. We should also catch them for the CSS, if it comes to it. But that requires removing the runSafeSyncWithoutClone calls, which leaves the question of what to do when there are multiple errors, and when we're loading scripts defined in the manifest. So I decided I'd leave it until I implemented `runAt` and `allFrames`, since they have the same problems. > Also, I think Chrome treats the result of the last statement (if it's an > expression) as the result in this case. We would need platform support for > that. Could you please file a bug? It looks like we already have support, so it's easy enough to fix. > ::: toolkit/components/extensions/ExtensionContent.jsm:198 > (Diff revision 1) > > + this.deferred.reject(e.message); > > Return here before also resolving the promise? It shouldn't make a difference. I decided against it because I'm pretty sure in most cases we should keep going, even if there's an error, and just return the result of the first resolution or rejection to the caller. I'm going to look into it more for runAt and allFrames support.

Comment on attachment 8709736 [details] MozReview Request: Bug 1210583: Part 1 - [webext] Add support for cross-process messaging with async responses. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31529/diff/1-2/

Comment on attachment 8709737 [details] MozReview Request: Bug 1210583: Part 2 - [webext] Support callbacks in tabs.executeScript/tabs.insertCSS. r=billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31531/diff/1-2/

I can't say that I disagree that this feels too complicated. I'll probably wind up reworking it at some point to simplify things, but this is the kind of thing that I'll work on for months before releasing if I don't stop myself and go with an initial implementation that works. At this point, I'm more interested in the interface than the implementation. If it turns out to be too complicated, or unhelpful, I'm happy to rewrite it, or rip it out and do something else. At the moment, I'm happy with the way it works for my callback use cases. I'm not in a rush to make changes to the messenger/port code, because so far it's working pretty well. But I'm keeping it in mind as a use case, because I think this kind of callback logic is inherently brittle and leak prone, and the fewer different places we have to implement it/think about it, the better.

Comment on attachment 8709736 [details] MozReview Request: Bug 1210583: Part 1 - [webext] Add support for cross-process messaging with async responses. r?billm https://reviewboard.mozilla.org/r/31529/#review29285 ::: toolkit/components/extensions/MessageChannel.jsm:14 (Diff revision 2) > + * Since each message may have only one recipient, the listener end may s/may/must/

https://hg.mozilla.org/integration/fx-team/rev/65b634919d299de20908a8dbffe33ba8a4ab6ad9 Bug 1210583: Part 1 - [webext] Add support for cross-process messaging with async responses. r=billm https://hg.mozilla.org/integration/fx-team/rev/261e997621c182a03bc330c8bd18c98eddb9c7eb Bug 1210583: Part 2 - [webext] Support callbacks in tabs.executeScript/tabs.insertCSS. r=billm

https://hg.mozilla.org/mozilla-central/rev/65b634919d29 https://hg.mozilla.org/mozilla-central/rev/261e997621c1