1286746 - runtime.sendMessage callback not called if there are no onMessage listeners

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Rob Wu [:robwu]]

Attachment: sendMessage-no-receiver.zip

1. Load the attached extension.
2. The extension opens a new tab with a demo page.
3. Open the JS console for that tab.
4. Click on the "sendMessage" button.
5. Look at the console.

Expected result: Two messages that show that the callback was invoked.
Actual result  : Response callback is not invoked.

More info: When any message listener is added, the bug goes away. This is caused by lazy initialization of the FilteringMessageManager. As a result, there are no listeners for the MessageChannel:Message message, so there won't be any reply.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1286746 - Invoke sendMessage callback even if there are no listeners

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

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

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

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

https://reviewboard.mozilla.org/r/65038/#review62428

::: toolkit/components/extensions/MessageChannel.jsm:347
(Diff revision 1)
> +   * handler, to make sure that the message always receives a reply, even if
> +   * no handlers have been registered yet.
> +   *
> +   * @param {[nsIMessageSender]} messageManagers
> +   */
> +  setupMessageManagers(messageManagers) {

This is way over-complicated. All we need to do is call call `messageManagers.get` on the messge managers that we care about.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_sendMessage_no_receiver.html:18
(Diff revision 1)
> +<script>
> +"use strict";
> +
> +add_task(function* test_sendMessage_without_listener() {
> +  function backgroundScript() {
> +    browser.runtime.sendMessage("msg", reply => {

Please use the Promise-based version of the API rather than using a callback and checking lastError.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_sendMessage_no_receiver.html:33
(Diff revision 1)
> +    manifest: {},
> +  };
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  yield extension.startup();
> +  info("extension loaded");

Please remove.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_sendMessage_no_receiver.html:38
(Diff revision 1)
> +  info("extension loaded");
> +
> +  yield extension.awaitFinish("sendMessage callback was invoked");
> +
> +  yield extension.unload();
> +  info("extension unloaded");

Please remove.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

https://reviewboard.mozilla.org/r/65040/#review62430

::: toolkit/components/extensions/ExtensionUtils.jsm:1170
(Diff revision 1)
>      let portId = nextPortId++;
>      let port = new Port(this.context, messageManager, name, portId, null);
>      let msg = {name, portId};
> -    // TODO: Disconnect the port if no response?
> -    this._sendMessage(messageManager, "Extension:Connect", msg, recipient);
> +    this._sendMessage(messageManager, "Extension:Connect", msg, recipient)
> +      .catch(e => {
> +        if (e.result == MessageChannel.RESULT_NO_HANDLER) {

It shouldn't matter what the error is here. If we get an error, we should disconnect.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_connect_no_receiver.html:25
(Diff revision 1)
> +      browser.test.notifyPass("port.onDisconnect was called");
> +    });
> +  }
> +  let extensionData = {
> +    background: `(${backgroundScript})();`,
> +    manifest: {},

Not necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_connect_no_receiver.html:30
(Diff revision 1)
> +    manifest: {},
> +  };
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  yield extension.startup();
> +  info("extension loaded");

Please remove.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_connect_no_receiver.html:35
(Diff revision 1)
> +  info("extension loaded");
> +
> +  yield extension.awaitFinish("port.onDisconnect was called");
> +
> +  yield extension.unload();
> +  info("extension unloaded");

Please remove.

Comment 5

[Rob Wu [:robwu]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

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

Comment 6

[Rob Wu [:robwu]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

Please disregard this attachment. I amended the last commit, but the changes should be split over the two patches. I'll shortly submit a fixed version.

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

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

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

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

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

https://reviewboard.mozilla.org/r/65038/#review62724

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

https://reviewboard.mozilla.org/r/65040/#review62726

Comment 11

[Carsten Book [:Tomcat]]

don't apply cleanly:

patching file toolkit/components/extensions/ExtensionUtils.jsm
Hunk #1 FAILED at 1026
Hunk #2 FAILED at 1155
2 out of 2 hunks FAILED -- saving rejects to file toolkit/components/extensions/ExtensionUtils.jsm.rej
patching file toolkit/components/extensions/test/mochitest/mochitest.ini
Hunk #1 FAILED at 54
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/test/mochitest/mochitest.ini.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

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

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26f41a685662
Invoke sendMessage callback even if there are no listeners. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/562910144a3a
Invoke port.onDisconnect if there are no onConnect listeners. r=kmag

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing test_ext_runtime_connect_twoway.html and test_ext_storage_content.html on Android.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f78b9f4278fab7f3fc606f974c104a2721ba769
https://hg.mozilla.org/integration/mozilla-inbound/rev/b62676e921c5d985713576df7ddec6a0c3a3c760

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=562910144a3a32a69af4957e52d250df5a83136e
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=32742119&repo=mozilla-inbound

Comment 16

[Kris Maglione [:kmag]]

Please use the xpcshell version of these tests from https://hg.mozilla.org/integration/mozilla-inbound/rev/af745bd5c20928ef776a7c94c76cb9c87ac77c71 when you re-land.

Comment 17

[Rob Wu [:robwu]]

Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

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

Comment 18

[Rob Wu [:robwu]]

Comment on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

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

Comment 19

[Rob Wu [:robwu]]

+checkin-needed for revised patch, with r+ reaffirmed over IRC.

I converted the tests from mochitests to xpcshell as requested over IRC, and confirmed locally that the tests themselves pass in Fennec.

Locally in the 4.3 emulator I see the following log when I run
MOZCONFIG=~/mozilla-central/.mozconfig-android mach --verbose xpcshell-test toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_no_receiver.js toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js

 1:34.11 PROCESS_OUTPUT: Thread-2 (pid:toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_no_receiver.js) "JavaScript strict warning: /storage/sdcard/tests/xpc/head.js, line 107: ReferenceError: reference to undefined property Components.interfaces.nsICrashReporter"
 1:34.11 PROCESS_OUTPUT: Thread-2 (pid:toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_no_receiver.js) "JavaScript strict warning: /storage/sdcard/tests/xpc/head.js, line 1217: ReferenceError: reference to undefined property Components.interfaces.nsICrashReporter"
-1:37.60 LOG: Thread-2 ERROR NS_ERROR_XPC_BAD_IID: Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]
do_get_profile@/storage/sdcard/tests/xpc/head.js:1216:9
init@resource://testing-common/ExtensionXPCShellUtils.jsm:219:23
@/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js:30:1
load_file@/storage/sdcard/tests/xpc/head.js:659:7
_load_files@/storage/sdcard/tests/xpc/head.js:671:3
_execute_test@/storage/sdcard/tests/xpc/head.js:512:3
@-e:1:1

... but they look unrelated to my change, hence I ignored these warnings.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c208ce0321e8
Invoke port.onDisconnect if there are no onConnect listeners. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/02afaa4b6e39
Invoke sendMessage callback even if there are no listeners. r=kmag

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c208ce0321e8
https://hg.mozilla.org/mozilla-central/rev/02afaa4b6e39

Comment 22

[CosminB]

On Firefox 53.0a1(20161206030203) under Windows 7 64-bit I have the next result: http://screencast.com/t/YAhdGBVAocr6, is this the expected behaviour?

Comment 23

[Rob Wu [:robwu]]

Looks good. The test case prints two messages to the console (error messages, because there is no recipient for the sent messages).

Comment 24

[CosminB]

This issue is verified as fixed on Firefox 51.0b6 (20161201172143), Firefox 52.0a2 (20161207004003) and Firefox 53.0a1 (20161207030204) under Windows 7 64-bit and Ubuntu 16.04 LTS 32-bit according to Comment 22 and Comment 23 with the mention that the stack trace it is not displayed on Firefox 51.0b6 http://screencast.com/t/la1ZtCQg.