Silence irrelevant browser.runtime.sendMessage error by lydell · Pull Request #131 · mozilla/webextension-polyfill

lydell · 2018-06-19T16:35:13Z

Fixes #130.

See the comments in the code.

You can test here (by changing to browser-polyfill-fork.js in manifest.json): https://github.com/lydell/webextension-polyfill-messaging-issue

Fixes mozilla#130.

rpl · 2018-06-21T19:52:37Z

@lydell Thanks for reporting and looking into this tricky issue, I'm a bit "on the fence" about silencing the errors (because it may theoretically make harder to find real issues that may trigger the same error message, and also on the other side if the error message changes we will not notice it and we will start to produce noise again), but I agree that these errors are going to be confusing given that the developers are not going to be in charge of passing the callbacks explicitly.

I'm going to give a try to your test extension asap, so that I can form a more precise opinion on the issue (and it may be my initial concerns are not really motivated).

In the meantime, as an initial feedback on the change:

the inline comment is a bit too big ;-), I agree that it is a workaround that definitely requires some details but I would prefer a single (or no more than 2 lines) of inline comment and then the link to the issue Chrome: "The message port closed before a response was received." #130 can easily provide any additional details in the issue description
if "making the error silent" is our only option and we are going to proceed with this strategy, it could be worth to still produce some logs for the silenced errors when running in some kind of "dev mode" (e.g. y checking if a certain global is defined, something like typeof WEBEXT_POLYFILL_DEBUG !== "undefined" or another similar kind of runtime check, or by building a separated browser-polyfill.devmode.js file), so that we can document for the addon developer how they can see the errors that may have been silenced (if they need to).

Rob--W · 2018-06-22T09:25:08Z

@rpl I wouldn't worry about false positives. This error message is only generated when the onMessage handler is not invoked. In Chrome, errors (chrome.runtime.lastError) in runtime.sendMessage's callback are silently swallowed, so it is better to ignore known errors too.

In Chrome, runtime.sendMessage is internally implemented as runtime.connect + port.postMessage at the sender's end, and port.postMessage + port.disconnect at the recipient (if they choose to respond). If there is no explicit response in the onMessage handler, the port is closed with the message The message port closed before a response was received..

Without --enable-features=NativeCrxBindings : https://chromium.googlesource.com/chromium/src/+/a647c750dfb95d621807607c75939b8286c7acae/extensions/renderer/resources/messaging.js#437
With --enable-features=NativeCrxBindings : https://chromium.googlesource.com/chromium/src/+/a647c750dfb95d621807607c75939b8286c7acae/extensions/renderer/one_time_message_handler.cc#425

rpl

So, I've been finally able to give this a deeper look (also thanks @Rob--W for the links to the relevant Chrome sources,
it has been very helpful) and it turned out that this actually fix an incompatibility with the behavior that the same scenario has on Firefox: when none of the listeners sent any response, the promise returned by sendMessage is resolved to undefined.

and so it is not only a matter of how annoying or irrelevant is the error message,
it is actually a fix for an incompatibility :-)

@lydell Thanks a lot for catching this, this was a very very subtle incompatibility.

Anyway this looks good to me and there are only some very small nits and tweaks that it would be nice to apply before merging this pull request.

Besides that, as an additional test and proof of the compatibility of this behavior on both Firefox and Chrome,
it would be great to land this change with also a test case for it in the integration tests.

Something like the following diff should be enough to add this the existing messaging test cases:

diff --git a/test/fixtures/runtime-messaging-extension/background.js b/test/fixtures/runtime-messaging-extension/background.js
index c090a8e..52a485b 100644
--- a/test/fixtures/runtime-messaging-extension/background.js
+++ b/test/fixtures/runtime-messaging-extension/background.js
@@ -45,6 +45,9 @@ browser.runtime.onMessage.addListener((msg, sender, sendResponse) => {
     case "test - sendMessage with listener callback throws":
       throw new Error("listener throws");
 
+    case "test - sendMessage and no listener answers":
+      return undefined;
+
     default:
       return Promise.resolve(
         `Unxpected message received by the background page: ${JSON.stringify(msg)}\n`);
@@ -52,6 +55,10 @@ browser.runtime.onMessage.addListener((msg, sender, sendResponse) => {
 });
 
 browser.runtime.onMessage.addListener((msg, sender, sendResponse) => {
+  if (msg === "test - sendMessage and no listener answers") {
+    return undefined;
+  }
+
   setTimeout(() => {
     sendResponse("second listener reply");
   }, 100);
diff --git a/test/fixtures/runtime-messaging-extension/content.js b/test/fixtures/runtime-messaging-extension/content.js
index 5773af2..64d5d6c 100644
--- a/test/fixtures/runtime-messaging-extension/content.js
+++ b/test/fixtures/runtime-messaging-extension/content.js
@@ -75,3 +75,8 @@ test("sendMessage with listener callback throws", async (t) => {
     t.equal(err.message, "listener throws", "Got an error with the expected message");
   }
 });
+
+test("sendMessage and no listener answers", async (t) => {
+  const reply = await browser.runtime.sendMessage("test - sendMessage and no listener answers");
+  t.equal(reply, undefined, "Got undefined as a reply a expected");
+});

rpl · 2018-07-01T18:59:10Z

src/browser-polyfill.js

      removed from the specs (See
      https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onMessage)
    `.replace(/\s+/g, " ").trim();
+  const CHROME_SEND_MESSAGE_CALLBACK_NO_RESPONSE_MESSAGE =


Nit, do you mind to move this one at line 10 (so that two const are going to be in alphabetic order).

rpl · 2018-07-01T19:05:12Z

src/browser-polyfill.js

    const wrappedSendMessageCallback = ({reject, resolve}, reply) => {
      if (chrome.runtime.lastError) {
-        reject(chrome.runtime.lastError);
+        // Passing a callback to `chrome.runtime.sendMessage`


Let's trim this inline comment a bit, and we should actually mention that this is needed to match the behavior on Firefox, e.g. something like the following could be enough:

// Detect when none of the listener replied to the sendMessage call and resolve // the promise to undefined as in Firefox. // See https://github.com/mozilla/webextension-polyfill/issues/130

rpl · 2018-07-01T19:07:46Z

test/test-runtime-onMessage.js

    });
  });
+
+  it("ignores a certain chrome.runtime.lastError", () => {


How about it("resolves to undefined when no listeners reply"?

rpl · 2018-07-01T19:10:41Z

test/test-runtime-onMessage.js

+    const fakeChrome = {
+      runtime: {
+        lastError: {
+          message: "The message port closed before a response was received.",


let's add a 'single line' inline comment which mention that this is the error defined as CHROME_SEND_MESSAGE_CALLBACK_NO_RESPONSE_MESSAGE in the polyfill internals.

rpl · 2018-07-01T19:13:34Z

test/test-runtime-onMessage.js

+    };
+
+    return setupTestDOMWindow(fakeChrome).then(window => {
+      fakeChrome.runtime.sendMessage


Nit, do you mind to move this out of this callback? right after const fakeChrome = {...}; at line 252.

rpl · 2018-07-01T19:23:50Z

test/test-runtime-onMessage.js

+        .onFirstCall().callsArgWith(1, ["res1", "res2"]);
+
+      return window.browser.runtime.sendMessage("some_message").then(
+        (...args) => deepEqual(args, [undefined], "The error was ignored")


Nit, some small tweak to the assertions:

const promise = window.browser.runtime.sendMessage("some_message"); ok(fakeChrome.runtime.sendMessage.calledOnce, "sendMessage has been called once"); return promise.then(reply => { deepEqual(reply, undefined, "sendMessage promise should be resolved to undefined"); });

lydell · 2018-07-01T20:24:37Z

Thanks for confirming/explaining what’s going and for the review, I appreciate it!

I think it’ll be much easier if you do this yourself (when you get the time).

rpl · 2018-07-02T20:25:04Z

@lydell sure, no problem, I just created it as #140

Silence irrelevant browser.runtime.sendMessage error

85151f3

Fixes mozilla#130.

ExE-Boss mentioned this pull request Jun 19, 2018

Release 0.3.0 #88

Closed

ExE-Boss mentioned this pull request Jul 1, 2018

refactor: Make the wrapAPIs function take the chrome object as a parameter #139

Merged

rpl requested changes Jul 1, 2018

View reviewed changes

lydell closed this Jul 1, 2018

rpl mentioned this pull request Jul 2, 2018

fix: sendMessage promise should resolve to undefined when no listeners reply #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Silence irrelevant browser.runtime.sendMessage error#131

Silence irrelevant browser.runtime.sendMessage error#131
lydell wants to merge 1 commit intomozilla:masterfrom
lydell:message-port-closed

lydell commented Jun 19, 2018

Uh oh!

rpl commented Jun 21, 2018

Uh oh!

Rob--W commented Jun 22, 2018

Uh oh!

rpl left a comment

Uh oh!

rpl Jul 1, 2018

Uh oh!

rpl Jul 1, 2018

Uh oh!

rpl Jul 1, 2018

Uh oh!

rpl Jul 1, 2018

Uh oh!

rpl Jul 1, 2018

Uh oh!

rpl Jul 1, 2018

Uh oh!

lydell commented Jul 1, 2018

Uh oh!

rpl commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lydell commented Jun 19, 2018

Uh oh!

rpl commented Jun 21, 2018

Uh oh!

Rob--W commented Jun 22, 2018

Uh oh!

rpl left a comment

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

rpl Jul 1, 2018

Choose a reason for hiding this comment

Uh oh!

lydell commented Jul 1, 2018

Uh oh!

rpl commented Jul 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants