Closed Bug 1286746 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox50 affected, firefox51 verified, firefox52 verified, firefox53 verified)

VERIFIED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: robwu, Assigned: robwu)

Details

(Whiteboard: triaged)

Attachments

(3 files)

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.
Attachment #8772150 - Flags: review?(kmaglione+bmo)
Attachment #8772151 - Flags: review?(kmaglione+bmo)
Attachment #8772150 - Flags: review?(kmaglione+bmo) → review-
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.
Attachment #8772151 - Flags: review?(kmaglione+bmo)
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 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/
Attachment #8772151 - Flags: review?(kmaglione+bmo)
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.
Attachment #8772151 - Flags: review?(kmaglione+bmo)
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/
Attachment #8772150 - Flags: review- → review?(kmaglione+bmo)
Attachment #8772151 - Flags: review?(kmaglione+bmo)
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 on attachment 8772150 [details]
Bug 1286746 - Invoke sendMessage callback even if there are no listeners

https://reviewboard.mozilla.org/r/65038/#review62724
Attachment #8772150 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8772151 [details]
Bug 1286746 - Invoke port.onDisconnect if there are no onConnect listeners

https://reviewboard.mozilla.org/r/65040/#review62726
Attachment #8772151 - Flags: review?(kmaglione+bmo) → review+
Whiteboard: triaged
Keywords: checkin-needed
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
Flags: needinfo?(rob)
Keywords: checkin-needed
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 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/
Flags: needinfo?(rob)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Please use the xpcshell version of these tests from https://hg.mozilla.org/integration/mozilla-inbound/rev/af745bd5c20928ef776a7c94c76cb9c87ac77c71 when you re-land.
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 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/
+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.
Flags: needinfo?(rob)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c208ce0321e8
https://hg.mozilla.org/mozilla-central/rev/02afaa4b6e39
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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?
Flags: needinfo?(rob)
Looks good. The test case prints two messages to the console (error messages, because there is no recipient for the sent messages).
Flags: needinfo?(rob)
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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.