1308421 - chrome.runtime.sendMessage callback not called in extension popup

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Markus Hartung]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.50 Safari/537.36

Steps to reproduce:

Using chrome.runtime.sendMessage send a message to the background script and respond in the background script


Actual results:

The callback in the popup was not called


Expected results:

The callback in the popup should be called

Comment 1

[Markus Hartung]

See https://github.com/markus-hartung-avira/web-ext-popup-messaging-bug for minimal example

It is reproducable in FF 51.0a2 (2016-10-06) (32-bit) developer edition, but not in the regular FF 49 version.

Comment 2

[Kris Maglione [:kmag]]

[Tracking Requested - why for this release]: This is a serious regression that was introduced in Firefox 51

Comment 3

[Marcia Knous [:marcia]]

Tracking 52+ for the reason noted in Comment 2.

Comment 4

[Rob Wu [:robwu]]

This is caused by the fact that the popup is initially a preloaded browser.

1) A quick work-around is to route the message through the process manager instead of the frame message manager. Originally, responses went through the process manager as well, but I changed it since frame message managers make more sense and no tests failed: https://hg.mozilla.org/mozilla-central/rev/329102a4c68241bc15f4cd775d9f269e588a91f9#l7.9

2) Another solution is to track browser swaps, e.g. as done in https://reviewboard.mozilla.org/r/78662/diff/4

