Closed
Bug 1259944
Opened 9 years ago
Closed 8 years ago
runtime.sendMessage does not handle the three-argument form correctly.
Categories
(WebExtensions :: General, defect, P3)
Tracking
(firefox55 fixed)
People
(Reporter: michael.oneill, Assigned: aswan)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [runtime]triaged)
Attachments
(1 file)
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
Comment 1•9 years ago
|
||
(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]
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•9 years ago
|
Priority: -- → P3
Whiteboard: [runtime] → [runtime]triaged
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: General
Assignee | ||
Updated•8 years ago
|
webextensions: --- → ?
Updated•8 years ago
|
webextensions: ? → +
Updated•8 years ago
|
Assignee: nobody → aswan
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8864296 -
Flags: review?(tomica)
Comment 4•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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.
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 13•8 years ago
|
||
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•8 years ago
|
||
That looks good to me. Note that bug 1368669 will change the underlying code a little more...
Flags: needinfo?(aswan)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•