runtime.sendMessage does not handle the three-argument form correctly.

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
a year ago
17 days ago

People

(Reporter: michael.oneill, Assigned: aswan)

Tracking

({dev-doc-complete})

48 Branch
mozilla55
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [runtime]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:

I had to change a call to chrome.runtime.sendMessage from: 
    //
    // Chrome version
    //chrome.runtime.sendMessage(properties, {}, function (originContext) {
to:

// Firefox version
    chrome.runtime.sendMessage(undefined, properties, undefined, function (originContext) {


   I also had to check for "??" returned from chrome.i18n.getMeaage:

// Chrome version
//function T(text) {
//    return chrome.i18n.getMessage(text.startsWith("__MSG__")?text.substring(7):text);
//}

// Firefox version (works)
function T(text) {
    var s = text.startsWith("__MSG__") ? text.substring(7) : text;
    var locs = chrome.i18n.getMessage(s);
    if (locs == "??")
        return s;
    return locs;
}


Actual results:

the call to sendMessage failed, no error message
the call to i18n.getMessage reyurned "??", the Chrome one returns the input string
(In reply to michael.oneill from comment #0)
>    I also had to check for "??" returned from chrome.i18n.getMeaage:
> 
> // Chrome version
> //function T(text) {
> //    return
> chrome.i18n.getMessage(text.startsWith("__MSG__")?text.substring(7):text);
> //}
> 
> // Firefox version (works)
> function T(text) {
>     var s = text.startsWith("__MSG__") ? text.substring(7) : text;
>     var locs = chrome.i18n.getMessage(s);
>     if (locs == "??")
>         return s;
>     return locs;
> }
> 
> 
> Actual results:
> 
> the call to sendMessage failed, no error message
> the call to i18n.getMessage reyurned "??", the Chrome one returns the input
> string

This is bug 1258199. We're going to change the return value for missing keys,
but we're not planning to remove the reported error. I suggest that you
looking up keys that you don't expect to exist.
Summary: I found 2 issues porting a blink extension to Firefox webextensions → runtime.sendMessage does not handle the three-argument form correctly.
Whiteboard: [runtime]
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

a year ago
Priority: -- → P3
Whiteboard: [runtime] → [runtime]triaged

Updated

8 months ago
Component: WebExtensions: Untriaged → WebExtensions: General
(Assignee)

Updated

3 months ago
webextensions: --- → ?

Updated

3 months ago
webextensions: ? → +

Updated

3 months ago
Assignee: nobody → aswan
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1341073
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8864296 - Flags: review?(tomica)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8864296 [details]
Bug 1259944 Fix runtime.sendMessage() handling of 2 arguments

https://reviewboard.mozilla.org/r/135920/#review139046

This API is just crazy, sadly ambiguous:

    browser.runtime.sendMessage("sendMessage1@tests.mozilla.org", {});    // WTF?
    
and unfortunately, Chrome implementation disagrees with yours [1].  I think we should try to disambiguate further by:
 - checking if the first argument is a string (maybe even in the right extensionId format?),
 - making the `toProxyScript` non-optional, so that we limit the possible legal values of the second argument.

Also, I would appreciate an explicit comment calling out how we are dealing with this ambiguity.

1) https://cs.chromium.org/chromium/src/extensions/renderer/messaging_utils_unittest.cc?l=155-164

::: toolkit/components/extensions/ext-c-runtime.js:26
(Diff revision 1)
>  
>            return context.messenger.connect(context.messageManager, name, recipient);
>          },
>  
> -        sendMessage: function(...args) {
> +        sendMessage(...args) {
>            let options; // eslint-disable-line no-unused-vars

doesn't seem necessary anymore.

::: toolkit/components/extensions/ext-c-runtime.js:59
(Diff revision 1)
>              message = args[0];
>            } else if (args.length === 2) {
> -            if (typeof args[0] === "string" && args[0]) {
> -              [extensionId, message] = args;
> +            try {
> +              checkOptions(args[1]);
> -            } else {
>                [message, options] = args;

This is "using exceptions as flow control" anti-pattern: it is throwing in non-exceptional, normal or expected circumstances, which can impact  readability and performance.  It seems simple enough to avoid, so we should.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_args.js:52
(Diff revision 1)
> +    equal(result.msg, msg, "Received internal message");
> +    equal(result.sender.id, ID1, "Received correct sender id");
> +  }
> +
> +  // Check that a message was sent from extension1 to extension2
> +  // to the other extension.

nit: reduntant
Attachment #8864296 - Flags: review?(tomica) → review-
(Assignee)

Comment 5

2 months ago
mozreview-review-reply
Comment on attachment 8864296 [details]
Bug 1259944 Fix runtime.sendMessage() handling of 2 arguments

https://reviewboard.mozilla.org/r/135920/#review139046

I agree the api is insane but I believe the existing code (http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/toolkit/components/extensions/ext-c-runtime.js#35-40) does follow Chrome's behavior and I think that is a mistake.
In particular, the case of `sendMessage("some string", {});` to send a message within the extension with no options, seems much more common than sending an empty object to anther extension but the "if the first arg is a string" rule gets this case wrong.

I don't think making `toProxyScript` mandatory is a good idea, it would likely break existing extensions and it would break compatibility with chrome (and anybody else who follows the browserext standard)

I'll address the other comments though, revision coming shortly
Comment hidden (mozreview-request)

Comment 7

2 months ago
mozreview-review
Comment on attachment 8864296 [details]
Bug 1259944 Fix runtime.sendMessage() handling of 2 arguments

https://reviewboard.mozilla.org/r/135920/#review139668

I agree about use-cases, so we need to make sure to document this and at least call it out in a blog post, since we don't exactly have "release notes".

> I don't think making `toProxyScript` mandatory is a good idea, it would likely break [...]
Sure, though your explicit disallowing of any `options` keys other than `toProxyScript` will already break some.  The only difference is when calling with an explicit empty options object, which I don't know if/why anyone would do.

Anyway, r=me with the one additional test.

::: toolkit/components/extensions/ext-c-runtime.js:37
(Diff revision 2)
> +            let toProxyScript = false;
> +            if (typeof options !== "object") {
> +              return [false, "runtime.sendMessage's options argument is invalid"];
> +            }
> +
> +            for (let key of Object.keys(options)) {

nit:  There are only two valid cases for the options object: 1) no keys, or 2) exactly one key named "toProxyScript", so `for of` looks a bit overkill here.

::: toolkit/components/extensions/ext-c-runtime.js:49
(Diff revision 2)
> +              } else {
> +                return [false, `Unexpected property ${key}`];
> +              }
> +            }
> +
> +            return [true, {toProxyScript}];

nit:  This returns an object with exactly the same value as `options` if it is valid, so it can be simplified.  You can maybe even flip it to just return the error message or `null` when valid.

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_args.js:73
(Diff revision 2)
> +  extension1.sendMessage(ID2, "message");
> +  await checkRemoteMessage("message");
> +
> +  extension1.sendMessage("message", {});
> +  await checkLocalMessage("message");
> +

If we are going with this change in behavior, we should also test it explicitly:

    extension1.sendMessage(ID2, {mgs: "message"});
Attachment #8864296 - Flags: review?(tomica) → review+
(Assignee)

Comment 8

2 months ago
mozreview-review-reply
Comment on attachment 8864296 [details]
Bug 1259944 Fix runtime.sendMessage() handling of 2 arguments

https://reviewboard.mozilla.org/r/135920/#review139668

> I don't think making `toProxyScript` mandatory is a good idea, it would likely break [...]
Sure, though your explicit disallowing of any `options` keys other than `toProxyScript` will already break some.  The only difference is when calling with an explicit empty options object, which I don't know if/why anyone would do.

Fair, but any extension that was passing other keys in options was already arguably broken (schema-validated apis fail when they are passed unknown options, this bring `sendMessage()` in line with everything else).
As for why anybody would pass an empty options object, I dunno but its what the original reporter of this bug was doing...

> nit:  There are only two valid cases for the options object: 1) no keys, or 2) exactly one key named "toProxyScript", so `for of` looks a bit overkill here.

