Closed
Bug 1299411
Opened 8 years ago
Closed 8 years ago
Port.onDisconnect does not indicate an error
Categories
(WebExtensions :: Untriaged, defect, P2)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: zhongyzh, Assigned: robwu)
References
Details
(Keywords: dev-doc-complete, Whiteboard: triaged)
Attachments
(10 files, 1 obsolete file)
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kmag
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kmag
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kmag
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
aswan
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
kmag
:
review+
|
Details |
(deleted),
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
Updated•8 years ago
|
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit
Comment 1•8 years ago
|
||
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]
Comment 2•8 years ago
|
||
(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)
Updated•8 years ago
|
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: [native messaging] → [native messaging]triaged
Comment 3•8 years ago
|
||
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
Comment 4•8 years ago
|
||
This is happening as a side effect of Rob's work on bug 1287007.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 5•8 years ago
|
||
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 hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
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...
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
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 20•8 years ago
|
||
mozreview-review |
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 21•8 years ago
|
||
mozreview-review |
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 22•8 years ago
|
||
mozreview-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 23•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•8 years ago
|
||
mozreview-review-reply |
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 33•8 years ago
|
||
mozreview-review |
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 34•8 years ago
|
||
mozreview-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 35•8 years ago
|
||
mozreview-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 36•8 years ago
|
||
mozreview-review |
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+
Updated•8 years ago
|
Attachment #8794522 -
Flags: review?(kmaglione+bmo)
Comment 37•8 years ago
|
||
mozreview-review |
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 38•8 years ago
|
||
mozreview-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 39•8 years ago
|
||
mozreview-review |
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 40•8 years ago
|
||
mozreview-review |
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 41•8 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 42•8 years ago
|
||
mozreview-review-reply |
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®exp=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).
Assignee | ||
Comment 43•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 44•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8794516 -
Attachment is obsolete: true
Attachment #8794516 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 73•8 years ago
|
||
mozreview-review |
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 74•8 years ago
|
||
mozreview-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 75•8 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 83•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 84•8 years ago
|
||
mozreview-review-reply |
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 85•8 years ago
|
||
mozreview-review-reply |
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 86•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 94•8 years ago
|
||
mozreview-review-reply |
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...
Comment 95•8 years ago
|
||
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
Comment 96•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a41f871e2d1b
https://hg.mozilla.org/mozilla-central/rev/502aaf0691cc
https://hg.mozilla.org/mozilla-central/rev/e08ee4c2b1e2
https://hg.mozilla.org/mozilla-central/rev/0ce4a6653d19
https://hg.mozilla.org/mozilla-central/rev/a9c19ee017a4
https://hg.mozilla.org/mozilla-central/rev/57f7a5c7044d
https://hg.mozilla.org/mozilla-central/rev/2829c46a636d
https://hg.mozilla.org/mozilla-central/rev/24d81c7b335e
https://hg.mozilla.org/mozilla-central/rev/ed1afd2aad61
https://hg.mozilla.org/mozilla-central/rev/c456c1797299
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox52:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 97•8 years ago
|
||
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)
Assignee | ||
Comment 98•8 years ago
|
||
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)
Comment 99•8 years ago
|
||
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
Comment 101•8 years ago
|
||
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 ?
Assignee | ||
Comment 102•8 years ago
|
||
(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.
Comment 103•7 years ago
|
||
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.
Comment 104•7 years ago
|
||
Sorry… just realized this is closed. I will open a new bug
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•