runtime.onMessage is broken

[eight04]

Error in event handler for runtime.onMessage: TypeError: sendResponse is not a function
    at runtime.onMessage.addListener (chrome-extension://oommdnlanbpnfobibbnfmnmobfddkofp/background.js:4:2)
    at onMessage (chrome-extension://oommdnlanbpnfobibbnfmnmobfddkofp/browser-polyfill.js:1338:22)

https://github.com/mozilla/webextension-polyfill/blob/master/src/browser-polyfill.js#L314-L328
I'm not sure this is due to API changes or something. The wrapper of listener looks completely nonsense to me. Shouldn't it just invoke the listener directly?

      return function onMessage(message, sender, sendResponse) {
        return listener(message, sender, sendResponse);
      };

[kumar303]

Are you using the compiled source?

[eight04]

https://github.com/eight04/ansi-viewer/blob/chrome/polyfill/browser-polyfill.js#L826
Which is compiled and copied from dist/browser-polyfill.js and patched by me.

[kumar303]

ok, just checking. This has created confusion: #5

[joncu01]

I'm seeing this problem too. Is there a fix on the way?

[eight04]

Currently I patch it myself.
https://github.com/eight04/ansi-viewer/blob/8f6d33f6ee8713ca29d00075c94e9fff89ba04e9/lib/browser-polyfill.js#L828

[collinsauve]

Just spent a good hour figuring this out for myself. Unfortunately different documentation on MDN seems to disagree how this this is supposed to work:

runtime.onMessage (which should probably be considered the source of truth) shows this working with a sendResponse callback, returning a boolean indicating if the callback will be invoked asynchronously.

function handleMessage(request, sender, sendResponse) {  
  console.log(`content script sent a message: ${request.content}`);
  setTimeout(() => {
    sendResponse({response: "async response from background script"});
  }, 1000);  
  return true;
}

browser.runtime.onMessage.addListener(handleMessage);

tabs.sendMessage shows this working by returning a promise.

browser.runtime.onMessage.addListener(request => {
  console.log("Message from the background script:");
  console.log(request.greeting);
  return Promise.resolve({response: "Hi from content script"});
});

[collinsauve]

@kmaglione please see my above note about MDN discrepancy. It looks like you wrote this code and added the example in the MDN tabs.sendMessage. Can you clarify if this should be a callback or a promise?

[Rob--W]

FYI I submitted a PR at #22.

[collinsauve]

Also added PR #23 on top of #22, which allows multiple async/promise-based handlers as long as only 1 returns a result.

[Treeeater]

@eight04 's fix works, however, I slightly modified his fix so that it doesn't touch the Firefox side (retains isThenable part)

  return function onMessage(message, sender, sendResponse) {
    let result = listener(message, sender, sendResponse);

    if (isThenable(result)) {
      result.then(sendResponse, error => {
        console.error(error);
        sendResponse(error);
      });

      return true;
    } else if (result !== undefined) {
      //sendResponse(result);
      return result;
    }
  };

[carlin-q-scott]

So... is someone going to merge the PR?

[rpl]

My apologies for the late reply, I deferred closing this issue in the last bug triage meeting because I wanted to discuss it again with @kmaglione (mainly to collect additional details and then be able to provide a more complete comment here).

While discussing this issue with @kmaglione it did come out that the polyfill implementation of the runtime.onMessage API doesn't provide the sendResponse callback on purpose:
the reason is that the sendResponse callback is going to be removed from the W3C specs (https://browserext.github.io/browserext/#runtime, the callback is still listed in the current spec draft, but as far as I know, mainly what I did learn from the discussion with Kris, its removal has been discussed and it is going to be removed).

We agreed that this "incompatibility", between the current Firefox implementation and the W3C spec for this API event, should be better highlighted and explained in the API documentation on MDN to prevent confusion about the implementation in the polyfill.

[eight04]

So is sendResponse deprecated? I thought request-response is a common pattern.

[collinsauve]

@rpl can you expand on what the expected usage in WebExtensions be? I have some guesses as to what you might end up with, is the following correct?

• Any number of listeners can:
  • return undefined to indicate no result (no response sent back to sender)
• Only 1 listener is allowed to do 1 of:
  • return a non-Promise to indicate synchronous result
  • return a Promise to indicate asynchronous result.

This means that:

• no more callback argument
• no more returning true to indicate asynchronous result
• a promise that resolves to undefined asynchronously sends an undefined result

[TomerHeber]

@rpl thank you for the explanation. Is it still true?

Looking in the draft I noticed that it's still:
callback BrowserExtRuntimeOnMessageCallback = void (optional any message,
BrowserExtRuntimeMessageSender sender,
BrowserExtRuntimeSendResponseCallback callback);

Though nothing in the draft for BrowserExtRuntimeSendResponseCallback.

Edit:
When using polyfill in Chrome return "value" works.
When "using polyfill" in Firefox return "value" does not work, instead BrowserExtRuntimeSendResponseCallback is defined and must be used.

Therefore, it's not just a documentation issue, but an inconsistent behavior of the polyfill script.

[jaredhirsch]

@rpl Hi, I'm confused by how this issue has been handled. Could we revisit this decision?

As I understand it, this polyfill exists to make W3C-compatible (i.e. Firefox) webextensions compatible with Chrome.

Oddly, the decision here was to deliberately break the polyfilled behavior in Chrome, because Chrome's implementation didn't match planned changes to the W3C spec. That's the opposite of what this library is supposed to do. There's even a working PR that was closed.

Meanwhile, close to a year later, this is still a bug with this library in the latest version of Chrome (I ran into it today).

How about, instead, landing the PR to fix this bug, then filing a followup bug to update the polyfill if/when Chrome updates its implementation?

[eight04]

@6a68 With the polyfill, you can only send the response by returning a promise.

Here is what I do now:

1. There is no sendResponse function when using the polyfill.
2. To send a response, return a promise in the handler.
3. If there is no response but the message is handled, return false. Never return true or Firefox would expect you to call sendResponse function.
4. If the message is not handled, return undefined.

I think the proper "fix" to this issue is to remove/deprecate (since it is going to be removed) sendResponse on MDN and add a warning about the current state of polyfilled environment. Currently, the polyfill disagrees with MDN, which is confusing.

Also, I found that the document had been updated and the usage of the promise is now included (which was not when the issue was filed):
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/onMessage$compare?to=1204311&from=1178671

[Juraj-Masiar]

So is somebody gonna edit the MDN?
I will do it myself if nobody else will. The question is whether I mark the sendResponse deprecated or just add a NOTE about future plans of W3C and about current behavior of popular webextension-polyfill library.

EDIT:
One more thing, what about similar browser.runtime.onMessageExternal (MDN )? Does it work with polyfill and does it share the same destiny?

EDIT 2:
MDN is now updated with warning block.

[fosskers]

Hi there from the future. Since this is the issue linked to from MDN, I decided to post my discovery here. For any Rust folks trying to bind to browser.runtime.onMessage.addListener properly, here's how to do it:

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(js_namespace = ["browser", "runtime"], js_name = "sendMessage")]
    pub async fn send_message(message: JsValue) -> JsValue;

    #[wasm_bindgen(js_namespace = ["browser", "runtime", "onMessage"], js_name = "addListener")]
    pub fn add_listener(handle: &Closure<dyn Fn(JsValue, JsValue) -> Promise>);
}

For the pub async fn send_message, make sure you have wasm-bindgen-futures as a dependency. It auto-converts JS Promises into Rust's async system.

Notice that we're only accepting two arguments to the closure passed to add_listener. We completely ignore the sendMessage argument and force a Promise to be returned, as per the MDN docs.

Downstream in our background script, we can add the listener like so:

    let handle_msg = Closure::wrap(Box::new(move |msg, sender| {
        Promise::resolve(&JsValue::from_str("From background!"))
    }) as Box<dyn Fn(JsValue, JsValue) -> Promise>);
    add_listener(&handle_msg); // The binding we just wrote.
    handle_msg.forget();

You're free to do pretty much whatever you want inside the Box'd closure, including moving in Rust structures, write log messages, etc. As long as a Promise is the return value. The final forget is also critical to ensure that the closure isn't dropped on the Rust side before it has a chance to be used on the JS side.

Cheers!