Sorry i don't undersatnd what you're proposing instead.
Also it seems quite possible that new options will get added at some point, which will be a lot simpler with this structure.

> If we are going with this change in behavior, we should also test it explicitly:
> 
>     extension1.sendMessage(ID2, {mgs: "message"});

Good idea, added
Comment hidden (mozreview-request)

Comment 10

2 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70db5f9c852d
Fix runtime.sendMessage() handling of 2 arguments r=zombie

Comment 11

2 months ago
mozreview-review-reply
Comment on attachment 8864296 [details]
Bug 1259944 Fix runtime.sendMessage() handling of 2 arguments

https://reviewboard.mozilla.org/r/135920/#review139668

> Sorry i don't undersatnd what you're proposing instead.
> Also it seems quite possible that new options will get added at some point, which will be a lot simpler with this structure.

I meant something like:

    let keys = Object.keys(options);
    if (keys.length && keys[0] !== "toProxyScript" || keys.length > 1) {
        return `Invalid options properties: ${keys}`;
    }
    if (keys.length && typeof options.toProxyScript !== "boolean") {
        return `options.toProxyScript is invalid...`;
    }

As for possible new options at some point: "YAGNI" applies, since this is local code, not a method/class.
Keywords: dev-doc-needed

Comment 12

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70db5f9c852d
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1368669
I've added a bit at the end of the "Parameters" section that tries to explain this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendMessage#Parameters.

Please let me know if it looks correct. I'm a bit unclear  on the Chrome behavior here - the link at https://cs.chromium.org/chromium/src/extensions/renderer/messaging_utils_unittest.cc?l=155-164 suggests that the Chrome behavior is the same as the old Firefox behavior, but the original bug suggest it isn't.
Flags: needinfo?(aswan)
(Assignee)

Comment 14

17 days ago
That looks good to me.  Note that bug 1368669 will change the underlying code a little more...
Flags: needinfo?(aswan)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.