1220154 - chrome.runtime.lastError should be set when sending unhandled content script message

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Markus Hartung]

Attachment: content.js

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

Send a message from a content script that is not handled by the background.


Actual results:

response callback is not called


Expected results:

response callback should be called and chrome.runtime.lastError should be set to {message: "Could not establish connection. Receiving end does not exist."}

Comment 1

[Michelle Funches - QA]

Changing Status to New

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1220154, 1249830: Handle sendMessage replies with 0 and >1 listeners correctly. r?billm

Review commit: https://reviewboard.mozilla.org/r/38359/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38359/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8727035 [details]
MozReview Request: Bug 1220154, 1249830: Handle sendMessage replies with 0 and >1 listeners correctly. r?billm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38359/diff/1-2/

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8727035 [details]
MozReview Request: Bug 1220154, 1249830: Handle sendMessage replies with 0 and >1 listeners correctly. r?billm

https://reviewboard.mozilla.org/r/38359/#review35195

::: toolkit/components/extensions/MessageChannel.jsm:8
(Diff revision 2)
>   * This module provides wrappers around standard message managers to
>   * simplify bidirectional communication. It currently allows a caller to
>   * send a message to a single listener, and receive a reply. If there
>   * are no matching listeners, or the message manager disconnects before
>   * a reply is received, the caller is returned an error.
>   *
>   * Since each message must have only one recipient, the listener end may
>   * specify filters for the messages it wishes to receive, and the sender
>   * end likewise may specify recipient tags to match the filters.

This needs to be updated.

::: toolkit/components/extensions/MessageChannel.jsm:37
(Diff revision 2)
>   *      this.messageFilter = {

Need to document listenerInfo as well. Also, listenerInfo is not a great name. How about messageFilterPermissive (for listenerInfo) and messageFilterStrict (for messageFilter)?

::: toolkit/components/extensions/MessageChannel.jsm:185
(Diff revision 2)
> -   *     property on which to filter messages. Final dispatching is handled
> +   *     property on which to filter messages.

Can you also mention the listenerInfo?

::: toolkit/components/extensions/MessageChannel.jsm:474
(Diff revision 2)
> +    let responseType = options.responseType;

Can you make the default be RESPONSE_SINGLE here rather than in the receiver? Seems cleaner to me.

::: toolkit/components/extensions/MessageChannel.jsm:517
(Diff revision 2)
> +      return new Promise(resolve => {
> +        resolve(handlers[0].receiveMessage(data));

Promise.resolve?

::: toolkit/components/extensions/MessageChannel.jsm:522
(Diff revision 2)
> +    let responses = handlers.map(handler => handler.receiveMessage(data))
> +                            .filter(response => response !== undefined);

What do you think the behavior should be if one handler throws? I think we should guarantee to invoke all the listeners but to return an error to the caller. I wonder what Chrome does though.

::: toolkit/components/extensions/MessageChannel.jsm:532
(Diff revision 2)
> +        return Promise.race(responses);

We need to be careful here. Our promise API is not really well-defined right now, but we need to figure out what it is. What if onMessage returns a promise that eventually resolves to undefined? Does that count as a response that should be returned to the sender? It will be here. That might be fine, but I just wanted to call it out.

::: toolkit/components/extensions/MessageChannel.jsm:548
(Diff revision 2)
> -  _handleMessage({handler, error}, data) {
> +  _handleMessage({handlers}, data) {

Not sure why you're passing handlers in an object rather than an ordinary param.

Comment 5

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/38359/#review35195

> Need to document listenerInfo as well. Also, listenerInfo is not a great name. How about messageFilterPermissive (for listenerInfo) and messageFilterStrict (for messageFilter)?

Yeah, that would make more sense.

> Promise.resolve?

Doing it this way, any errors thrown by `receiveMessage` automatically become rejected promises. With `Promise.resolve`, they'd just throw. I'll add a comment.

> What do you think the behavior should be if one handler throws? I think we should guarantee to invoke all the listeners but to return an error to the caller. I wonder what Chrome does though.

Yeah, that's a good point. I agree.

I think Chrome just ignores errors. I'll check again, but ignoring errors doesn't seem like the right behavior to me.

> We need to be careful here. Our promise API is not really well-defined right now, but we need to figure out what it is. What if onMessage returns a promise that eventually resolves to undefined? Does that count as a response that should be returned to the sender? It will be here. That might be fine, but I just wanted to call it out.

I think that returning a promise that resolves to undefined should count as a response. At least that has the benefit of letting handlers unambiguously respond with `undefined` if they need to. Maybe there are use cases for letting them return promises that resolve to undefined if they don't want to respond, that I haven't thought of.

I'll document that as the expected behavior.

> Not sure why you're passing handlers in an object rather than an ordinary param.

Yeah, I guess it doesn't make sense anymore.

Comment 6

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/557e8160f3518163e4d734852f376bc17da0a9b0
Bug 1220154, 1249830: Handle sendMessage replies with 0 and >1 listeners correctly. r=billm

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/557e8160f351

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/41a9ead06eee530634b596df6e6738fb38505bf0
Bug 1220154: Follow-up: Fix ESLint error in test. r=me

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41a9ead06eee