Support multiple runtime.onMessage listeners that do not return results

[collinsauve]

When multiple listeners are setup, if they are both async or return promises but those promises do not actually return something when resolved, it will throw the following:

Cannot send a response more than once per chrome.runtime.onMessage listener per document (message was sent by extensionoegnngioepgemfkdgnfhmlaohjkddmlb).

This should be able to be reproduced as follows:

async function handle() {
    await somethingElse();
}
browser.runtime.onMessage.addListener(handle);
browser.runtime.onMessage.addListener(handle);

The listeners should not call sendResponse if there is no result from a promise.

[Rob--W]

Note that if none of the handlers return a non-undefined value, then it is up to the garbage collector to clean the callback (if possible) and then ultimately trigger a response. Can you verify that the callback at the other end (the caller of browser.runtime.sendMessage) is actually called?

And you don't need to base your PR on mine, I don't touch the promise part of the onMessage handler. Furthermore, the PR title and commit title do not actually reflect the effect of the change, please choose a more accurate title. Thanks!

[collinsauve]

Rewrote PR to not build off of @Rob--W 's PR.

I will test to see if callback is called if all of the handlers undefined

[collinsauve]

@Rob--W you are correct, if no listener returns a non-undefined result then the callback will not be called. Eventually the caller will throw an exception: "The message port closed before a response was received."

I'm not yet sure how to fix this though, or even if its possible.

[Rob--W]

That behavior looks reasonable given the conflicting constraints. The promise rejection matches Firefox's implementation too (in Firefox the error is "No handler returned a response").

If an extension dev wishes to explicitly send no response (so a fulfilled promise instead of a rejected one), then they can yield null instead of a void promise.

[collinsauve]

For what its worth the Firefox implementation seems to treat a promise that resolves with undefined the same as any other value. I believe it takes the first value returned and ignores others. I do not believe this is reconcilable with Chrome's behavior, since we can't return 2 values. Since listeners can be in different content scripts we also cannot intercept all listeners at once to determine which is the first one to return a non-undefined and just use that one.

Here's a test I just did on FF 51.0.1:

background.js:

function handle(result) {    
    console.log('handling message');
    return new Promise(resolve => resolve());
}
browser.runtime.onMessage.addListener(handle);
browser.runtime.onMessage.addListener(handle);

content.js:

console.log("sending");
browser.runtime.sendMessage({})
    .then((result) => {
        console.log("done " + result);
    });

background:

handling message background.js:2:5
handling message background.js:2:5

content:

sending content.js:1:1
done undefined content.js:4:9

[collinsauve]

Unfortunately this PR will be a breaking change for existing extensions that may only use a single listener. If it returns a Promise that yields undefined, the background script will have a delay (I believe as you said until the GC picks it up) and then throw an exception.

[rpl]

Hi @collinsauve, in #115 we added a test case (

webextension-polyfill/test/fixtures/multiple-onmessage-listeners-extension/background.js

Lines 28 to 30 in 5d186ba

	// Register the same message handler twice.
	browser.runtime.onMessage.addListener(testMessageHandler);
	browser.runtime.onMessage.addListener(testMessageHandler);

) which should be similar to the scenario you describe in this issue, the test also verifies that the scenario described works on both Chrome and Firefox in similar ways (actually on Chrome when undefined is sent as a response, the sender receives it as null, on Firefox it is received as undefined).

I haven't seen the Cannot send a response more than once per chrome.runtime.onMessage listener per document message on Chrome, but I would expect it to be more like a warning, the first message sent should be delivered in Chrome and that error (or warning) shouldn't be blocking.

I'm wondering if the test case is missing something from the scenario that allowed you to trigger it, or if the changes applied recently to the onMessage listener wrapper may have changed the behavior that was triggering it.

I'm closing this PR in the meantime, preventing undefined to be sent as a response would potentially brake any extensions that expects that behavior (and like the new test shows in practice, it is also the expected behavior on Firefox, where the polyfill is inactive when included by a cross-browser extension)