1210583 - Callback argument ignored by tabs.executeScript and tabs.insertCss

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1208257 +++

The functions accept a callback argument, but it's never actually called.

Comment 1

[Tyler Steiman]

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

Comment 2

[Kris Maglione [:kmag]]

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.

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1210583: Part 1 - [webext] Add support for cross-process messaging with async responses. r?billm

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/

Comment 4

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1210583: Part 2 - [webext] Support callbacks in tabs.executeScript/tabs.insertCSS. r=billm

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

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 7

[Kris Maglione [:kmag]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[Kris Maglione [:kmag]]

(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.

Comment 10

[Andrew McCreight [:mccr8]]

IIRC, putting things in weakmaps requires full WebIDLification.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 12

[Kris Maglione [:kmag]]

(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.

Comment 13

[Andrew McCreight [:mccr8]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 17

[Kris Maglione [:kmag]]

(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.

Comment 18

[Kris Maglione [:kmag]]

(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 19

[Kris Maglione [:kmag]]

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 20

[Kris Maglione [:kmag]]

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/

Comment 21

[Kris Maglione [:kmag]]

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 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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/

Comment 23

[Kris Maglione [:kmag]]

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

Comment 24

[Carsten Book [:Tomcat]]

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