Closed Bug 1270359 Opened 8 years ago Closed 8 years ago

Implement runtime.connectNative for Windows

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Iteration:
50.1 - Jun 20
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [native messaging] triaged)

Attachments

(2 files)

Bug 1270359 Make MockRegistry a common test-only module
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1270359 Implement connectNative on windows
58 bytes, text/x-review-board-request
kmag
: review+
Details 
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...
Assignee: nobody → aswan
Status: NEW → ASSIGNED
Iteration: --- → 50.1
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)
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/
Attachment #8762290 - Flags: review?(kmaglione+bmo)
Attachment #8762291 - Flags: review?(kmaglione+bmo)
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.
Attachment #8762291 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8762290 - Flags: review?(kmaglione+bmo) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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"
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: