1270360 - Implement runtime.sendNativeMessage

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

https://developer.chrome.com/extensions/runtime#method-sendNativeMessage

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1270360 Implement runtime.sendNativeMessage()

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

https://reviewboard.mozilla.org/r/59400/#review56506

Looks good. Thanks!

::: toolkit/components/extensions/NativeMessaging.jsm:364
(Diff revision 1)
> +
> +  sendMessage(msg) {
> +    let responsePromise = new Promise((resolve, reject) => {
> +      let msgListener, disconnectListener;
> +
> +      msgListener = (what, msg) => {

Please add a `let` here rather than predeclaring this.

::: toolkit/components/extensions/NativeMessaging.jsm:371
(Diff revision 1)
> +        this.off("disconnect", disconnectListener);
> +        resolve(msg);
> +      };
> +      disconnectListener = (what, err) => {
> +        this.off("message", msgListener);
> +        this.off("disconnect", disconnectListener);

I don't think there's a need to remove these listeners. Just disable the balanced listeners rule for this scope, and then we can just use inline arrow functions for the listeners.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:251
(Diff revision 1)
> +    }, err => {
> +      browser.test.succeed("sendNativeMessage() to a nonexistent app failed");
> +    });
> +
> +    // Check regular message exchange
> +    browser.runtime.sendNativeMessage("echo", MSG).then(reply => {

We should chain this to the last listener, so we're sure that the promise either resolves or rejects.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:269
(Diff revision 1)
> +  }, ID);
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("finished");
> +
> +  // with sendNativeMessage(), the subprocess should be disconnected

Please capitalize and end with full stop.

Comment 3

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59400/#review56506

> I don't think there's a need to remove these listeners. Just disable the balanced listeners rule for this scope, and then we can just use inline arrow functions for the listeners.

I think we do need to remove them.  If the native app happens to send two messages back-to-back or do something to trigger an error after sending a message, then we'll end up calling `resolve()` or `reject()` on a promise that is already settled.

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59400/#review56506

> I think we do need to remove them.  If the native app happens to send two messages back-to-back or do something to trigger an error after sending a message, then we'll end up calling `resolve()` or `reject()` on a promise that is already settled.

It doesn't matter. Calling resolve or reject has no effect after the first time one of them is called.

Comment 5

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/59400/#review56506

> It doesn't matter. Calling resolve or reject has no effect after the first time one of them is called.

Hm, is that documented behavior?  I went looking for it on MDN and couldn't find anything definitive.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

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

Comment 7

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/59400/#review56506

> Hm, is that documented behavior?  I went looking for it on MDN and couldn't find anything definitive.

Yes[1]: "Attempting to resolve or reject a resolved promise has no effect."

[1]: http://www.ecma-international.org/ecma-262/6.0/#sec-promise-objects

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8763065 [details]
Bug 1270360 Implement runtime.sendNativeMessage()

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5592e9d2a86e
Implement runtime.sendNativeMessage() r=kmag

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5592e9d2a86e

Comment 11

[Will Bamberg [:wbamberg]]

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/sendNativeMessage
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging