This bug covers implementing this feature of the native messaging protocol: > On Windows, the native messaging host gets passed a command line argument with a handle to the calling chrome native window: --parent-window=<decimal handle value>. This lets the native messaging host create native UI windows that are correctly focused.

Over at https://wiki.mozilla.org/Talk:WebExtensions/Native_Messaging#Host_Manifests_-_location_manifest, Kattamine raises a question about searching the 32-bit and 64-bit registries. I'm not familiar enough with Windows to have an informed opinion...

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

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

Try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6db9ddc9c9c&selectedJob=22339070 trychooser continues to baffle me though, I appear to have gotten chrome mochitests without e10s but not with (though since the native messaging mochitest doesn't do anything with content, I wouldn't expect a difference here)

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module https://reviewboard.mozilla.org/r/59084/#review56104 ::: testing/modules/MockRegistry.jsm:13 (Diff revision 1) > +Cu.import("resource://testing-common/MockRegistrar.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +let MockRegistry = {}; > + > +if ("nsIWindowsRegKey" in Ci) { This guard shouldn't be necessary. ::: testing/modules/MockRegistry.jsm:26 (Diff revision 1) > + case Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE: > + return MockRegistry.LOCAL_MACHINE; > + case Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER: > + return MockRegistry.CURRENT_USER; > + case Ci.nsIWindowsRegKey.ROOT_KEY_CLASSES_ROOT: > + return MockRegistry.CLASSES_ROOT; s/MockRegistry/this/ ::: testing/modules/MockRegistry.jsm:28 (Diff revision 1) > + case Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER: > + return MockRegistry.CURRENT_USER; > + case Ci.nsIWindowsRegKey.ROOT_KEY_CLASSES_ROOT: > + return MockRegistry.CLASSES_ROOT; > + default: > + do_throw("Unknown root " + aRoot); `do_throw` doesn't exist here. Just throw normally. ::: testing/modules/MockRegistry.jsm:34 (Diff revision 1) > + return null; > + } > + }, > + > + setValue: function(aRoot, aPath, aName, aValue) { > + let rootKey = MockRegistry.getRoot(aRoot); s/MockRegistry/this/ ::: testing/modules/MockRegistry.jsm:37 (Diff revision 1) > + > + setValue: function(aRoot, aPath, aName, aValue) { > + let rootKey = MockRegistry.getRoot(aRoot); > + > + if (!(aPath in rootKey)) { > + rootKey[aPath] = []; This should be a Map rather than an array. ::: testing/modules/MockRegistry.jsm:79 (Diff revision 1) > + // --- Overridden nsIWindowsRegKey interface functions --- > + open: function(aRootKey, aRelPath, aMode) { > + let rootKey = MockRegistry.getRoot(aRootKey); > + > + if (!(aRelPath in rootKey)) > + rootKey[aRelPath] = []; Map rather than array. ::: testing/modules/MockRegistry.jsm:108 (Diff revision 1) > + } > + return null; > + } > + }; > + > + MockRegistrar.register("@mozilla.org/windows-registry-key;1", MockWindowsRegKey); This shouldn't happen automatically when the module is loaded. Tests that need this should call a method to register it, and will also need to unregister it when they're finished.

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows https://reviewboard.mozilla.org/r/59086/#review56102 ::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:181 (Diff revision 1) > + allowed_extensions: [ID], > + }; > + let manifestPath = getPath(`${script.name}.json`); > + yield OS.File.writeAtomic(manifestPath, JSON.stringify(manifest)); > + > + MockRegistry.setValue(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, These values need to be cleared, and the mock registry unregistered, at the end of the test. ::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:13 (Diff revision 1) > Cu.import("resource://gre/modules/Services.jsm"); > const {Subprocess, SubprocessImpl} = Cu.import("resource://gre/modules/Subprocess.jsm"); > Cu.import("resource://gre/modules/NativeMessaging.jsm"); > Cu.import("resource://gre/modules/osfile.jsm"); > > +Cu.import("resource://testing-common/MockRegistry.jsm"); This only needs to be imported on Windows. ::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:14 (Diff revision 1) > const {Subprocess, SubprocessImpl} = Cu.import("resource://gre/modules/Subprocess.jsm"); > Cu.import("resource://gre/modules/NativeMessaging.jsm"); > Cu.import("resource://gre/modules/osfile.jsm"); > > +Cu.import("resource://testing-common/MockRegistry.jsm"); > +const isWin = ("nsIWindowsRegKey" in Ci); Please use `AppConstants.platform` instead. ::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:190 (Diff revision 1) > + } > + // global test.json and LOCAL_MACHINE registry key on windows are > + // still present from the previous test > > let result = yield HostManifestManager.lookupApplication("test", context); > - notEqual(result, null, "lookupApplication finds a manifest when entries exist in both user-specific and system-wide directories"); > + notEqual(result, null, "lookupApplication finds a manifest when entries exist in both user-specific and system-wide llocations"); s/llocations/locations/

