1270359 - Implement runtime.connectNative for Windows

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

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.

Comment 1

[Andrew Swan [:aswan]]

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...

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1270359 Make MockRegistry a common test-only module

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

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1270359 Implement connectNative on windows

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

Comment 4

[Andrew Swan [:aswan]]

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 5

[Kris Maglione [:kmag]]

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 6

[Kris Maglione [:kmag]]

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/

Comment 7

[Andrew Swan [:aswan]]

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...

Comment 8

[Kris Maglione [:kmag]]

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 9

[Andrew Swan [:aswan]]

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 10

[Andrew Swan [:aswan]]

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 11

[Andrew Swan [:aswan]]

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 12

[Andrew Swan [:aswan]]

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/

Comment 13

[Kris Maglione [:kmag]]

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 14

[Kris Maglione [:kmag]]

Comment on attachment 8762291 [details]
Bug 1270359 Implement connectNative on windows

https://reviewboard.mozilla.org/r/59086/#review56214

Comment 15

[Andrew Swan [:aswan]]

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 16

[Andrew Swan [:aswan]]

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 17

[Kris Maglione [:kmag]]

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 18

[Andrew Swan [:aswan]]

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 19

[Andrew Swan [:aswan]]

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/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[gorf4673]

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

Comment 23

[gorf4673]

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.

Comment 24

[Andy McKay]

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

Comment 25

[gorf4673]

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

Comment 26

[gorf4673]

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"

Comment 27

[Will Bamberg [:wbamberg]]

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