Closed Bug 1270356 Opened 8 years ago Closed 8 years ago

Implement native host manifest searching / parsing

Categories

(WebExtensions :: Untriaged, defect)

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

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: [native messaging] triaged)

Attachments

(2 files)

MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag
58 bytes, text/x-review-board-request
kmag
: review+
Details
MozReview Request: Bug 1270356 Part 1: Add native messaging paths to XRE directory provider r?kmag
58 bytes, text/x-review-board-request
kmag
: review+
Details
Blocks: 1270357
Assignee: nobody → aswan
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/1-2/
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

This isn't ready for a real review yet, the thing I would really like feedback on is the directory handling.  If you have feedback on the other code here that's helpful too but the handling of directories has some bigger open questions.
For reference, the Chrome documentation and our proposal so far are at:
https://developer.chrome.com/extensions/nativeMessaging#native-messaging-host-location
https://wiki.mozilla.org/WebExtensions/Native_Messaging#Host_Manifests
Getting the Mac system-wide directory via a directory provider seems reasonable since the Apple-blessed way to get that path is to use FSFindFolder() instead of hard-coding the path.
Linux gets gnarlier since we have lib, lib64, and share to consider.  I can't help but notice that these paths are pretty similar to system extension directories, the existing code that handles those is here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1416-1463
Perhaps that should be refactored slightly to have a single function (littered with ifdefs) to get the "system directory" and then let extensions and NativeMessagingHosts be subdirectories of that system directory?  That would make us consistent with the existing stuff for system extensions (eg /usr/local instead of /usr on some BSDs for what thats worth).  It doesn't really account for share/ but these days, a share/ directory seems like a real anachronism, I think we could just simplify by changing the specification for Linux to not include share/
Of course, if the w3c browserext group agrees on some set of browser-independent paths then a bunch of this becomes irrelevant.
Ugh.  Thoughts?
Attachment #8749935 - Flags: feedback?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/51251/#review48203

This looks pretty good so far.