https://reviewboard.mozilla.org/r/59084/#review56106 Most of the comments are about stuff that I inherited and just moved around, but they're reasonable suggestions, I'll implement them and push a new version shortly. ::: testing/modules/MockRegistry.jsm:13 (Diff revision 1) > +Cu.import("resource://testing-common/MockRegistrar.jsm"); > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > + > +let MockRegistry = {}; > + > +if ("nsIWindowsRegKey" in Ci) { Why do you say that? We reference constants from it just a few lines below, that would fail on non-Windows platforms...

https://reviewboard.mozilla.org/r/59084/#review56106 > Why do you say that? > We reference constants from it just a few lines below, that would fail on non-Windows platforms... It would only fail if the functions were actually called. But this module shouldn't ever be loaded on non-Windows platforms. In fact, it really shouldn't even be packaged.

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59084/diff/1-2/

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59086/diff/1-2/

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59084/diff/2-3/

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59086/diff/2-3/

https://reviewboard.mozilla.org/r/59084/#review56212 ::: testing/modules/MockRegistry.jsm:46 (Diff revision 3) > + open: function(root, path, mode) { > + let rootKey = registry.getRoot(root); > + if (!rootKey) { > + throw new Error(`no such root ${root}`); > + } > + this.values = rootKey.get(path); Can we either throw here if it doesn't exist, or create it? Or does existing code depend on it throwing only when we try to touch a value? The IDL suggests that this should throw if the key doesn't exist, so it doesn't seem likely. ::: testing/modules/MockRegistry.jsm:100 (Diff revision 3) > + } > + > + setValue(root, path, name, value) { > + let rootKey = this.getRoot(root); > + if (rootKey == null) { > + throw new Error(`no such root ${root}`); Can we move this check to `getRoot`, or are there any callers that depend on it returning undefined for invalid values? ::: testing/modules/moz.build:16 (Diff revision 3) > 'AppData.jsm', > 'AppInfo.jsm', > 'Assert.jsm', > 'CoverageUtils.jsm', > 'MockRegistrar.jsm', > + 'MockRegistry.jsm', We should only add this when we're building for Windows.

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows https://reviewboard.mozilla.org/r/59086/#review56214

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59084/diff/3-4/

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59086/diff/3-4/

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module https://reviewboard.mozilla.org/r/59084/#review56236 ::: testing/modules/moz.build:23 (Diff revisions 3 - 4) > 'TestUtils.jsm', > ] > > +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'windows': > + TESTING_JS_MODULES += [ > + 'MockRegistry.jsm' Please add trailing comma.

Comment on attachment 8762290 [details] Bug 1270359 Make MockRegistry a common test-only module Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59084/diff/4-5/

Comment on attachment 8762291 [details] Bug 1270359 Implement connectNative on windows Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59086/diff/4-5/

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1394ec844127 Make MockRegistry a common test-only module r=kmag https://hg.mozilla.org/integration/mozilla-inbound/rev/69f8d4458159 Implement connectNative on windows r=kmag

https://hg.mozilla.org/mozilla-central/rev/1394ec844127 https://hg.mozilla.org/mozilla-central/rev/69f8d4458159

https://reviewboard.mozilla.org/r/59086/#review56844 <p>&lt;p&gt;Tested with manifest containing relative path and failed to execute native app.&lt;/p&gt;<br /> &lt;p&gt;Referring to Chrome doc, path description&lt;br /&gt;<br /> On Windows it can be relative to the directory in which the manifest file is located. The host process is started with the current directory set to the directory that contains the host binary.&lt;/p&gt;</p> ::: toolkit/components/extensions/NativeMessaging.jsm:96 (Diff revision 5) > + return this._tryPath(path, application, context) > + .then(manifest => manifest ? {path, manifest} : null); Might need to do further manipulation for relative path here. ::: toolkit/components/extensions/NativeMessaging.jsm:193 (Diff revision 5) > let subprocessOpts = { > command: hostInfo.manifest.path, > arguments: [hostInfo.path], > workdir: OS.Path.dirname(hostInfo.manifest.path), > }; > return Subprocess.call(subprocessOpts); Getting failure here with relative path, seems unsupported? As a hack to validate I added the following and was able to use relative path. hostInfo.manifest.path = OS.Path.dirname(hostInfo.path) + "\\\\" + hostInfo.manifest.path

https://reviewboard.mozilla.org/r/59086/#review56844 > Getting failure here with relative path, seems unsupported? > > As a hack to validate I added the following and was able to use relative path. > hostInfo.manifest.path = OS.Path.dirname(hostInfo.path) + "\\\\" + hostInfo.manifest.path After this point I'm now getting issue - Failed to create pipe this.NativeApp</this.startupPromise<() Debugging subprocess_worker_win.js see that line 371 their_pipes[2] = ok && win32.Handle(handle); is resulting in false.

Since this bug is marked as fixed, could you file a new bug on your problems and block it against bug 1190682 please?

Raised Bug 1281995 - Support runtime.connectNative manifest containing relative path

As suggested by :aswan though it will be addressed for connectNative in bug 1272522 also raised subprocess issue Bug 1282001 - subprocess_worker_win fails initPipes when stderr:"ignore"

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