1202481 - Fix browser.runtime.onMessage reply handling

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

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

Attachment: patch

The sendResponse function for chrome.runtime.onMessage is pretty complicated:
https://developer.chrome.com/extensions/runtime#event-onMessage
I didn't get it right when I first implemented it.

One problem is that EventManager isn't the right model for onMessage. EventManager collapses all addListener together. However, each onMessage listener can independently decide whether to send a response asynchronously or not (by returning true). So it's important that we run each listener independently. This patch switches us over to a SingletonEventManager, which does the right thing.

(I'm starting to realize that EventManager is not the right designed. I want to switch everything over to SingletonEventManager soon.)

The patch also fixes a bug where we sent the {gotData: false} message even if the onMessage handler returned true. The whole point is that we shouldn't do that.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8657916 [details] [diff] [review]
patch

Review of attachment 8657916 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe I'm just missing something, but it feels like this is still not the correct behavior... If that's a known issue and you want to take care of that in another bug/patch let me know, we can land this one as it is in that case, since it's an improvement for sure.

::: toolkit/components/extensions/ExtensionUtils.jsm
@@ +508,5 @@
>            if (!valid) {
>              return;
>            }
>            sent = true;
>            mm.sendAsyncMessage(replyName, {data, gotData: true});

"If you have more than one onMessage listener in the same document, then only one may send a response"

I don't see that we're doing this right now. It's quite vague, I would expect that the "one" mentioned above can only send response _once_ (even though that restriction is not explicitly stated). For that in sendResponse we should return if sent===true not just in the !valid case.

But making sure that only one listener can call sendMessage when multiple listeners returns true is a bit more complicated. Since this closure is not shared between the listeners (if I'm not mistaken) so that |sent| local variable maybe should live on the context or in a shared closure? Anyway, could you please add a test for this case? (so multiple listeners from the same document returns true and then each try and call sendResponse but only the first one has an effect)

Comment 2

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

Attachment: patch v2

This adds a test for the double reply case. When you have multiple onMessage listeners that send replies, Chrome just accepts one of the replies and ignores the other ones. Our code does the same, because the sendMessage code stops listening for a reply after it gets the first one. I commented out the removeMessageListener call and this new test fails.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8658975 [details] [diff] [review]
patch v2

Review of attachment 8658975 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag here, I was completely off the grid for my PTO... and thanks for the test!

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f501bd57cd2

Comment 5

[Phil Ringnalda (:philor)]

Backed out the whole push in https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e83a8b6b8e since it was intertangled and I wasn't entirely sure which part caused Mulet mochitest-5 to become permaorange with https://treeherder.mozilla.org/logviewer.html#?job_id=14545777&repo=mozilla-inbound

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afd749cdf9ce

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afd749cdf9ce