Closed Bug 1220154 Opened 4 years ago Closed 4 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P2)

45 Branch
defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
47.3 - Mar 7
Tracking Status
firefox48 --- fixed

People

(Reporter: markus.hartung, Assigned: kmag)

References

()

Details

(Whiteboard: [runtime])

Attachments

(2 files)

Attached file 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."}
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Depends on: 1190680
Summary: Support chrome.runtime.lastError → chrome.runtime.lastError should be set when sending unhandled content script message
Changing Status to New
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-webextensions?
Whiteboard: [runtime]
Flags: blocking-webextensions? → blocking-webextensions+
Priority: -- → P2
Assignee: nobody → kmaglione+bmo
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/
Iteration: --- → 47.3 - Mar 7
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.
Attachment #8727035 - Flags: review?(wmccloskey) → review+
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.
https://hg.mozilla.org/integration/fx-team/rev/557e8160f3518163e4d734852f376bc17da0a9b0
Bug 1220154, 1249830: Handle sendMessage replies with 0 and >1 listeners correctly. r=billm
https://hg.mozilla.org/mozilla-central/rev/557e8160f351
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1249830
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.