3) Another option is to queue extension API invocations until the popup browser is swapped (the API is asynchronous, so this shouldn't be a problem, right?).

Which of the options do you prefer (short-term for backporting, and long-term)?

Comment 5

[Kris Maglione [:kmag]]

We need to handle browser swapping correctly so we don't lose resposnes from tab pages either.

Do you mind if I take this bug? I have a working fix that I needed to write so I could test it properly.

Comment 6

[Rob Wu [:robwu]]

Not at all - thanks for fixing it.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

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

Comment 8

[Gerry Chang [:gchang]]

Track 51+ as regression.

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

https://reviewboard.mozilla.org/r/85014/#review83888

::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:25
(Diff revision 1)
>  
> +    files: {
> +      "popup.html": scriptPage("popup.js"),
> +      "popup.js": function() {
>          browser.runtime.onMessage.addListener(msg => {
> -          if (msg == "close-popup") {
> +          if (msg == "popup-ping") {

Turn this into a `browser.test.assertEq`. We are not expecting any other messages (recall that sendMessage is not supposed to dispatch onMessage in the same frame). Similarly, for `onMessage` in the background script.

::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:52
(Diff revision 1)
> -});
> +          });
>  
> -add_task(function* testBrowserActionInPanel() {
> -  yield testInArea(CustomizableUI.AREA_PANEL);
> +          return new Promise(resolve => {
> +            // Wait long enough that we're relatively sure the docShells have
> +            // been swapped.
> +            setTimeout(resolve, 250);

Where is the 250 coming from? It seems to be chosen such that it is "more than POPUP_LOAD_TIMEOUT_MS" from ext-utils.js. If that is indeed the case, please add a comment in case the value of that arbitrarily chosen constant ever changes.

::: browser/components/extensions/test/browser/browser_ext_popup_sendMessage.js:81
(Diff revision 1)
>  
> -  EventUtils.synthesizeMouseAtCenter(widget.node, {type: "mouseup", button: 0}, window);
> +  {
> +    clickPageAction(extension);
>  
> -  yield mouseUpPromise;
> +    let pong = yield extension.awaitMessage("popup-pong");
> +    is(pong, "background-pong", "Got pong");

The use of the same string for two independent test paths is a bit confusing. Please rename one of them, e.g. this one to "background-pong-reply".

Similarly for popup-pong -> popup-pong-reply.

::: toolkit/components/extensions/MessageChannel.jsm:147
(Diff revision 1)
> +   * destroyed.
> +   */
> +  dispose() {
> +    if (this.eventTarget) {
> +      this.removeListeners(this.eventTarget);
> +      this.eventTarget = null;

Move this line inside `removeListeners`.

::: toolkit/components/extensions/MessageChannel.jsm:149
(Diff revision 1)
> +  dispose() {
> +    if (this.eventTarget) {
> +      this.removeListeners(this.eventTarget);
> +      this.eventTarget = null;
> +    } else {
> +      this.messageManager = null;

This is read-only (but configurable). Change it to `delete this.messageManager;`

::: toolkit/components/extensions/MessageChannel.jsm:213
(Diff revision 1)
> +   * Removes docShell swap listeners to the message manager owner.
> +   *
> +   * @param {Element} target
> +   *        The target element.
> +   */
> +  removeListeners(target) {

Remove the in-parameter and use this.eventTarget directly. It is always the same anyway, and then it is obvious that it does not accept arbitrary objects.

::: toolkit/components/extensions/MessageChannel.jsm:778
(Diff revision 1)
>              }
>            }
>          }
>  
>          target.sendAsyncMessage(MESSAGE_RESPONSE, response);
> +      }).then(() => {

FYI: The above calls to `target.sendAsyncMessage` may throw. Then this `dispose` call will never happen.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Kris, is there an action based on the review above?

Comment 11

[Kris Maglione [:kmag]]

Yes, but I've been bogged down dealing with critical uplifts for the past couple of weeks.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

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

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

https://reviewboard.mozilla.org/r/85014/#review88936

Looks good to me.  Would a test with an options page be useful?  (you know the internals here much better than I do, but I think in that case there isn't a docshell swap so that test would confirm this all still works in that case)

::: toolkit/components/extensions/MessageChannel.jsm:116
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
>                                    "resource://gre/modules/PromiseUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");
> +/**
> + * Acts as a proxy for a message manager or message manager owner, and

A clarification that the proxy is only for sending messages and not for receiving would be helpful (it becomes obvious from the code and the use below, but for somebody starting reading with this class like I did, its a useful bit of information)

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

https://reviewboard.mozilla.org/r/85014/#review88936

It couldn't hurt, I guess, but options pages shouldn't be affected by this particular issue.

> A clarification that the proxy is only for sending messages and not for receiving would be helpful (it becomes obvious from the code and the use below, but for somebody starting reading with this class like I did, its a useful bit of information)

Yeah, I guess. I was considering extending it at some point, since there are other places where we could benefit from this helper.

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

https://reviewboard.mozilla.org/r/85014/#review88936

> Yeah, I guess. I was considering extending it at some point, since there are other places where we could benefit from this helper.

Cool, the comment can be updated then along with those extensions.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd43923701cd09a0630be439b19d43fa745ff36
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped. r=aswan

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1dd43923701c

Comment 18

[Gerry Chang [:gchang]]

Hi :kmag,
Do you think this is worth uplifting to 51 aurora?

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

Approval Request Comment
[Feature/regressing bug #]: Bug 1259093
[User impact if declined]: This causes some extensions which rely on receiving replies to messages sent from their browserAction popups shortly after opening to fail in unpredictable ways.
[Describe test coverage new/current, TreeHerder]: The related features are covered by extensive tests, and new tests have been added for this issue.
[Risks and why]: Low. This change simply tracks docshell swaps so that response messages are sent to the correct message manager. If we were likely to see issues from it, they would almost certainly have been seen by now.
[String/UUID change made/needed]: None.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8799977 [details]
Bug 1308421: Handle MessageChannel responses correctly after docshells have been swapped.

Fix a webextension issue related to message handling. Take it in 51 aurora.

Comment 21

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 51.0a2 (2016-11-07) under Windows 10.

Manually verified as fixed on Firefox 52.0a1 (2016-11-07) under Windows 10 64-bit and Ubuntu 16.04 32-bit. The callback is successfully called in the popup: http://screencast.com/t/N5p0Tk5K

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fe13d90d53a

Comment 23

[Vasilica Tamas, QA [:vtamas]]

Confirm that this bug is also fixed on Firefox 51.0a2 (2016-11-08) under Windows 10 64-bit and Ubuntu 16.04 32-bit.