::: toolkit/components/extensions/schemas/native_host_manifest.json:12
(Diff revision 2)
> +        "type": "object",
> +        "description": "Represents a native host manifest file",
> +        "properties": {
> +          "name": {
> +            "type": "string",
> +            "pattern": "\\w+(\\.\\w+)*",

This needs to be anchored at both ends.

::: toolkit/components/extensions/schemas/native_host_manifest.json:13
(Diff revision 2)
> +        "description": "Represents a native host manifest file",
> +        "properties": {
> +          "name": {
> +            "type": "string",
> +            "pattern": "\\w+(\\.\\w+)*",
> +            "optional": false

I'd leave out the `"optional": false`. It defaults to false, and we're going to have to replace all of the "optional" properties with "required" arrays in the future anyway.

::: toolkit/components/extensions/schemas/native_host_manifest.json:21
(Diff revision 2)
> +            "type": "string",
> +            "optional": false
> +          },
> +          "path": {
> +            "type": "string",
> +            "optional": false

Might be good to add an "absolutePath" string format to validate this.

::: toolkit/components/extensions/schemas/native_host_manifest.json:33
(Diff revision 2)
> +          },
> +          "allowed_extensions": {
> +            "type": "array",
> +            "items": {
> +              "type": "string"
> +            },

Maybe add `"minItems": 1`, and use `"$ref": "ExtensionID"` for the type?

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:54
(Diff revision 2)
> +
> +  let outstream = Cc["@mozilla.org/network/file-output-stream;1"]
> +      .createInstance(Ci.nsIFileOutputStream);
> +  outstream.init(file, FileUtils.MODE_WRONLY | FileUtils.MODE_CREATE | FileUtils.MODE_TRUNCATE, FileUtils.PERMS_FILE, 0);
> +  outstream.write(manifest, manifest.length);
> +  outstream.close();

It would be better to use `OS.File.writeAtomic` for this.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:69
(Diff revision 2)
> +};
> +
> +let templateManifest = {
> +  name: "test",
> +  description: "this is only a test",
> +  path: "/bin/cat",

We should use `Subprocess.pathSearch` for this. I don't think this will work on Windows, but hopefully `cat` should be in the $PATH in our test environment there.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:76
(Diff revision 2)
> +  allowed_extensions: [],
> +};
> +
> +add_task(function* test_nonexistent_manifest() {
> +  let result = yield HostManifestManager.lookupApplication("test", context);
> +  Assert.equal(result, null, "lookupApplication returns null for non-existent application");

You can just use `equal`. The basic test functions are copied to the global scope for xpcshell tests.

::: toolkit/xre/nsXREDirProvider.cpp:321
(Diff revision 2)
>    }
> +#if defined(XP_MACOSX)
> +  else if (!strcmp(aProperty, XRE_LOCAL_APP_DATA_DIR)) {
> +    FSRef fsRef;
> +    OSErr err = ::FSFindFolder(kLocalDomain, kApplicationSupportFolderType, kCreateFolder, &fsRef);
> +    NS_ENSURE_FALSE(err, NS_ERROR_FAILURE);

In general, if you're in a section of code where `NS_ENSURE_*` macros aren't used, you shouldn't use them. A lot of module owners still prefer them, but in general they're officially frowned-upon because they hide returns.

::: toolkit/xre/nsXREDirProvider.cpp:327
(Diff revision 2)
> +
> +    rv = NS_NewNativeLocalFile(EmptyCString(), true, getter_AddRefs(file));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsILocalFileMac> dirFileMac = do_QueryInterface(file);
> +    NS_ENSURE_TRUE(dirFileMac, NS_ERROR_UNEXPECTED);

Add an `&rv` param to `do_QueryInterface` and check `NS_SUCCEEDED(rv)` instead, as you do below.

::: toolkit/xre/nsXREDirProvider.cpp:333
(Diff revision 2)
> +
> +    rv = dirFileMac->InitWithFSRef(&fsRef);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    file = do_QueryInterface(dirFileMac, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

You can include "SpecialSystemDirectory.h" and use `GetOSXFolderType` for most of the above.

::: toolkit/xre/nsXREDirProvider.cpp:335
(Diff revision 2)
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    file = do_QueryInterface(dirFileMac, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    rv = AppendProfilePath(file, nullptr, nullptr, nullptr, true);

I'm not sure `AppendProfilePath` is what we want here, although in practice it does seem to do the right thing...

::: xpcom/build/nsXULAppAPI.h:57
(Diff revision 2)
>  #define XRE_USER_APP_DATA_DIR "UAppData"
>  
> +#if defined(XP_MACOSX)
> +/**
> + * The Mac system-wide application data directory:
> + * /Library/Application/Support/$name

"/Application Support"
(In reply to Andrew Swan [:aswan] from comment #3)
> Perhaps that should be refactored slightly to have a single function
> (littered with ifdefs) to get the "system directory" and then let extensions
> and NativeMessagingHosts be subdirectories of that system directory?

Yeah, that does sound like a good idea for NativeMessagingHosts. I'd rather
not touch global extension installs, at this point. It's a hornets' nest as it
is.

> Of course, if the w3c browserext group agrees on some set of
> browser-independent paths then a bunch of this becomes irrelevant.

We can cross that bridge when we come to it. We may wind up continuing to
support both.
https://reviewboard.mozilla.org/r/51251/#review48203

> Might be good to add an "absolutePath" string format to validate this.

Eh, except paths can be relative on Windows, see the description of path slightly below https://developer.chrome.com/extensions/nativeMessaging#native-messaging-host
https://reviewboard.mozilla.org/r/51251/#review48203

> Eh, except paths can be relative on Windows, see the description of path slightly below https://developer.chrome.com/extensions/nativeMessaging#native-messaging-host

OK. In that case we should probably add a "filePath" string format instead.
https://reviewboard.mozilla.org/r/51251/#review48203

> We should use `Subprocess.pathSearch` for this. I don't think this will work on Windows, but hopefully `cat` should be in the $PATH in our test environment there.

This is just testing manifest parsing and validation, we're not actually spawning any subprocesses yet, I expect a separate test file for that.  In particular, I think that will be a mochitest that tests the actual `connectNative()` / `sendNativeMessage()` extension APIs.

> I'm not sure `AppendProfilePath` is what we want here, although in practice it does seem to do the right thing...

I agree the name makes it sound totally inappropriate but passing it a bunch of null arguments seems to be the way to get what we want.
https://reviewboard.mozilla.org/r/51251/#review48203

> This is just testing manifest parsing and validation, we're not actually spawning any subprocesses yet, I expect a separate test file for that.  In particular, I think that will be a mochitest that tests the actual `connectNative()` / `sendNativeMessage()` extension APIs.

OK. But even so, this is not a legal path on Windows, so if we add path validation to the manifest schema, this will fail.

> I agree the name makes it sound totally inappropriate but passing it a bunch of null arguments seems to be the way to get what we want.

Yeah, I agree that it does the right thing, but it seems like it might be better to just directly append `gAppData->name` instead, since we're not actually trying to append a profile path here.
Attachment #8749935 - Flags: feedback?(kmaglione+bmo) → feedback+
https://reviewboard.mozilla.org/r/51251/#review48203

> OK. But even so, this is not a legal path on Windows, so if we add path validation to the manifest schema, this will fail.

Okay, is there anything existing in OS.Path that I can use here or am I writing the validator myself?  A quick glance didn't turn up anything...
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/2-3/
Attachment #8749935 - Flags: feedback+
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Addressed most of the comments from the first pass (still need to create a filePath string format checker).  Also implemented the directory search scheme we discussed on IRC.
This is also not yet tested at all on Linux but I could get one more round of feedback on the overall approach (and any thoughts you might have on the test case that is currently disabled and marked with an XXX comment), then I'll make sure this is good on Linux and split it up into smaller pieces that can be reviewed by the appropriate people.
Attachment #8749935 - Flags: feedback?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/51251/#review48535

::: toolkit/components/extensions/NativeMessaging.jsm:33
(Diff revision 3)
> +  _initialized: false,
> +  _lookup: null,
> +
> +  decoder: new TextDecoder(),
> +
> +  _search_paths() {

We should make this a lazy getter. Also, please use camel case rather than underscores. And I don't think the leading underscore is necessary here.

::: toolkit/components/extensions/NativeMessaging.jsm:54
(Diff revision 3)
> +    }
> +
> +    return Schemas.load(HOST_MANIFEST_SCHEMA);
> +  },
> +
> +  _try_path(dir, application, context) {

Please use camel case rather than underscores. Same for the other methods.

::: toolkit/components/extensions/NativeMessaging.jsm:75
(Diff revision 3)
> +      });
> +  },
> +
> +  _try_paths(application, dirs, context) {
> +    if (dirs.length == 0) {
> +      return null;

This should always return a promise.

::: toolkit/components/extensions/NativeMessaging.jsm:78
(Diff revision 3)
> +  _try_paths(application, dirs, context) {
> +    if (dirs.length == 0) {
> +      return null;
> +    }
> +
> +    return this._try_path(dirs[0], application, context)

This would be easier to follow if you made this a `Task.async` function.

::: toolkit/components/extensions/NativeMessaging.jsm:92
(Diff revision 3)
> +    if (!VALID_APPLICATION.test(application)) {
> +      throw new context.cloneScope.Error(`Invalid application "${application}"`);
> +    }
> +
> +    let promise = Promise.resolve();
> +    if (!this._initialized) {

We generally put the initialized check in the `init()` function.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:8
(Diff revision 3)
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +
> +const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm", {});
> +const {OS} = Cu.import("resource://gre/modules/osfile.jsm", {});
> +const {HostManifestManager} = Cu.import("resource://gre/modules/NativeMessaging.jsm", {});

Is there a reason you're assigning the return value of `Cu.import` for these, but not for the others?

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:28
(Diff revision 3)
> +
> +let globalDir = dir.clone();
> +globalDir.append("global");
> +globalDir.create(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> +
> +function write_manifest(dir, filename, manifest) {

Camel case rather than underscore, please.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:34
(Diff revision 3)
> +  if (typeof manifest != "string") {
> +    manifest = JSON.stringify(manifest);
> +  }
> +
> +  let path = OS.Path.join(dir.path, filename);
> +  OS.File.writeAtomic(path, manifest);

`return`

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:40
(Diff revision 3)
> +}
> +
> +// Test that the default paths searched for native host manifests
> +// are the ones we expect.
> +// XXX need to register the xre directory provider?  bah, is that going
> +// to clash with registering the test-specific provider below?

You can move this to a different test file where you don't override the provider.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:43
(Diff revision 3)
> +// are the ones we expect.
> +// XXX need to register the xre directory provider?  bah, is that going
> +// to clash with registering the test-specific provider below?
> +if (false) {
> +add_task(function* test_default_paths() {
> +  let home = Services.dirsvc.get("Home", Ci.nsIFile).path;

`OS.Constants.Path.homeDir`

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:56
(Diff revision 3)
> +    break;
> +
> +  case "linux":
> +    expected = [
> +      OS.Path.join(home, ".mozilla/native-messaging-hosts"),
> +      OS.Path.join("/usr", (OS.Constants.Sys.bits == 32) ? "lib" : "lib64", "mozilla/native-messaging-hosts"),

The path for 64 bit libs is highly distro-dependent. Many distros put 64 bit libs in /usr/lib and 32 bit in /usr/lib32

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:61
(Diff revision 3)
> +      OS.Path.join("/usr", (OS.Constants.Sys.bits == 32) ? "lib" : "lib64", "mozilla/native-messaging-hosts"),
> +    ];
> +    break;
> +
> +  default:
> +    break;

We should disable these tests on unsupported platforms, and fail if we get a platform we don't expect.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:83
(Diff revision 3)
> +      } else if (property == "LocAppData") {
> +        return globalDir.clone();
> +      }
> +      return null;
> +    },
> +  });

Need to register a cleanup function to unregister this provider at the end of the test.

::: toolkit/components/extensions/test/xpcshell/test_native_messaging.js:90
(Diff revision 3)
> +
> +// Test of HostManifestManager.lookupApplication() begin here...
> +
> +let context = {
> +  url: null,
> +  logError: error => { },

`logError(error) {}`

::: toolkit/components/extensions/test/xpcshell/xpcshell.ini:17
(Diff revision 3)
>  [test_ext_json_parser.js]
>  [test_ext_manifest_content_security_policy.js]
>  [test_ext_schemas.js]
>  [test_getAPILevelForWindow.js]
> +[test_native_messaging.js]
> +# Re-enable for windows with bug bug 1270359

s/bug bug/bug/
s/windows/Windows/

::: toolkit/xre/nsXREDirProvider.cpp:242
(Diff revision 3)
>    }
>    file.swap(*aResult);
>    return NS_OK;
>  }
>  
> +#if defined(XP_UNIX) || defined(XP_MACOSX)

`XP_MACOSX` implies `XP_UNIX`

::: toolkit/xre/nsXREDirProvider.cpp:244
(Diff revision 3)
>    return NS_OK;
>  }
>  
> +#if defined(XP_UNIX) || defined(XP_MACOSX)
> +static nsresult
> +GetSystemParentDirectory(nsIFile** aFile)

I find this function name a bit confusing, but I can't think of anything obviously better.

::: toolkit/xre/nsXREDirProvider.cpp:251
(Diff revision 3)
> +  nsresult rv;
> +  nsCOMPtr<nsIFile> localDir;
> +#if defined(XP_MACOSX)
> +  FSRef fsRef;
> +  OSErr err = ::FSFindFolder(kOnSystemDisk, kApplicationSupportFolderType, kCreateFolder, &fsRef);
> +  NS_ENSURE_FALSE(err, NS_ERROR_FAILURE);

I know that you're just moving this code, but if we're touching old code, we should take the opportunity to update it to the prevailing style and remove the `NS_ENSURE_*` macros.

::: toolkit/xre/nsXREDirProvider.cpp:262
(Diff revision 3)
> +  NS_ENSURE_TRUE(dirFileMac, NS_ERROR_UNEXPECTED);
> +
> +  rv = dirFileMac->InitWithFSRef(&fsRef);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  localDir = do_QueryInterface(dirFileMac, &rv);

We can replace most of the above with a call to `GetOSXFolderType`.

::: toolkit/xre/nsXREDirProvider.cpp:265
(Diff revision 3)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  localDir = do_QueryInterface(dirFileMac, &rv);
> +
> +  static const char* const sXR = "Mozilla";
> +  rv = localDir->AppendNative(nsDependentCString(sXR));

`localDir->AppendNative(NS_LITERAL_CSTRING("Mozilla"))`

::: toolkit/xre/nsXREDirProvider.cpp:268
(Diff revision 3)
> +
> +  static const char* const sXR = "Mozilla";
> +  rv = localDir->AppendNative(nsDependentCString(sXR));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +#elif defined(XP_UNIX)
> +  static const char *const sysSExtDir = 

Trailing space.

::: toolkit/xre/nsXREDirProvider.cpp:362
(Diff revision 3)
>    }
>    else if (!strcmp(aProperty, NS_APP_APPLICATION_REGISTRY_DIR) ||
>             !strcmp(aProperty, XRE_USER_APP_DATA_DIR)) {
>      rv = GetUserAppDataDirectory(getter_AddRefs(file));
>    }
> +#if defined (XP_UNIX) || defined(XP_MACOSX)

No space after `defined`. Also, `XP_MACOSX` implies `XP_UNIX`.

::: toolkit/xre/nsXREDirProvider.cpp:374
(Diff revision 3)
> +#if defined(XP_MAXOSX)
> +        "NativeMessagingHosts";
> +#else
> +        "native-messaging-hosts";
> +#endif
> +      rv = localDir->AppendNative(nsDependentCString(dirname));

I know this is done this way elsewhere in the file, but it's a really weird way to do it. It should be something more like:

    NS_NAMED_LITERAL_CSTRING(dirname,
    #if defined(XP_MAXOSX)
                             "NativeMessagingHosts"
    #else
                             "native-messaging-hosts"
    #endif
    );
    rv = localDir->AppendNative(dirname);

We should probably do the same for the above.

::: toolkit/xre/nsXREDirProvider.cpp:376
(Diff revision 3)
> +#else
> +        "native-messaging-hosts";
> +#endif
> +      rv = localDir->AppendNative(nsDependentCString(dirname));
> +      if (NS_SUCCEEDED(rv)) {
> +        file = localDir.forget();

`localDir.swap(file)`
https://reviewboard.mozilla.org/r/51251/#review48203

> Okay, is there anything existing in OS.Path that I can use here or am I writing the validator myself?  A quick glance didn't turn up anything...

I don't think there is. There's a FILE_ILLEGAL_CHARACTERS macro, but it's only available to native code, and there are probably some other things to take into consideration as well.

Trying to init a nsIFile object with an illegal path should throw, but it would be unfortunate to have to resort to that.
https://reviewboard.mozilla.org/r/51251/#review48203

> I don't think there is. There's a FILE_ILLEGAL_CHARACTERS macro, but it's only available to native code, and there are probably some other things to take into consideration as well.
> 
> Trying to init a nsIFile object with an illegal path should throw, but it would be unfortunate to have to resort to that.

This is making me think that trying to validate the path during schema validation just isn't worth it.  We'll have to have decent error reporting for the case that the path appears valid but doesn't exist or isn't accessible due to permissions etc.  Illegal characters will get also caught at that point so since there doesn't appear to be a reasonable way to do it easily here, I'm inclined to just let it be a string and punt to runtime errors.
https://reviewboard.mozilla.org/r/51251/#review48535

> No space after `defined`. Also, `XP_MACOSX` implies `XP_UNIX`.

Yeah, this came from here: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1416 and I figured I should remain consistent with the rest of this file (which has the same pattern in several other places as well)
https://reviewboard.mozilla.org/r/51251/#review48535

> You can move this to a different test file where you don't override the provider.

okay, but the XRE provider doesn't seem to be created (or at least registered with the Services.dirsvc) by default and its not clear to me exactly how to make that happen.
https://reviewboard.mozilla.org/r/51251/#review48535

> We should make this a lazy getter. Also, please use camel case rather than underscores. And I don't think the leading underscore is necessary here.

I think a lazy getter might be overkill, this just gets called once from `init()`.  I exposed it in this way so I could also call it from tests (hence the leading underscore since its not really meant as a "public" interface).  I'll convert everything to camelCase, when I write stuff at night I revert to my old habits :-)

> Is there a reason you're assigning the return value of `Cu.import` for these, but not for the others?

laziness with regard to eslint and getting the imported symbols defined.  I can just do naked Cu.import calls and then add an eslint globals comment if you feel strongly about it.

> The path for 64 bit libs is highly distro-dependent. Many distros put 64 bit libs in /usr/lib and 32 bit in /usr/lib32

I'm mirroring what we do right now for system extensions (https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1452) where lib is hard-coded for 32-bit and we don't try to figure out what distribution it is and where it wants things to go.

> Need to register a cleanup function to unregister this provider at the end of the test.

Even for xpcshell?  Every test runs in a fresh process...
https://reviewboard.mozilla.org/r/51251/#review48535

> laziness with regard to eslint and getting the imported symbols defined.  I can just do naked Cu.import calls and then add an eslint globals comment if you feel strongly about it.

The AppConstants global should at least be automatically defined by the ESLint plugin. In general, we try to avoid using the return value from `Cu.import`, since it just returns the module global, rather than only the exported symbols.

> I'm mirroring what we do right now for system extensions (https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1452) where lib is hard-coded for 32-bit and we don't try to figure out what distribution it is and where it wants things to go.

That, at least, checks the `HAVE_USR_LIB64_DIR` constant, which won't be defined on systems that don't use /usr/lib64

> Even for xpcshell?  Every test runs in a fresh process...

Yes, tests are still expected to clean up after themselves.
Review commit: https://reviewboard.mozilla.org/r/52053/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52053/
Attachment #8749935 - Attachment description: MozReview Request: Bug 1270356 first draft of manifest parsing → MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag
Attachment #8751500 - Flags: review?(kmaglione+bmo)
Attachment #8749935 - Flags: feedback?(kmaglione+bmo) → review?(kmaglione+bmo)
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/3-4/
https://reviewboard.mozilla.org/r/51251/#review48535

> That, at least, checks the `HAVE_USR_LIB64_DIR` constant, which won't be defined on systems that don't use /usr/lib64

Right, and the provider uses the same logic for how this directory gets exported.  So, on a system that doesn't use /usr/lib64, this unit test will fail but it will be a false negative.  Is it practical to get at HAVE_USR_LIB64_DIR from javascript?  If not, I'm not sure how to fix this.

> Yes, tests are still expected to clean up after themselves.

There are many existing xpcshell tests that register directory providers and never unregister them, but I've fixed this one.
https://reviewboard.mozilla.org/r/51251/#review48535

> Right, and the provider uses the same logic for how this directory gets exported.  So, on a system that doesn't use /usr/lib64, this unit test will fail but it will be a false negative.  Is it practical to get at HAVE_USR_LIB64_DIR from javascript?  If not, I'm not sure how to fix this.

You can add it to AppConstants.jsm
https://reviewboard.mozilla.org/r/51251/#review48535

> You can add it to AppConstants.jsm

The other symbols there all seem much higher level thing (e.g., whether individual features are enabled).
How about if instead the test verifies that on Linux the path is either lib or lib64 but it doesn't actually care which one it is?
Its not as precise but we don't need to pollute AppConstants...
Comment on attachment 8751500 [details]
MozReview Request: Bug 1270356 Part 1: Add native messaging paths to XRE directory provider r?kmag

https://reviewboard.mozilla.org/r/52053/#review48961

::: toolkit/xre/nsXREDirProvider.cpp:246
(Diff revision 1)
>  
> +#if defined(XP_UNIX) || defined(XP_MACOSX)
> +/**
> + * Get the directory that is the parent of the system-wide directories
> + * for extensions and native-messaing manifests.
> + * 

Nit: Trailing space.

::: toolkit/xre/nsXREDirProvider.cpp:259
(Diff revision 1)
> +  nsresult rv;
> +  nsCOMPtr<nsIFile> localDir;
> +#if defined(XP_MACOSX)
> +  rv = GetOSXFolderType(kOnSystemDisk, kApplicationSupportFolderType, getter_AddRefs(localDir));
> +  if (NS_SUCCEEDED(rv)) {
> +    localDir = do_QueryInterface(localDir, &rv);

This isn't necessary. It's a (fairly expensive) no-op.

::: toolkit/xre/nsXREDirProvider.cpp:264
(Diff revision 1)
> +    localDir = do_QueryInterface(localDir, &rv);
> +    if (NS_SUCCEEDED(rv)) {
> +      rv = localDir->AppendNative(NS_LITERAL_CSTRING("Mozilla"));
> +    }
> +  }
> +#elif defined(XP_UNIX)

Nit: `defined(XP_UNIX)` will always be true here.

::: toolkit/xre/nsXREDirProvider.cpp:360
(Diff revision 1)
>    else if (!strcmp(aProperty, NS_APP_APPLICATION_REGISTRY_DIR) ||
>             !strcmp(aProperty, XRE_USER_APP_DATA_DIR)) {
>      rv = GetUserAppDataDirectory(getter_AddRefs(file));
>    }
> +#if defined(XP_UNIX) || defined(XP_MACOSX)
> +  else if (!strcmp(aProperty, XRE_SYS_NATIVE_MESSAGING_MANIFESTS)) {

Can we move the tests for these keys into this patch?

::: toolkit/xre/nsXREDirProvider.cpp:1507
(Diff revision 1)
> -
> -  localDir = do_QueryInterface(dirFileMac, &rv);
>  
> -  static const char* const sXR = "Mozilla";
> -  rv = localDir->AppendNative(nsDependentCString(sXR));
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = GetSystemParentDirectory(getter_AddRefs(localDir));
> +  if (NS_SUCCEEDED(rv)) {
> +    static const char* const sExtensions =

Can we also change this to use `NS_NAMED_LITERAL_CSTRING`?
Attachment #8751500 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

https://reviewboard.mozilla.org/r/51251/#review48965

::: toolkit/components/extensions/NativeMessaging.jsm:18
(Diff revision 4)
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://devtools/shared/event-emitter.js");
> +
> +const {
> +  OS,
> +  TextDecoder,

Please use `Cu.importGlobalProperties(["TextDecoder"])` instead, and use defineLazyModuleGetter for `OS`.

::: toolkit/components/extensions/NativeMessaging.jsm:62
(Diff revision 4)
> +
> +  _tryPath(dir, application, context) {
> +    let path = OS.Path.join(dir, `${application}.json`);
> +    return Promise.resolve()
> +      .then(() => OS.File.read(path))
> +      .then(raw => this.decoder.decode(raw))

Just path `{encoding: "utf-8"}` to OS.File.read instead, and it will do this for you.

::: toolkit/components/extensions/NativeMessaging.jsm:81
(Diff revision 4)
> +
> +  _tryPaths(application, dirs, context) {
> +    return Task.spawn(function* () {
> +      for (let dir of dirs) {
> +        let path = yield this._tryPath(dir, application, context)
> +            .catch(err => {

You can use a try-catch here instead:

    try {
      return yield this._tryPath(...);
    } catch (e) {
      // ...
    }

::: toolkit/components/extensions/NativeMessaging.jsm:90
(Diff revision 4)
> +        if (path) {
> +          return path;
> +        }
> +      }
> +      return null;
> +    }.bind(this));

You can just use `Task.async` on the method instead:

    _tryPaths: Task.async(function* () { ...

::: toolkit/components/extensions/NativeMessaging.jsm:93
(Diff revision 4)
> +      }
> +      return null;
> +    }.bind(this));
> +  },
> +
> +  lookupApplication(application, context) {

Can you please document at least what this function does, and what its arguments are?

::: toolkit/components/extensions/test/mochitest/test_chrome_native_messaging_paths.html:19
(Diff revision 4)
> +"use strict";
> +
> +const {
> +  interfaces: Ci,
> +  utils: Cu,
> +} = Components;

We normally do this on one line, and when you define any of these shortcuts, you should define all of Cc, Ci, Cr, and Cu, because people will expect them to exist.

::: toolkit/components/extensions/test/mochitest/test_chrome_native_messaging_paths.html:55
(Diff revision 4)
> +    default:
> +      ok(false, `This test should be skipped on ${AppConstants.platform}`);
> +      break;
> +  }
> +
> +  ok(ObjectUtils.deepEqual(HostManifestManager._searchPaths(), expected),

It would be nice to use Assert.deepEqual for this instead, since it has better reporting. You'd have to import Assert.jsm and hook it up to the reporting functions, though.
Attachment #8749935 - Flags: review?(kmaglione+bmo) → review+
https://reviewboard.mozilla.org/r/51251/#review48535

> The other symbols there all seem much higher level thing (e.g., whether individual features are enabled).
> How about if instead the test verifies that on Linux the path is either lib or lib64 but it doesn't actually care which one it is?
> Its not as precise but we don't need to pollute AppConstants...

AppConstants contains any config variables that we used to use the preprocessor for, but still need to access from JS. Don't worry too much about polluting it. That's what it's there for.
Comment on attachment 8751500 [details]
MozReview Request: Bug 1270356 Part 1: Add native messaging paths to XRE directory provider r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52053/diff/1-2/
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/4-5/
https://reviewboard.mozilla.org/r/51251/#review48965

> It would be nice to use Assert.deepEqual for this instead, since it has better reporting. You'd have to import Assert.jsm and hook it up to the reporting functions, though.

This became irrelevant when I rewrote part of the test after moving it to the first patch, but I actually specifically avoided Assert.jsm after reading https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#Test_functions which says:
"Additionally, tests can use the assertion methods provided by Assert.jsm. It implements the CommonJS Unit Testing specification version 1.1, which provides a basic, standardized interface for performing in-code logical assertions with optional, customizable error reporting. Usage of these functions is strongly discouraged in mochitest-plain and mochitest-chrome tests."
Is the MDN page out of date?
Comment on attachment 8751500 [details]
MozReview Request: Bug 1270356 Part 1: Add native messaging paths to XRE directory provider r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52053/diff/2-3/
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/5-6/
https://reviewboard.mozilla.org/r/51251/#review48965

> This became irrelevant when I rewrote part of the test after moving it to the first patch, but I actually specifically avoided Assert.jsm after reading https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#Test_functions which says:
> "Additionally, tests can use the assertion methods provided by Assert.jsm. It implements the CommonJS Unit Testing specification version 1.1, which provides a basic, standardized interface for performing in-code logical assertions with optional, customizable error reporting. Usage of these functions is strongly discouraged in mochitest-plain and mochitest-chrome tests."
> Is the MDN page out of date?

It's not out of date, but it doesn't really apply in this case, as long as we set up the reporting function properly.
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

https://reviewboard.mozilla.org/r/51251/#review49197

::: toolkit/components/extensions/NativeMessaging.jsm:88
(Diff revisions 4 - 6)
> +   * Search for a valid native host manifest for the given application name.
> +   * The directories searched and rules for manifest validation are all
> +   * detailed in the native messaging documentation.
> +   *
> +   * @param {string} application The name of the applciation to search for.
> +   * @param {ExtensionContext} context The extension context.

I don't think this is accurate. The contexts that we pass to Schemas.jsm are a different thing from extension contexts.
Attachment #8749935 - Flags: review+
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/6-7/
Attachment #8749935 - Flags: review?(kmaglione+bmo)
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/7-8/
Cleaned up error handling and also made lookup return both the path and the manifest contents (which we'll need in connectNative).  I think this should be good to go...
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

https://reviewboard.mozilla.org/r/51251/#review50112

::: toolkit/components/extensions/NativeMessaging.jsm:57
(Diff revisions 6 - 8)
>        .then(data => {
> -        let manifest = JSON.parse(data);
> +        let manifest;
> +        try {
> +          manifest = JSON.parse(data);
> +        } catch (ex) {
> +          return null;

Do we really want to silently ignore JSON parse errors here? It seems like that would make things pretty hard to debug.

::: toolkit/components/extensions/NativeMessaging.jsm:71
(Diff revisions 6 - 8)
>          if (manifest.name != application) {
>            throw new Error(`Name mismatch (${path} contains name property ${manifest.name})`);
>          }
>          return normalized.value;
> +      })
> +      .catch(ex => {

Please move to previous line.

::: toolkit/components/extensions/NativeMessaging.jsm:72
(Diff revisions 6 - 8)
>            throw new Error(`Name mismatch (${path} contains name property ${manifest.name})`);
>          }
>          return normalized.value;
> +      })
> +      .catch(ex => {
> +        if (ex instanceof OS.File.Error && ex.becauseNoSuchFile) {

I'm not sure we need the `instanceof` check.

::: toolkit/components/extensions/NativeMessaging.jsm:82
(Diff revisions 6 - 8)
>    },
>  
>    _tryPaths: Task.async(function* (application, dirs, context) {
>      for (let dir of dirs) {
> -      try {
> -        let path = yield this._tryPath(dir, application, context);
> +      let path = OS.Path.join(dir, `${application}.json`);
> +      let manifest = yield this._tryPath(path, application, context);

So, we want to bail out here now if one of the manifests has an error, even if we haven't tried all paths?
Attachment #8749935 - Flags: review?(kmaglione+bmo)
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51251/diff/8-9/
Attachment #8749935 - Flags: review?(kmaglione+bmo)
https://reviewboard.mozilla.org/r/51251/#review50112

> Please move to previous line.

It seems more readable to me to have it lined up with all the other handlers above it but this isn't worth quibbling over, I've changed it.

> I'm not sure we need the `instanceof` check.

This just came from https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File.Error#Example.3A_Finding_out_whether_the_problem_is_due_to_a_file_that_does_not_exist
https://reviewboard.mozilla.org/r/51251/#review50112

> So, we want to bail out here now if one of the manifests has an error, even if we haven't tried all paths?

Whoops, the only case where we were bailing out was if the application property didn't match the file name.  For other manifest errors, `_tryPath` returns null which lets us proceed.  In the case of an unexpected error, we can still get an exception which will bail out.
https://reviewboard.mozilla.org/r/51251/#review50112

> This just came from https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File.Error#Example.3A_Finding_out_whether_the_problem_is_due_to_a_file_that_does_not_exist

It seems unnecessarily paranoid. If the error has a truthy `becauseNoSuchFile` property, we should be safe assuming it's an OS.File.Error object.
Comment on attachment 8749935 [details]
MozReview Request: Bug 1270356 Part 2: Implement parsing and validation of native host manifests r?kmag

https://reviewboard.mozilla.org/r/51251/#review50176
Attachment #8749935 - Flags: review?(kmaglione+bmo) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31f81c1c3b5
Part 1: Add native messaging paths to XRE directory provider r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/401305ffbd99
Part 2: Implement parsing and validation of native host manifests r=kmag
https://hg.mozilla.org/mozilla-central/rev/d31f81c1c3b5
https://hg.mozilla.org/mozilla-central/rev/401305ffbd99
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1278705
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: