feat: Added support for the sendResponse callback in the runtime.onMessage listeners

[rpl]

This pull requests contains a reworked version of Rob's pull request #22 which aimed to fix #16 by implementing a wrapper for the runtime.onMessage API event which would behave similarly to how it works natively on Firefox (and described in the related API docs on MDN).

Starting from the original #22 pull request, I applied the following changes:

• fixes on the runtime.onMessage listener wrapper to implement the following two scenario as in Firefox:

  • when the listener calls the sendResponse but also returns a promise, the sendResponse calls should be ignored (basically the returned promise has an higher precedence and it will override any sendResponse calls, both synchronously called in the listener or asynchronously called after the listener returns)
  • the listener return value is used to send a response only if it is a promise, any other plain return value should to be just ignored (and it is going to be considered as "this listener is not going to call sendResponse asynchronously")
  • if the listener return value is true, we should let Chrome know that the listener is going to call sendResponse asynchronously (by ensuring to return true also from the "listener wrapper" generated by this polyfill)

• simplify and fix the new unit tests (so that they actually cover the above expected behaviors)

• added a small smoke test to verify that the the polyfill wrappers are working as expected when running on a real Chrome test extension, and that they behave as expected in the above scenarios

Follows some additional context about the initial decision about #16 and the why I think it is time to re-evaluate that decision:

At the time we closed #22 and marked #16 as wontfix because the sendResponse callback was expected to be deprecated in the w3c draft once it would have also been updated to include the Promise-based API (#16 (comment)).

Almost 1 year later, it seems reasonable to me to look again at the updated w3c draft and verify if it has been updated as expected or if fixing #16 is something that would be worth to re-evaluate.

Unfortunately it seems that, while the w3c draft has been updated to the Promise-based API (more or less defined as implemented in Firefox natively, and by this polyfill where it is not supported natively, with some exceptions e.g. the pageAction.show/hide in the w3c draft do not seem to return any Promise, at least not yet), but the sendResponse callback seems to be still part of the proposal and so the behavior implemented by this polyfill is still going to be confusing for a lot of the developers, especially because the API docs on MDN (as well as the API docs from Chrome) still mention the usage of the sendResponse callback.

Also, by looking into the w3c draft, it seems that sendResponse callback has been even updated to mention that it is "an asynchronous function that returns a Promise" (from: https://browserext.github.io/browserext/#dom-browserextbrowsertabs-sendmessage), which is not actually true even in Firefox where the Promise-based APIs are natively supported.

If at some point the w3c draft will actually be updated to remove the sendResponse callback, we can plan the updates to apply on the polyfill accordingly (e.g. as a first step we may log a warning message when the sendResponse callback is called and points the developer to an updated doc page, and then as a second step we may raise an error every time the sendResponse callback is called and then remove sendResponse completely in a third step, or just removing the sendResponse completely as the second step).

[Rob--W]

Why did you remove error handling?

[rpl]

Well, I did it because it doesn't seem that it actually reproduce the expected behavior (which should be to make the promise returned by the runtime.sendMessage, or tabs.sendMessage, on the other side to be rejected), but it adds an additional unexpected behavior: the rejected value is sent to the other side as the resolved value of the promise returned by sendMessage, which will be an empty object if the error was an instance of Error, or the rejected value if it is a plain object that can be successfully send over the messaging abstraction).

I guess that it could be safer to leave it there for now (given that it is how the polyfill currently behave) and evaluate if we can make it more similar to what we need in a follow up.

@Rob--W How that sounds to you?

As an additional side note: the only way I can think of right now to make it to behave more similar to how it does on Firefox is to add an additional custom static wrapper for the runtime.sendMessage/tabs.sendMessage side, so that we can send a "json representation" of the resolve and reject scenarios (and so basically turning the Error object into a plain json object and convert it back to an error on the other side) and reject the promise returned by runtime.sendMessage/tabs.sendMessage accordingly, e.g. something like this on the onMessage wrapper side:

          result.then(msg => {
            // send the message value.
            sendResponse({msg});
          }, error => {
            console.error(error);
            // Send a JSON representation of the error if the rejected value
            // is an instance of error, or the object itself otherwise.
            sendResponse((error instanceof Error) ? {
              error: {
                isInstanceOfError: true,
                message: String(error),
                fileName: error.fileName,
                lineNumber: error.lineNumber,
              },
            } : {error});

and then on the wrapped sendMessage side something like:

return new Promise((resolve, reject) => {
   const wrappedCallback = {msg, error}) => {
     if (error) {
       // Convert the JSON representation of the error back to an Error instance,
       // otherwise just reject with the error value as it is.
       if (error.isInstanceOfError) {
         reject(new Error(error.message, error.fileName, error.lineNumber));
       } else {
         reject(error);
       }
     } else {
       resolve(msg);
     }
  });

  ....
});

This has clearly its own drawbacks, e.g. if on the other side the extension is mixing the browser and chrome API namespaces (e.g. using the native chrome.runtime.sendMessage instead of the wrapped one) or if the message is being send to another extension (that may be not be using the polyfill).

[Rob--W]

The error should be a rare case. So you could also just pass the message as-is, and wrap the error in an object with a very unusual key that is unlikely to conflict with a real message. If isInstanceOfError fits that bill, then use that. I would be more inclined to use a more verbose string from which it is obvious that the polyfill is the source.

[ExE-Boss]

Perhaps something like __mozWebExtPolyfill_isSerializedErrorObject could work, because isInstanceOfError is likely to be used by something.

The double-_ prefix is used since it’s somewhat common practice to use __comment in place of real comments in JSON, and a single-_ prefix already denotes an internal varible.

The moz prefix is also used by non-standard Mozilla properties on Web APIs, and WebExtPolyfill identifies this polyfill.

[Rob--W]

s/on returned Promise/on returned false/.

[rpl]

Thanks for catching it, it will make easier to read the failure message if it regress here.

[Rob--W]

s/on returned Promise/on non-boolean/Promise return value/ .

[rpl]

same as above, thanks!

[Rob--W]

If someone calls sendResponse with a Promise, the promise will be unwrapped. If that promise is rejected, then we end up in a dead spot here. Is that the desired behavior?

[rpl]

good point, I think that we should at least do the same of what we do for the returned promise here.

[Rob--W]

There is another significant, likely wrong change.
Now sendResponse is called unconditionally. This causes issues when there are multiple recipients to a message, where only one is expected to send a message.

The logic should be:

• If sendResponse was called before returning, send the response.
• Otherwise if the return value is true, wait for sendResponse.
• Otherwise if the return value is a promise, handle it (call sendResponse upon promise resolution).
• Otherwise, don't call sendResponse. You can either return the value as-is, or not return anything (undefined).

[rpl]

@Rob--W There is a return false at line 382 which is meant to tell to Chrome that the listener is not going to send any response when the result is not true, is not a thenable and the wrappedSendResponse callback has not already been called by the listener.

After that return false the wrapped listener is supposed to send a response, from the returned promise if any, otherwise it means that sendResponse has been called synchronously or the return value is true.

Is there is any additional test case that I may add to the smoke test that is running on a real chrome extension to catch any other scenario that may be missing from your point of view?
(besides the one related to the "rejected promise" scenarios, that at this point it seems better to move in a follow up given that the issue was already there, even without the changes added here for the sendResponse callback support).

[Rob--W]

Oops, I stand corrected.

There is no test coverage for scenarios with more than 1 recipient. This can be tested either via an all_frames content script in a tab, or with another extension frame in the background page.

Scenarios:

• Neither replies.
• One replies, the other does not.
• Both reply (maybe with the same value, since the order of receiving messages is unspecified?).

For full coverage, try to run the test twice, once with sendResponse and again with Promise.

[Rob--W]

There is no unit test for sendResponse with a promise. If it's easy to add, do so, otherwise never mind.

[Rob--W]

Oops, I stand corrected.

There is no test coverage for scenarios with more than 1 recipient. This can be tested either via an all_frames content script in a tab, or with another extension frame in the background page.

Scenarios:

• Neither replies.
• One replies, the other does not.
• Both reply (maybe with the same value, since the order of receiving messages is unspecified?).

For full coverage, try to run the test twice, once with sendResponse and again with Promise.

[Rob--W]

(also on line 397)
catch(console.error) or err => console.error(err) . Not err => console.error.

Chrome logs uncaught promise rejections to the console, so you don't even need to call console.error here.

Side note: The polyfill is quite chatty. Chrome is generally quiet about messages in onMessage / sendMessage, unless an obviously incorrect error occurs (e.g. calling sendResponse without return true;). Firefox doesn't print rejected promises either, it just forwards the promise result to the sendMessage caller.

An error is usually an indication that something is wrong, so I can see value in preserving the console.error call. To not change the behavior too much,

Usually error messages are not even serializable (they become {} in Chrome), so perhaps it would be better to just invoke sendResponse without any value (undefined).

[jaredhirsch]

@rpl 👋 Hey! Let me know if I can help get this over the finish line 🏁

[rpl]

I've merged this PR as it is.

I've spent some time experimenting on "how to run the smoke tests on multiple browsers", so that we can more easily compare the "behaviors implemented by the polyfill" with the ones "provided by the native promised APIs on Firefox", and also in a way more reliable then "let's manually try a test extension on both the browsers and then manually verify the different behaviors, if any", but the resulting change is big enough to need its own pull request.

In particular I'd like to run these "smoke tests" at least on Chrome and Firefox for a start (Edge should also be theoretically possible, but I've not been yet able to convince webdriver to install a test extension):
the usual expectation of the users of this polyfill is that it is going to behave as close as possible as" the promised APIs do behave natively on Firefox", and the only reliable way to be sure of that is running the exact same test on both.

Running the same test across the browsers is also going to be useful to catch and document differences (when there is no way to provide exactly the same behavior), and test the expected behaviors for regressions on every pull request (on both Chrome, in case the polyfill and/or chrome regress, and Firefox, in case the native behaviors have been changed in the meantime).

I'm going to include, into the pull request which introduces the "multi browser smoke tests", also the changes needed to implement the behavior on "returned rejected promise from the onMessage listeners", which is one of those things that really need to be explicitly verified on both the browsers to be able to ensure that the implemented behaviors are as close as possible to the ones natively provided by the Firefox promised APIs.