1286124 - chrome.runtime.sendMessage triggers onMessage in the same frame

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

Test steps:
1. Open the debugger for any background page and paste the following (or create a bg page with the following content):
browser.runtime.onMessage.addListener(msg => console.log(`Unexpected ${msg}`));
browser.runtime.sendMessage('xxx').then(console.log, console.error);

Result  : In Firefox Nightly (built today), the console contains "Unexpected xxx".
Expected: The console should show "Could not establish connection. Receiving end does not exist."

The correct behavior is to only trigger the listener in all *extension frames* except for the frame of the sender. The idea is that the onMessage/sendMessage API (also onConnect/connect) APIs are used to exchange data with other frames/tabs/pages (NOT as an internal communication method).

Comment 1

[Kris Maglione [:kmag]]

Why is that the expected behavior? It's certainly not documented in the Chrome docs.

Comment 2

[Rob Wu [:robwu]]

The expected behavior is written in my report: If a message is sent from the extension to itself, the event should not be dispatched in the same frame (of the sender).

It is documented here: https://developer.chrome.com/extensions/messaging#port-lifetime

> Ports are designed as a two-way communication method between different parts of the extension, where a (top-level) frame is viewed as the smallest part.

That part in the documentation was only added a few months ago, before that it was not explicitly documented. I expanded the documentation here: https://codereview.chromium.org/1874133002/

Comment 3

[Nancy Grossman]

In Chrome, `runtime.sendMessage` sends to every extension page, including the method caller. That's canonical [1]: "If sending to your extension, the runtime.onMessage event will be fired _in each page_".

Chrome's deprecated `extension.sendRequest` sends to every page except the caller's page. That's also canoncial [2]: "Sends a single request to _other listeners_ within the extension".

Chrome's still older (and now undocumented?) `extension.sendMessage` currently sends to all pages like `runtime.sendMessage` but IIRC, when this was still in use, some pages would hear an echo and other (including the background page) would not.

If you must improve messaging, maybe wire it up as `extension.sendMessage` to avoid breaking extensions that already use `runtime.sendMessage`?

[1] https://developer.chrome.com/extensions/runtime#method-sendMessage
[2] https://developer.chrome.com/extensions/extension#method-sendRequest

Comment 4

[Nancy Grossman]

Sorry no, I meant wire it up as `extension.sendRequest` of course. (And of course I really think you should not.)

Comment 5

[Rob Wu [:robwu]]

extension.sendRequest is not implemented in Firefox (and has been deprecated since Chrome 20).
Chrome's extension.sendMessage is an alias for runtime.sendMessage, with the latter being the recommended method.

> "If sending to your extension, the runtime.onMessage event will be fired _in each page_".

This should include: "excluding the frame of the sender".

I'm certain that Firefox's current behavior is incompatible with Chromium's extension model, because I have developed numerous extensions that rely on extension messaging, and also contributed to Chromium's implementation of extension messaging.
Extensions that exchange messages bidirectionally may be affected by this bug (in the best case the extension ignores the message, in the worst case the extension breaks).

Comment 6

[Kris Maglione [:kmag]]

(In reply to Rob Wu [:robwu] from comment #2)
> The expected behavior is written in my report: If a message is sent from the
> extension to itself, the event should not be dispatched in the same frame
> (of the sender).
> 
> It is documented here:
> https://developer.chrome.com/extensions/messaging#port-lifetime
> 
> > Ports are designed as a two-way communication method between different parts of the extension, where a (top-level) frame is viewed as the smallest part.

This documentation refers to ports, which are not the same thing as messages. If something similar does apply to messages, it seems to imply that sub-frames should not be able to use runtime messaging to communicate with parent frames, or vice versa.

Comment 7

[Rob Wu [:robwu]]

(In reply to Kris Maglione [:kmag] from comment #6)
> (In reply to Rob Wu [:robwu] from comment #2)
> > The expected behavior is written in my report: If a message is sent from the
> > extension to itself, the event should not be dispatched in the same frame
> > (of the sender).
> > 
> > It is documented here:
> > https://developer.chrome.com/extensions/messaging#port-lifetime
> > 
> > > Ports are designed as a two-way communication method between different parts of the extension, where a (top-level) frame is viewed as the smallest part.
> 
> This documentation refers to ports, which are not the same thing as
> messages.

In Chrome, sendMessage is currently implemented using ports.

> If something similar does apply to messages, it seems to imply
> that sub-frames should not be able to use runtime messaging to communicate
> with parent frames, or vice versa.

The "(top-level)" in "(top-level) frame" means that "frame" does not just mean "subframe", but that it includes main frames. I'll submit a documentation patch to avoid confusion.

Comment 8

[Nancy Grossman]

At this point Firefox and Chrome work alike, yes? So, maybe wait for clarification from Chromeo?
https://bugs.chromium.org/p/chromium/issues/detail?id=597698

Comment 9

[Nancy Grossman]

Wait, no, that's not it. Did you report this yet as a Chromium bug, Rob Wu? I can't find it.

Comment 10

[Rob Wu [:robwu]]

At this point Firefox and Chrome do not work alike. Try the following snippet. In Chrome, nothing is printed; In Firefox "unexpected xxx" is printed.

// From the background page
chrome.runtime.onMessage.addListener(msg => console.log(`Unexpected ${msg}`));
chrome.runtime.sendMessage('xxx');

Here is the behavior of runtime.onMessage fro calling runtime.sendMessage in various contexts:

Extension page 1
Extension page 2
Content script 3

sender -> receiver / onMessage triggered in Chrome? / onMessage triggered in Firefox?
1 -> 2  Y Y (different frame) OK
3 -> 1  Y Y (from non-extension page to extension) OK
1 -> 3  N N (runtime.sendMessage does not deliver to content script. use tabs.sendMessage instead). OK
3 -> 3  N N (runtime.sendMessage sends a message to the extension, not the content script) OK
1 -> 1  N Y (same frame) INCONSISTENT

The expected behavior for the same-frame case is specified in https://crbug.com/479951, and reaffirmed in https://crbug.com/479425.
The bug that you are referencing (https://crbug.com/597698) shows that onMessage is only triggered if the response callback is not set. This behavioral difference depending on the presence of the callback makes no sense (hence the crbug report).

(In reply to Nancy Grossman from comment #9)
> Wait, no, that's not it. Did you report this yet as a Chromium bug, Rob Wu?
> I can't find it.

What is "this"?

Comment 11

[Nancy Grossman]

"this" is apparently a misunderstanding stemming from another Chrome bug. I did not see inconsistent behaviour in Chrome 51, so I assumed you were proposing a change in both browsers. However, I was testing with the extension attached in [1]. With an extension page open in a tab, the background page hears an echo from `runtime.sendMessage`, which is what I reported in #c3. With no extension pages open, the echo disappears, which is what you report in #c10. 

Not good. However, I think the first step is still the same: File a Chrome bug to elicit a ruling from the judges on what is the correct behaviour, preferably codified as API documentation, and proceed from there.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=626926#c2

Comment 12

[Rob Wu [:robwu]]

(In reply to Nancy Grossman from comment #11)
> I think the first step is still the same: File a Chrome
> bug to elicit a ruling from the judges on what is the correct behaviour,
> preferably codified as API documentation, and proceed from there.

What part of the behavior is incorrect and unclear / requires clarification?

PS. Your test extension contains your email address in the applications.gecko.id field. It's better to choose a semi-random identifier, because the ID needs to be unique for every addon.

Comment 13

[Nancy Grossman]

Whether or not a sender should receive an `onMessage` event after it calls `runtime.sendMessage`. I read it one way in [1], you interpret it another way in comment 2, and Chrome can go either way depending on what's open in the tabs. Don't you think that deserves clarification?

(In reply to Rob Wu [:robwu] from comment #12)
> PS. Your test extension contains your email address in the applications.gecko.id field.

Oh, I meant to remove that. I don't want the Chrome people to feel badly that I'm using another browser. Does it matter? It was only an example for the bug guy.

Comment 14

[Rob Wu [:robwu]]

(In reply to Nancy Grossman from comment #13)
> Whether or not a sender should receive an `onMessage` event after it calls
> `runtime.sendMessage`. I read it one way in [1], you interpret it another
> way in comment 2, (...)and Chrome can go either way depending on what's open in
> the tabs. Don't you think that deserves clarification?

Chromium's stance is clear, onMessage shouldn't be triggered in the sender: https://crbug.com/479951#c4 and https://crbug.com/479425#c6 (these and other bugs were also linked in my reply in comment 10).
 
> (In reply to Rob Wu [:robwu] from comment #12)
> > PS. Your test extension contains your email address in the applications.gecko.id field.
> 
> Oh, I meant to remove that. I don't want the Chrome people to feel badly
> that I'm using another browser. Does it matter? It was only an example for
> the bug guy.

It does not matter, don't worry about it :)

Comment 15

[Nancy Grossman]

(In reply to Rob Wu [:robwu] from comment #14)
> Chromium's stance is clear, onMessage shouldn't be triggered in the sender:

Your second reference is just you making the same assertion you've made here, so let's set that one aside. Your first reference is some poor guy who tried and failed to restore the `runtime.sendMessage` behaviour that now prevails in Firefox, threw up his hands, and said "I like this better anyway. Let's just give up and call it a bug fix." Which is really funny (thank you), but it's a long way from "Chromium's stance is clear".

So you say its cannon, the extension writer says its a bug [3], and poor Kalman says "Uh uh, I'm not goin' in *there* again." Meanwhile Chrome still exhibits both behaviours depending on conditions.

[3] https://bugs.chromium.org/p/chromium/issues/detail?id=480508

> https://crbug.com/479951#c4 and https://crbug.com/479425#c6 (these and other
> bugs were also linked in my reply in comment 10).
>  
> > (In reply to Rob Wu [:robwu] from comment #12)
> > > PS. Your test extension contains your email address in the applications.gecko.id field.
> > 
> > Oh, I meant to remove that. I don't want the Chrome people to feel badly
> > that I'm using another browser. Does it matter? It was only an example for
> > the bug guy.
> 
> It does not matter, don't worry about it :)

Comment 16

[Rob Wu [:robwu]]

(In reply to Nancy Grossman from comment #15)
> (In reply to Rob Wu [:robwu] from comment #14)
> > Chromium's stance is clear, onMessage shouldn't be triggered in the sender:
> 
> Your second reference is just you making the same assertion you've made
> here, so let's set that one aside. Your first reference is some poor guy who
> tried and failed to restore the `runtime.sendMessage` behaviour that now
> prevails in Firefox, threw up his hands, and said "I like this better
> anyway. Let's just give up and call it a bug fix." Which is really funny
> (thank you), but it's a long way from "Chromium's stance is clear".
> 
> So you say its cannon, the extension writer says its a bug [3], and poor
> Kalman says "Uh uh, I'm not goin' in *there* again." Meanwhile Chrome still
> exhibits both behaviours depending on conditions.

In the case where the actual behavior differed from the documented behavior (described below), the head of the extension team (Kalman) clarified the intended behavior, and even added a regression test (https://crbug.com/479951#c8) to make sure that Chrome continues to behave in that way. This is quite a strong assertion of expected behavior.

My claim at https://crbug.com/479425#c6 ("sendMessage should never trigger onMessage in the same frame.") is followed by a patch that fixed the inconsistency. If the claim were to be bogus, the patch wouldn't be accepted, hence that reference supports the claim about the expected behavior.

If you're still not convinced, please join the #webextensions channel at irc.mozilla.org and then we can discuss it further.


The expected behavior can be inferred from Chrome's documentation as follows:

[runtime.sendMessage]
https://developer.chrome.com/extensions/runtime#method-sendMessage

> ... Similar to [runtime.connect] but only sends a single message ...

[runtime.connect]
https://developer.chrome.com/extensions/runtime#method-connect
The connect method specifies a return value of type [runtime.Port].

[runtime.Port]
https://developer.chrome.com/extensions/runtime#type-Port

> An object which allows two way communication with other pages. See [Long-lived connections] for more information.

[Long-lived connections]
https://developer.chrome.com/extensions/messaging#port-lifetime
In the notes on the lifetime semantics of ports:

> Ports are designed as a two-way communication method between **different parts** of the extension, where a (top-level) **frame is viewed as the smallest part.**

Comment 17

[Rob Wu [:robwu]]

Attachment: Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

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

Comment 18

[Rob Wu [:robwu]]

Attachment: Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

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

Comment 19

[Rob Wu [:robwu]]

Attachment: Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

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

Comment 20

[Rob Wu [:robwu]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64138/diff/1-2/

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/1-2/

Comment 22

[Rob Wu [:robwu]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64142/diff/1-2/

Comment 23

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

I switched these over to Kris since he's more familiar with this code.

Comment 24

[Rob Wu [:robwu]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64138/diff/2-3/

Comment 25

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/2-3/

Comment 26

[Rob Wu [:robwu]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64142/diff/2-3/

Comment 27

[Rob Wu [:robwu]]

Updated patches to use xpcshell tests instead of mochitest. Kris, please review them!

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

https://reviewboard.mozilla.org/r/64142/#review66932

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

https://reviewboard.mozilla.org/r/64138/#review66934

::: toolkit/components/extensions/test/xpcshell/test_ext_background_runtime_connect_params.js:24
(Diff revision 3)
> +    }
> +  }
> +
> +  browser.runtime.onConnect.addListener(countReceivedPorts);
> +
> +  let f = document.createElement("iframe");

Please avoid one-letter variable names.

::: toolkit/components/extensions/test/xpcshell/test_ext_background_runtime_connect_params.js:25
(Diff revision 3)
> +  }
> +
> +  browser.runtime.onConnect.addListener(countReceivedPorts);
> +
> +  let f = document.createElement("iframe");
> +  f.src = browser.extension.getURL("extensionpage.html");

No need for `getURL`. Relative URLs should work fine.

::: toolkit/components/extensions/test/xpcshell/test_ext_background_runtime_connect_params.js:61
(Diff revision 3)
>  
>  let extensionData = {
>    background: backgroundScript,
> -  manifest: {},
> -  files: {},
> +  files: {
> +    "senderScript.js": senderScript,
> +    "extensionpage.html": "<script src='senderScript.js'><\/script>",

"extensionpage.html": String.raw`<!DOCTYPE html>
      <html>
        <head>
          <meta charset="utf-8">
          <script src="senderScript.js"></script>
        </head>
      </html>
    `,

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js:32
(Diff revision 3)
> +    let f = document.createElement("iframe");
> +    f.src = browser.extension.getURL("extensionpage.html");
> +    document.body.appendChild(f);

Same issues as above.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js:73
(Diff revision 3)
>  
>    let extension = ExtensionTestUtils.loadExtension({
>      background,
> +    files: {
> +      "senderScript.js": senderScript,
> +      "extensionpage.html": "<script src='senderScript.js'><\/script>",

Same issues as above.

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review66936

::: toolkit/components/extensions/MessageChannel.jsm:178
(Diff revision 3)
> -      if (MessageChannel.matchesFilter(handler.messageFilterStrict || {}, recipient) &&
> +      if ((!handler.messageSender ||
> +           !MessageChannel.matchesFilter(handler.messageSender, sender)) &&
> +          MessageChannel.matchesFilter(handler.messageFilterStrict || {}, recipient) &&

I'd rather this logic not be in MessageChannel for now. The message listeners can choose to ignore messages from themselves.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:6
(Diff revision 3)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +
> +"use strict";
> +
> +function background() {

Please move these function declarations into the task.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:13
(Diff revision 3)
> +    browser.test.assertEq("last-message", msg);
> +    browser.test.sendMessage("onMessage triggered in child frame");
> +  });
> +
> +  browser.runtime.sendMessage("should not trigger same-frame onMessage")
> +  .then(reply => {

Please indent at least two spaces.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:14
(Diff revision 3)
> +    browser.test.sendMessage("onMessage triggered in child frame");
> +  });
> +
> +  browser.runtime.sendMessage("should not trigger same-frame onMessage")
> +  .then(reply => {
> +    browser.test.notifyFail(`Unexpected reply to sendMessage: ${reply}`);

We generally use the same message for `notifyFail` calls as we do for `notifyPass`. A separate `browser.test.fail` call would be better.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:19
(Diff revision 3)
> +  // Insert a child frame that sends a message. Extension messages are typically
> +  // delivered in-order, so if the frame's message is received without the above
> +  // message, then we can be quite confident that the test passes - i.e. the
> +  // onMessage listener in the sender's frame is not triggered.

I'd rather we not rely on that behavior. We should add listeners to both frames and make sure we only get one reply to the message sent from the background script, and one reply to the message sent from the subframe.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:23
(Diff revision 3)
> +  let f = document.createElement("iframe");
> +  f.src = browser.extension.getURL("extensionpage.html");
> +  document.body.appendChild(f);

Same issues here as in the other patch.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_self.js:37
(Diff revision 3)
> +add_task(function* test_sendMessage_to_self_should_not_trigger_onMessage() {
> +  let extensionData = {
> +    background,
> +    files: {
> +      "lastScript.js": lastScript,
> +      "extensionpage.html": "<script src='lastScript.js'><\/script>",

Same issues as other patch.

Comment 31

[Nancy Grossman]

This isn't how it works in Chrome.
https://bugs.chromium.org/p/chromium/issues/detail?id=628083

Comment 32

[Rob Wu [:robwu]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64138/diff/3-4/

Comment 33

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/3-4/

Comment 34

[Rob Wu [:robwu]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64142/diff/3-4/

Comment 35

[Nancy Grossman]

I see you quashed my bug report by marking it a dup of the issue which you said in Comment 10 was unrelated. I can't help thinking that you're avoiding the oversight of other Chrome developers, which in turn leads me to believe that `runtime.sendMessage` functions in Chrome as documented by Chrome because that's the way it's intended to work.

The same method should behave identically in Chrome and in Firefox unless there is some substantial advantage to be realized by "improving" it. What do we get in exchange for introducing this inconsistent behaviour?

Comment 36

[Rob Wu [:robwu]]

(In reply to Nancy Grossman from comment #35)
> I see you quashed my bug report by marking it a dup of the issue which you
> said in Comment 10 was unrelated.

See my reply at https://crbug.com/628083#c9

> The same method should behave identically in Chrome and in Firefox unless
> there is some substantial advantage to be realized by "improving" it. What
> do we get in exchange for introducing this inconsistent behaviour?

I also want identical behavior, that's why I opened this bug and write a patch.

Before this patch, Firefox sends messages to the same frame. This is incompatible with Chrome in most cases*.
There is one edge case in Chrome where messages are received by the same frame, but that is a bug in Chrome that will be fixed (please see my risk analysis at the bottom of https://crbug.com/628083#c9).

* In the following cases, Chrome does not trigger onMessage at the sender (as expected). I marked the situations where Firefox differs with "INCOMPATIBLE ATM":
runtime.sendMessage(msg, callback);  // From content script
runtime.sendMessage(msg);            // From content script
runtime.sendMessage(msg, callback);  // From extension frame (the only one) INCOMPATIBLE ATM
runtime.sendMessage(msg);            // From extension frame (the only one) INCOMPATIBLE ATM
runtime.sendMessage(msg, callback);  // From extension frame (with more than one extension frames) INCOMPATIBLE ATM
runtime.sendMessage(msg);            // From extension frame (with more than one extension frames) NOT AS EXPECTED

The last case is why I put "INCONSISTENT" in comment 10. If you look at the scenarios, then it should be obvious that this is an edge case and that in the majority of cases onMessage is not triggered in the same frame.

Comment 37

[Nancy Grossman]

> * In the following cases, Chrome does not trigger onMessage at the sender (as expected)

In fact `runtime.sendMessage` does trigger `onMessage` at the sender in Chrome whenever 2+ extension pages are open; you can confirm with the extension attached to the Chrome bug you quashed. This is the de facto behaviour that Chrome extensions have been developed against, is the behaviour that Firefox has implemented, and is likely the only behaviour of interest to extension developers (very little utility in sending messages to yourself, and it doesn't work anyway in Chrome without a second page open).

You've apparently described what you believe Chrome should do, not what Chrome actually does. Unless and until Chrome changes the way `runtime.sendMessage` operates, identical behaviour means preserving the current *observable* behaviour, which is the echo-thingy that so offends you.

Messaging between background and content script isn't germaine, though I take your point that the `runtime.sendMessage` method available to content scripts doesn't behave like the `runtime.sendMessage` method available to background scripts. However, that particular peccadillo is preserved in Firefox's implemention and should remain so for now.

Comment 38

[Rob Wu [:robwu]]

The Chrome team lead confirmed the expected behavior in https://crbug.com/628083#c11.

> This is the de facto behaviour that Chrome extensions have been developed against,
> and is likely the only behaviour of interest to extension developers

I just analyzed a random sample of 38543 public extensions in the Chrome Web Store and looked for use of the messaging APIs (including the deprecated ones).

- 7250 of them use extension.sendRequest, extension.sendMessage or runtime.sendMessage.
- 1841 of them use extension.sendRequest (note: deprecated over 4 years ago in Chrome, unsupported in Firefox)
- 2020 of them use extension.sendMessage (note: deprecated over 3 years ago in Chrome, unsupported in Firefox)
- 4060 of them use runtime.sendMessage
- 1422 of them ONLY use extension.sendRequest (deprecated)
- 1543 of them ONLY use extension.sendMessage (deprecated)
- 3642 of then ONLY use runtime.sendMessage (good)

The deprecated APIs are not used by Firefox, so I'll only look at the 4060 extensions that use runtime.sendMessage.

- 1332 of them are certainly unclassifiable (I only looked at sendMessage calls on a single lines, and if the call is not on the same line I cannot tell whether a callback is present or not).
- 1319 of them certainly use an inline callback - runtime.sendMessage(...., function(...) { ... })
- 932 use a callback as a variable name - runtime.sendMessage(..., ...)
- 427 use runtime.sendMessage(primitive values/expressions or variable names)
  (of those, 113 send a single primitive value: string, number, boolean, null)

Only the last category may potentially be affected by the fix that prevents onMessage from being trigered in the same frame.
Please note that those results include content scripts and unprivileged extension frames (which are not affected by the change).
If I manually inspected the few dozen addons that use sendMessage with a single value, and found only one extension that uses runtime.sendMessage without callback in a popup window while the background is open. And event this extension would work perfectly well with the proposed change because sets the checkbox state in response to a click on a checkbox. Well, after clicking on the checkbox the state is already changed so the message is redundant.
(this is the extension: https://chrome.google.com/webstore/detail/steam-incoming-trades-con/eonngpbppibmbhhnbjmpbnnhpbbfeick)

I also looked specifically for the last pattern in the source code of addons on addons.mozilla.org and did not find any addon that makes use of this pattern.

This experimental analysis confirms my hypothesis (that the effect of making the API consistent has minimal risk of breaking extensions). Further, another Chromium team member confirmed that the expected behavior is correct, so I think that is is now without doubt that we should proceed with this patch.

I'd like to stress that this the the behavior that *both* Chrome and Firefox (will) follow. Ultimately the messaging APIs are internally consistent and Chrome and Firefox will be 100% interoperable in this regard.

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review68842

It doesn't look like you've addressed any of the issues from my last review.

Comment 40

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review66936

> I'd rather this logic not be in MessageChannel for now. The message listeners can choose to ignore messages from themselves.

We need this to distinguish between the case where no listeners are defined, and the case where listeners are defined (even if the listeners don't handle the message). The first case should generate an error (= no handlers are yield here), which is different from the second case where no error is generated but an empty reply is received (=your patch at bug 1290117).

The `_callHandlers` method of MessageChannel is responsible for discerning the two (http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/toolkit/components/extensions/MessageChannel.jsm#527), and ExtensionUtils ultimately handles the result (http://searchfox.org/mozilla-central/rev/389a3e0817b873a64376ec2b65f6807afbba9d8f/toolkit/components/extensions/ExtensionUtils.jsm#1220).

Hence not having the logic in MessageChannel is not possible at the moment.

Comment 41

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review68842

Are we looking at the same patch? I've resolved all review points except for the handler.messageSender one (with a motivation).

List of resolved issues: https://reviewboard.mozilla.org/r/64140/#issue-summary
Diff: https://reviewboard.mozilla.org/r/64140/diff/3-4/

Comment 42

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

(see previous comment)

PS. I just realized that comment 40 from reviewboard was not included in the initial published review. Please take another look, and consider comment 40 during your review.

Comment 43

[Kris Maglione [:kmag]]

(In reply to Rob Wu [:robwu] from comment #41)
> Are we looking at the same patch? I've resolved all review points except for
> the handler.messageSender one (with a motivation).

Well, in any event, you didn't address it or comment on why you didn't
address it.

(In reply to Rob Wu [:robwu] from comment #40)
> > I'd rather this logic not be in MessageChannel for now. The message listeners can choose to ignore messages from themselves.
> 
> We need this to distinguish between the case where no listeners are defined,
> and the case where listeners are defined (even if the listeners don't handle
> the message). The first case should generate an error (= no handlers are
> yield here), which is different from the second case where no error is
> generated but an empty reply is received (=your patch at bug 1290117).

OK. Well, in any case, this logic is getting far too specialized, and the
`messageSender` property has the opposite behavior of the other filter
properties. If we're going to handle this on the message channel side, I'd
rather we just add a `filterMessage` method, or something along those lines,
rather than try to fit it into the current declarative logic.

Comment 44

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/4-5/

Comment 45

[Rob Wu [:robwu]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64142/diff/4-5/

Comment 46

[Kris Maglione [:kmag]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review69644

This doesn't appear to have addressed my comments.

Comment 47

[Rob Wu [:robwu]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64138/diff/4-5/

Comment 48

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/5-6/

Comment 49

[Rob Wu [:robwu]]

Comment on attachment 8770810 [details]
Fix typo in Extension.jsm and ext-tabs.js, page-open -> page-load

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64142/diff/5-6/

Comment 50

[Kris Maglione [:kmag]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

https://reviewboard.mozilla.org/r/64140/#review72876

r=me with the change to the handling of `filterMessage` return values.

::: toolkit/components/extensions/MessageChannel.jsm:443
(Diff revision 6)
> +   *      filterMessage:
> +   *        An optional function that prevents the handler from handling a
> +   *        message by returning `true`. See `getHandlers` for the parameters.

I think it makes more sense to return true for messages we want to handle, and false for ones we don't.

Comment 51

[Rob Wu [:robwu]]

Comment on attachment 8770808 [details]
Bug 1286124 - Part 1/2 - Modify existing tests to not use same-frame messaging

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64138/diff/5-6/

Comment 52

[Rob Wu [:robwu]]

Comment on attachment 8770809 [details]
Bug 1286124 - Part 2/2 - Do not deliver messages to the sender's frame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64140/diff/6-7/

Comment 53

[Rob Wu [:robwu]]

The relevant tests are looking good - requesting checkin.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39fd730e7fb9
Part 1/2 - Modify existing tests to not use same-frame messaging r=kmag
https://hg.mozilla.org/integration/autoland/rev/9a737024c5cc
Part 2/2 - Do not deliver messages to the sender's frame r=kmag

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39fd730e7fb9
https://hg.mozilla.org/mozilla-central/rev/9a737024c5cc

Comment 56

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this initial issue on Firefox 50.0a2 (2016-09-12) under Windows 10 64-bit.

Verified fixed on Firefox 51.0a1 (2016-09-12) under Windows 10 64-bit. The following error is successfully displayed: "Could not establish connection. Receiving end does not exist."