Implement runtime.connectNative for Windows

RESOLVED FIXED in Firefox 50

Status

RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

({dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [native messaging] triaged)

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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...
(Assignee)

Comment 2

2 years ago
Created attachment 8762290 [details]
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/
Attachment #8762290 - Flags: review?(kmaglione+bmo)
Attachment #8762291 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
Created attachment 8762291 [details]
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/
(Assignee)

Updated

2 years ago
Assignee: nobody → aswan
Status: NEW → ASSIGNED
Iteration: --- → 50.1
(Assignee)

Comment 4

2 years ago
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.
Attachment #8762290 - Flags: review?(kmaglione+bmo)
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/
Attachment #8762291 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 9

2 years ago
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/
Attachment #8762290 - Flags: review?(kmaglione+bmo)
Attachment #8762291 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

2 years ago
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/
(Assignee)

Comment 11

2 years ago
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/
(Assignee)

Comment 12

2 years ago
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.

Updated

2 years ago
Attachment #8762291 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 15

2 years ago
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/
(Assignee)

Comment 16

2 years ago
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.
Attachment #8762290 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Comment 19

2 years ago
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

2 years ago
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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1394ec844127
https://hg.mozilla.org/mozilla-central/rev/69f8d4458159
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Keywords: dev-doc-needed

Comment 22

2 years ago
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

2 years ago
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

2 years ago
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

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

Comment 26

2 years ago
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"
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 1332408

Updated

4 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.