Closed Bug 1299411 Opened 8 years ago Closed 8 years ago

Port.onDisconnect does not indicate an error

Categories

(WebExtensions :: Untriaged, defect, P2)

Product:
WebExtensions
Component:
Untriaged
Version:
47 Branch
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: zhongyzh, Assigned: robwu)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(10 files, 1 obsolete file)

Bug 1299411 - Decouple Port implementation from API
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1299411 - Pass port parameter to port.onMessage
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1299411 - Unify fire and fireWithoutClone.
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1299411 - Propagate errors to port.onDisconnect via port.error
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1299411 - separate serialization from sending
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1299411 - Deduplicate context getter logic in ParentAPIManager
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1299411 - Remove extension param from NativeApp
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1299411 - s/on/once/ in NativeApp's sendMessage
58 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1299411 - Move native messaging to child process
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1299411 - Error messages for native messaging
58 bytes, text/x-review-board-request
aswan
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

chrome.runtime.connectNative
At onDisconnect 


Actual results:

chrome.runtime.lastError  is null


Expected results:

chrome.runtime.lastError should not null and chrome.runtime.lastError.message has the reason
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
An early version of the native messaging implementation did this but it got removed.
Kris, you said it was undocumented and argued for making it a parameter to the handler instead but looking at the Chrome documentation, it is documented: https://developer.chrome.com/extensions/runtime#type-Port
Flags: needinfo?(kmaglione+bmo)
Summary: chrome.runtime.lastError is null → native Port.onDisconnect does not indicate an error
Whiteboard: [native messaging]
(In reply to Andrew Swan [:aswan] from comment #1)
> An early version of the native messaging implementation did this but it got
> removed.
> Kris, you said it was undocumented and argued for making it a parameter to
> the handler instead but looking at the Chrome documentation, it is
> documented: https://developer.chrome.com/extensions/runtime#type-Port

At the time, it was not: http://web.archive.org/web/20160423060505/https://developer.chrome.com/extensions/runtime
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: [native messaging] → [native messaging]triaged
Updating the title to make sure we cover this for every type of Port, not just native messaging ports.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: native Port.onDisconnect does not indicate an error → Port.onDisconnect does not indicate an error
Whiteboard: [native messaging]triaged → triaged
This is happening as a side effect of Rob's work on bug 1287007.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Not exactly. But I will attach a patch (that extends the changes from bug 1287007) to implement it (separate from the other bug because the required changes are not trivial and I don't want to block the other patch on this).
Assignee: aswan → rob
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment on attachment 8791455 [details]
Bug 1299411 - Decouple Port implementation from API

https://reviewboard.mozilla.org/r/78866/#review77598

Can you split the fire() handling stuff into a separate patch?  And have Kris take a look at that one, I think he had some plans in this area, I want to make sure we're consistent with those...
Depends on: 1287007
Comment on attachment 8791455 [details]
Bug 1299411 - Decouple Port implementation from API

https://reviewboard.mozilla.org/r/78866/#review80024

I'm not all that familiar with the existing code, and scratching my head a bit about unregisterMessageFuncs.  At first glance, it looks like an overly general solution to a specific problem.  But it also looks like its intended to fix an existing bug in which mm listeners don't get removed when a port is disconnected?  If that's true, is it feasible to add a unit test that covers that scenario?
Comment on attachment 8794518 [details]
Bug 1299411 - separate serialization from sending

https://reviewboard.mozilla.org/r/80944/#review80030

::: toolkit/components/extensions/NativeMessaging.jsm:8
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  this.EXPORTED_SYMBOLS = ["HostManifestManager", "NativeApp"];
> +/* globals NativeApp */

If this is for eslint, can you move it to toolkit/components/.eslintrc

::: toolkit/components/extensions/NativeMessaging.jsm:318
(Diff revision 1)
>    send(msg) {
>      if (this._isDisconnected) {
>        throw new this.context.cloneScope.Error("Attempt to postMessage on disconnected port");
>      }
> -
> -    let json;
> +    if (Cu.getClassName(msg, true) != "ArrayBuffer") {
> +      throw new this.context.cloneScope.Error("The message is not an ArrayBuffer");

If this ever happens, it is due to a programming error (in this code, not in an extension).  If it does ever happen and this message leaks back to the caller I think it will be very confusing.  Which is to say, this should throw, but either change the message or don't throw an error from cloneScope in which case it will turn into the general "internal error" message.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:272
(Diff revision 1)
>        resolve();
>      };
>      app.on("message", listener);
>    });
>  
> -  app.send(MSG);
> +  // This is equivalent to NativeApp.encodeMessage

so why not just call NativeApp.encodeMessage()?
Comment on attachment 8794519 [details]
Bug 1299411 - Deduplicate context getter logic in ParentAPIManager

https://reviewboard.mozilla.org/r/80946/#review80036
Attachment #8794519 - Flags: review?(aswan) → review+
Comment on attachment 8794520 [details]
Bug 1299411 - Remove extension param from NativeApp

https://reviewboard.mozilla.org/r/80948/#review80038
Attachment #8794520 - Flags: review?(aswan) → review+
Comment on attachment 8794521 [details]
Bug 1299411 - s/on/once/ in NativeApp's sendMessage

https://reviewboard.mozilla.org/r/80950/#review80040
Attachment #8794521 - Flags: review?(aswan) → review+
Comment on attachment 8794518 [details]
Bug 1299411 - separate serialization from sending

https://reviewboard.mozilla.org/r/80944/#review80030

> If this is for eslint, can you move it to toolkit/components/.eslintrc

No, because then it would apply to all files, whereas this `NativeApp` variable should only be declared global within this file.
Comment on attachment 8791455 [details]
Bug 1299411 - Decouple Port implementation from API

https://reviewboard.mozilla.org/r/78866/#review80024

`unregisterMessageFuncs` is analogous to `disconnectListeners`, and introduced so that listeners added via `registerOnMessage` are automatically removed when the context unloads/port disconnects, just like `registerOnDisconnect`. The purpose of the rewrite is not obvious from this commit, but if you consider the commit where the native messaging host switches to this Port implementation, then it'll probably make more sense :
See `onConnectNative` in `NativeMessaging.jsm` in https://reviewboard.mozilla.org/r/80952/diff/1#index_header
(=without `unregisterMessageFuncs`, the `onConnectNative` method has to save the returned unregister method and call it on disconnect).
Comment on attachment 8791455 [details]
Bug 1299411 - Decouple Port implementation from API

https://reviewboard.mozilla.org/r/78866/#review80024

Ah, so the previous code just kept the listener around but it returned immediately if the port was disconnected.  This seems like a reasonable improvement but it helps to either separate such changes or mention them in the comment for the patch.
Comment on attachment 8791455 [details]
Bug 1299411 - Decouple Port implementation from API

https://reviewboard.mozilla.org/r/78866/#review80184
Attachment #8791455 - Flags: review?(aswan) → review+
Comment on attachment 8794518 [details]
Bug 1299411 - separate serialization from sending

https://reviewboard.mozilla.org/r/80944/#review80188
Attachment #8794518 - Flags: review?(aswan) → review+
Comment on attachment 8794522 [details]
Bug 1299411 - Move native messaging to child process

https://reviewboard.mozilla.org/r/80952/#review80190

I'm going to deflect this one to Kris, I would be inclined to try to use different message names rather than using the toNativeApp property in the receiver.  But Kris has more background in that area.
Comment on attachment 8794523 [details]
Bug 1299411 - Error messages for native messaging

https://reviewboard.mozilla.org/r/80954/#review80192

::: toolkit/components/extensions/NativeMessaging.jsm:186
(Diff revision 2)
>      this.startupPromise = HostManifestManager.lookupApplication(application, context)
>        .then(hostInfo => {
> -        if (!hostInfo) {
> -          throw new Error(`No such native application ${application}`);
> -        }
> -
> +        // Put the two errors together to not leak information about whether a native
> +        // application is installed to addons that do not have the right permission.
> +        if (!hostInfo || !hostInfo.manifest.allowed_extensions.includes(context.extension.id)) {
> +          throw new Error(`This extension does not have permission to use native application ${application} (or the application was not installed)`);

nit: was->is
Attachment #8794523 - Flags: review?(aswan) → review+
Attachment #8794522 - Flags: review?(kmaglione+bmo)
Comment on attachment 8794514 [details]
Bug 1299411 - Pass port parameter to port.onMessage

https://reviewboard.mozilla.org/r/80936/#review80822
Attachment #8794514 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8794515 [details]
Bug 1299411 - Unify fire and fireWithoutClone.

https://reviewboard.mozilla.org/r/80938/#review80824

::: toolkit/components/extensions/ExtensionUtils.jsm:957
(Diff revision 1)
>    hasListener(callback) {
>      return this.callbacks.has(callback);
>    },
>  
>    fire(...args) {
> +    this._fireCommon(true, ...args);

No need to use spread expressions for the args. Just pass the array.

::: toolkit/components/extensions/ExtensionUtils.jsm:973
(Diff revision 1)
> +            runSafeSync(this.context, callback, ...args);
> +          } else {
> +            runSafeSyncWithoutClone(callback, ...args);

Please use `context.runSafe` and `context.runSafeWithoutClone`. And you may as well just pass the method name rather than using the `if`.

::: toolkit/components/extensions/ExtensionUtils.jsm
(Diff revision 1)
> -    for (let callback of this.callbacks) {
> -      this.context.runSafeWithoutClone(callback, ...args);
>      }

As I recall, there's code that depends on this happening synchronously. Have you done a full try run?
Attachment #8794515 - Flags: review?(kmaglione+bmo)
Comment on attachment 8794516 [details]
Bug 1299411 - Add test.assertLastError/assertNoLastError

https://reviewboard.mozilla.org/r/80940/#review80826

We shouldn't be using `lastError` enough for these to be especially useful, so I'd really rather not add them. Especially since it might encourage people to write tests that deal with `lastError` when they shouldn't.
Attachment #8794516 - Flags: review?(kmaglione+bmo)
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review80828

::: toolkit/components/extensions/ExtensionUtils.jsm:965
(Diff revision 1)
> +    // Read the internal value of lastError to avoid inadvertently marking the
> +    // error as checked if this event was fired as a part of some callback.
> +    let lastError = this.context._lastError;
>      for (let callback of this.callbacks) {
>        Promise.resolve(callback).then(callback => {
>          if (this.context.unloaded) {
>            dump(`${this.name} event fired after context unloaded.\n`);
>          } else if (!this.context.active) {
>            dump(`${this.name} event fired while context is inactive.\n`);
>          } else if (this.callbacks.has(callback)) {
> +          if (lastError) {
> +            this.context.lastError = lastError;
> +          }
> +
>            if (shouldClone) {
>              runSafeSync(this.context, callback, ...args);
>            } else {
>              runSafeSyncWithoutClone(callback, ...args);
>            }
> +
> +          if (lastError) {
> +            // Note: The error does not need to be checked, because Chrome does
> +            // not require that either and requiring it would cause log spam.
> +            this.context.lastError = null;
> +          }

Please use `context.withLastError` instead. If log spam actually does become a problem, we can reconsider, but I think it's more likely to be useful than it is to be an annoyance.

::: toolkit/components/extensions/ExtensionUtils.jsm:1237
(Diff revision 1)
> +          if (errorMessage) {
> +            this.context.lastError = this.context.normalizeError({message: errorMessage});
> +          }
> +          fire.withoutClone(portObj);
> +          if (errorMessage) {
> +            this.context.lastError = null;
> +          }
> +        });

Again, `context.withLastError()`.

But I'm surprised this works either way, given that you changed `fireWithoutClone` to call its handler asynchronously.

::: toolkit/components/extensions/ExtensionUtils.jsm:1341
(Diff revision 1)
> +   * Disconnect the port from the other end (which may not even exist).
> +   *
> +   * @param {string} [error] The reason for disconnecting, if it is an abnormal
> +   *      disconnect.
> +   */
> +  disconnectByOtherEnd(error) {

`(error = null)` since it's optional.

::: toolkit/components/extensions/ExtensionUtils.jsm:1370
(Diff revision 1)
>      this.handleDisconnection();
> -    this._sendMessage("Extension:Port:Disconnect", null);
> +    this._sendMessage("Extension:Port:Disconnect", error);
>    },
>  
>    close() {
> -    this.disconnect();
> +    this.disconnect("The port's context unloaded without calling disconnect().");

This doesn't seem like an error.

::: toolkit/components/extensions/ExtensionUtils.jsm:1474
(Diff revision 1)
> +        if (e) {
> +          e = String(e.message || e);
> +        }

This means we'll be exposing internal error messages to extension code, which we try to avoid.
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review80830
Attachment #8794517 - Flags: review?(kmaglione+bmo)
Comment on attachment 8794515 [details]
Bug 1299411 - Unify fire and fireWithoutClone.

https://reviewboard.mozilla.org/r/80938/#review80824

> As I recall, there's code that depends on this happening synchronously. Have you done a full try run?

`fireWithoutClone` is not directly used anywhere. It is exported as `fire.withoutClone`, which in its turn is only used by the implementation of port.onDisconnect: http://searchfox.org/mozilla-central/search?q=fire.%3FWithoutClone&case=false&regexp=true&path=. I'll reword the commit message to make this more clear.

Are you maybe confused with `runSafeWithoutClone` (which I did not change here)? (side note: having two functions with the same name but behave differently sucks - `ExtensionUtils.runSafeWithoutClone` runs the callback asynchronously whereas `context.runSafeWithoutClone` is synchronous).
Comment on attachment 8794516 [details]
Bug 1299411 - Add test.assertLastError/assertNoLastError

https://reviewboard.mozilla.org/r/80940/#review80826

`lastError` is the only way to signal errors in events. When the error is unexpectedly set (or not set), I want to have a useful error message (along the lines of "expected error Foo, go bar instead" or "expected no error, got foo"). Otherwise debugging becomes unnecessarily difficult. Since there are several tests with onDisconnect & errors (added in this commit and in the "Error messages for native messaging" commit), I want to maintain the new `assertNoLastError` / `assertLastError` methods.

For callbacks I agree that promises should be used instead of `lastError`. With code reviews you can enforce that these `assert*LastError` methods are not used unnecessarily. It is not that much different from using `browser.runtime.lastError`. The first time I used it you suggested to use promises and I've been doing that ever since - something like that can still continue to happen even when these new asserts are added.

If you're not convinced of that I can also add a comment to these methods (in test.json and in the implementation file) to emphasize that promises should be preferred over lastError checks.
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review80828

> Again, `context.withLastError()`.
> 
> But I'm surprised this works either way, given that you changed `fireWithoutClone` to call its handler asynchronously.

This works because `_cloneCommon` saves `lastError` before firing the callbacks asynchronously.
Attachment #8794516 - Attachment is obsolete: true
Attachment #8794516 - Flags: review?(kmaglione+bmo)
Comment on attachment 8794515 [details]
Bug 1299411 - Unify fire and fireWithoutClone.

https://reviewboard.mozilla.org/r/80938/#review88202
Attachment #8794515 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review88204

r=me if you move to storing the error as `Port.error` rather than passing it to the disconnect handler.

::: toolkit/components/extensions/ExtensionUtils.jsm:1266
(Diff revision 4)
>        postMessage: json => {
>          this.postMessage(json);
>        },
>        onDisconnect: new EventManager(this.context, "Port.onDisconnect", fire => {
> -        return this.registerOnDisconnect(() => fire.withoutClone(portObj));
> +        return this.registerOnDisconnect(error => {
> +          error = error ? this.context.normalizeError(error) : null;

`error && this.context.normalizeError(error)`

::: toolkit/components/extensions/ExtensionUtils.jsm:1267
(Diff revision 4)
>          this.postMessage(json);
>        },
>        onDisconnect: new EventManager(this.context, "Port.onDisconnect", fire => {
> -        return this.registerOnDisconnect(() => fire.withoutClone(portObj));
> +        return this.registerOnDisconnect(error => {
> +          error = error ? this.context.normalizeError(error) : null;
> +          fire.withoutClone(portObj, error);

I think I'd rather we store the error on the Port object itself. That would be consistent with things like DownloadItem.

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_connect.html:34
(Diff revision 4)
>          expected = "disconnect";
>          browser.test.notifyPass("runtime.connect");
>        }
>      });
> -    port.onDisconnect.addListener(() => {
> +    port.onDisconnect.addListener((port, error) => {
> +      browser.test.assertEq(error, null, "No error because port is closed by disconnect() at other end");

`assertEq(null, error, ...)`

::: toolkit/components/extensions/test/mochitest/test_ext_runtime_disconnect.html:20
(Diff revision 4)
>  
>  function background() {
>    browser.runtime.onConnect.addListener(port => {
>      browser.test.assertEq(port.name, "ernie", "port name correct");
> -    port.onDisconnect.addListener(() => {
> +    port.onDisconnect.addListener((portObj, error) => {
> +      browser.test.assertEq(error, null, "The port is implicitly closed without errors when the other context unloads");

`assertEq(null, error, ...)`

::: toolkit/components/extensions/test/mochitest/test_ext_unload_frame.html:49
(Diff revision 4)
>      let hasMessage = false;
> -    port.onDisconnect.addListener(() => {
> +    port.onDisconnect.addListener((port, error) => {
>        browser.test.assertFalse(disconnected, "onDisconnect should fire once");
>        disconnected = true;
>        browser.test.assertTrue(hasMessage, "Expected onMessage before onDisconnect");
> +      browser.test.assertEq(error, null, "The port is implicitly closed without errors when the other context unloads");

`assertEq(null, error)`
Attachment #8794517 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8794522 [details]
Bug 1299411 - Move native messaging to child process

https://reviewboard.mozilla.org/r/80952/#review88208

::: toolkit/components/extensions/Extension.jsm:196
(Diff revision 5)
> +        NativeApp.onConnectNative(context, target.messageManager, data.portId, sender, toNativeApp);
> +        return true;

How about `return NativeApp.onConnectNative(...)` ?

::: toolkit/components/extensions/NativeMessaging.jsm:231
(Diff revision 5)
> +   * @param {object} sender The object describing the creator of the connection
> +   *     request.
> +   * @param {string} application The name of the native messaging host.
> +   */
> +  static onConnectNative(context, messageManager, portId, sender, application) {
> +    messageManager.QueryInterface(Ci.nsIMessageSender);

If this is necessary (which it probably shouldn't be), it should be done by the caller.

::: toolkit/components/extensions/NativeMessaging.jsm:234
(Diff revision 5)
> +    app.once("disconnect", (what, err) => port.disconnect(err && err.message));
> +    /* eslint-disable mozilla/balanced-listeners */
> +    app.on("message", (what, msg) => port.postMessage(msg));
> +    /* eslint-enable mozilla/balanced-listeners */

Please add blank lines before and after this block.
Attachment #8794522 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1309926
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review88204

> `error && this.context.normalizeError(error)`

I use the current construct in case `error` is a non-null falsey value (e.g. undefined).
Yes, there is a "= null" default value, but that does not help when an explicit undefined value is passed (this happens e.g. when `_cleanup()` is called without arguments in NativeMessaging.jsm.
Comment on attachment 8794522 [details]
Bug 1299411 - Move native messaging to child process

https://reviewboard.mozilla.org/r/80952/#review88208

> How about `return NativeApp.onConnectNative(...)` ?

I put `return true` here because it is part of the contract of receiveMessage - any non-void value means that the message has been processed.
onConnectNative does not and should not know about the return type of the caller.

> If this is necessary (which it probably shouldn't be), it should be done by the caller.

This QI call serves as an assertion. It is here for two other reasons: 1) avoiding debugging nightmares and 2) consistency with onConnect in ExtensionUtils.jsm.

Longer version:
Once I lost more than a day on debugging to discover that runtime.onConnect is sometimes not called, and that was ultimately caused by the fact that target.messageManager was NOT a nsIMessageSender (unless I stepped through with a debugger, or added instanceof/QI). This was a real pain to debug because MessageChannel.jsm swallowed all errors and the source of the bug was NOT obvious at all.
The fix is to explicitly call QI in getMessageManager [1], which in turn is called by the handler of onConnect [2]. This onConnectNative method is analogous to the onConnect handler in ExtensionUtils.jsm.

[1] http://searchfox.org/mozilla-central/diff/3df8d2e80e659dba98735e2ae983189914084da4/toolkit/components/extensions/ExtensionUtils.jsm#1294
[2] http://searchfox.org/mozilla-central/diff/3df8d2e80e659dba98735e2ae983189914084da4/toolkit/components/extensions/ExtensionUtils.jsm#1405
Comment on attachment 8794517 [details]
Bug 1299411 - Propagate errors to port.onDisconnect via port.error

https://reviewboard.mozilla.org/r/80942/#review88204

> I use the current construct in case `error` is a non-null falsey value (e.g. undefined).
> Yes, there is a "= null" default value, but that does not help when an explicit undefined value is passed (this happens e.g. when `_cleanup()` is called without arguments in NativeMessaging.jsm.

The default value is used when an explicit `undefined` is passed:

» ((x = 1) => x)(undefined)
← 1
Comment on attachment 8794522 [details]
Bug 1299411 - Move native messaging to child process

https://reviewboard.mozilla.org/r/80952/#review88208

> This QI call serves as an assertion. It is here for two other reasons: 1) avoiding debugging nightmares and 2) consistency with onConnect in ExtensionUtils.jsm.
> 
> Longer version:
> Once I lost more than a day on debugging to discover that runtime.onConnect is sometimes not called, and that was ultimately caused by the fact that target.messageManager was NOT a nsIMessageSender (unless I stepped through with a debugger, or added instanceof/QI). This was a real pain to debug because MessageChannel.jsm swallowed all errors and the source of the bug was NOT obvious at all.
> The fix is to explicitly call QI in getMessageManager [1], which in turn is called by the handler of onConnect [2]. This onConnectNative method is analogous to the onConnect handler in ExtensionUtils.jsm.
> 
> [1] http://searchfox.org/mozilla-central/diff/3df8d2e80e659dba98735e2ae983189914084da4/toolkit/components/extensions/ExtensionUtils.jsm#1294
> [2] http://searchfox.org/mozilla-central/diff/3df8d2e80e659dba98735e2ae983189914084da4/toolkit/components/extensions/ExtensionUtils.jsm#1405

In that case, I'd rather fix the errors being swallowed than add an extra QI here.
Comment on attachment 8794522 [details]
Bug 1299411 - Move native messaging to child process

https://reviewboard.mozilla.org/r/80952/#review88208

> In that case, I'd rather fix the errors being swallowed than add an extra QI here.

I have already done that in the past: http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/toolkit/components/extensions/MessageChannel.jsm#621-627

I've now removed the QI. Hopefully it doesn't cause bugs...
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/a41f871e2d1b
Decouple Port implementation from API r=aswan
https://hg.mozilla.org/integration/autoland/rev/502aaf0691cc
Pass port parameter to port.onMessage r=kmag
https://hg.mozilla.org/integration/autoland/rev/e08ee4c2b1e2
Unify fire and fireWithoutClone. r=kmag
https://hg.mozilla.org/integration/autoland/rev/0ce4a6653d19
Propagate errors to port.onDisconnect via port.error r=kmag
https://hg.mozilla.org/integration/autoland/rev/a9c19ee017a4
separate serialization from sending r=aswan
https://hg.mozilla.org/integration/autoland/rev/57f7a5c7044d
Deduplicate context getter logic in ParentAPIManager r=aswan
https://hg.mozilla.org/integration/autoland/rev/2829c46a636d
Remove extension param from NativeApp r=aswan
https://hg.mozilla.org/integration/autoland/rev/24d81c7b335e
s/on/once/ in NativeApp's sendMessage r=aswan
https://hg.mozilla.org/integration/autoland/rev/ed1afd2aad61
Move native messaging to child process r=kmag
https://hg.mozilla.org/integration/autoland/rev/c456c1797299
Error messages for native messaging r=aswan
Depends on: 1313980
I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/Port. Please let me know if this covers it.
Flags: needinfo?(rob)
port.error is not string, but an object with a "message" string property.
I suggest that you also add a compatibility note that port.error is not supported in Chrome/Opera/Edge (i.e. only supported in Firefox), and maybe even that in these Chromium-based browsers, the error is available in chrome.runtime.lastError.
Flags: needinfo?(rob)
Sorry, I've corrected that and updated the compat notes too. I'm closing this but let me know if you see anything else.
Keywords: dev-doc-needed → dev-doc-complete
Hi,

Is there any chances that the fix for this bug will be released in earlier versions before Firefox 52 release ?

we have a plugin which we are migrating it to the new Native messaging web extensions. we have a situation on which if Native app is not installed we need to prompt user to download and install the MSI file.
However as we are not able to find any alternative method to find whether native app is installed or not apart from PORT.Error but that fix will in Firefox 52.
And in Firefox 52 existing XUL plugin will be disabled , so in current scenario we will not be able to release the new plugin before Firefox 52 and there might be a downtime also.

Can you please suggest what could be possible solution for this scenario ?
(In reply to Rohan Kapoor from comment #101)
> we have a plugin which we are migrating it to the new Native messaging web
> extensions. we have a situation on which if Native app is not installed we
> need to prompt user to download and install the MSI file.
> However as we are not able to find any alternative method to find whether
> native app is installed or not apart from PORT.Error but that fix will in
> Firefox 52.

If the native application and the add-on adhere to the native messaging protocol, then the only reason for exiting early is that the native messaging app is not installed. You could implement the following:
- In the native application, return "OK" if the message is "testavailability" (the messages can be completely different if desired).
- From the add-on, use
    browser.runtime.sendNativeMessage("your.native.messaging.app.id", "testavailability", function(response) {
        if (response === "OK") {
            // Application installed
        } else {
            // Application not installed.
        }
    });

Instead of "OK", you can also use a version number. And then if your add-on ever needs a newer version, you can ask to install (if no version number is found) or upgrade (if an old version number is found) the native app.
I'm having a similar issue to this with 1Password. Both runtime.lastError and port.error are null using 55.0b1 (64-bit). I've modified the native-messaging WebExtensions example only to add a disconnect handler:

function portDisconnectListener(port) {
	console.warn(port)
	console.warn(chrome.runtime.lastError);
	console.warn(browser.runtime.lastError);
	console.warn(port.error);
}
port.onDisconnect.addListener(portDisconnectListener);

Steps to reproduce:

1. Modify the native-messaging WebExtensions example to add the above code to background.js
2. Run the add-on and click the toolbar button
3. Kill the Python process powering the native messaging host script

Actual results:

Port object is logged, but the next three lines log null

Expected results:

Some error message indicates that the native process exited.

Other notes:

Chrome documents the expected error messages here: https://developer.chrome.com/extensions/nativeMessaging#native-messaging-debugging Specifically, 1Password is looking for the "native host has exited" error message so that it can delay attempting to re-establish the connection. Otherwise, when users update 1Password, aggressively reconnecting from the extension causes the update process to be disrupted. We instead need to delay reconnecting when we know specifically that the native host has exited.
Sorry… just realized this is closed. I will open a new bug
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: