1270357 - Implement runtime.connectNative for Linux/Mac

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andrew Swan [:aswan]]

https://developer.chrome.com/extensions/runtime#method-connectNative

Note the dependencies, when those are finished, this is a modest effort.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1270357 Implement runtime.connectNative()

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/1-2/

Comment 3

[Andrew Swan [:aswan]]

I should mention, the patch for review depends on and will only run with the current patches for 1269501 and 1270356 in place.

Comment 4

[gorf4673]

https://reviewboard.mozilla.org/r/52347/#review49323

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:52
(Diff revision 2)
> +    rawlen = sys.stdin.read(4)
> +    msglen = struct.unpack('@I', rawlen)[0]
> +    msg = sys.stdin.read(msglen)

Conern about testing binary stream for stdin/stdout
Which python version is in use?  
Does Python 2 need binary mode (-u) when native.py is run.
Otherwise Python 3 shouldn't this be using buffer - 
sys.stdout.buffer.read()/write().

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:105
(Diff revision 2)
> +  let payloads = [
> +    "this is a string",
> +    {test: "hello"},
> +    {
> +      what: "An object with a few properties",
> +      number: 123,
> +      bool: true,
> +      nested: {what: "another object"},
> +    },
> +  ];

Does this need to be extended to test issues with binary channel, UTF-8 chars that 
 - would cause issue with ascii.
 - of different lengths
On windows handling of \\r\\n

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/52347/#review49435

::: toolkit/components/extensions/NativeMessaging.jsm:124
(Diff revision 2)
> +    // We want a close() notification when the window is destroyed.
> +    this.context.callOnClose(this);
> +
> +    this.encoder = new TextEncoder();
> +    this.proc = null;
> +    this.connectPromise = null;

this.connectPromise seems to be currently unused.

::: toolkit/components/extensions/NativeMessaging.jsm:129
(Diff revision 2)
> +    this.startupPromise = HostManifestManager.lookupApplication(application, context)
> +      .then(hostInfo => {
> +        if (!hostInfo) {
> +          throw new context.cloneScope.Error(`No such native application ${application}`);
> +        }

It is not an issue, but it seems a bit strange that HostManifestManager.lookupApplication doesn't reject when no native application is found.

::: toolkit/components/extensions/NativeMessaging.jsm:193
(Diff revision 2)
> +    let buffer = this.sendQueue.shift();
> +    let uintArray = new Uint32Array(1);
> +    uintArray[0] = buffer.byteLength;
> +
> +    this.writePromise = this._writeStdin(uintArray.buffer)
> +      .then(written => this._writeStdin(buffer))

I think that a small inline comment here that briefly explains that "based on the connectNative protocol conventions, the first byte is the length of the data that is going to be sent to the native application" could be helpful to immediately recognize what is happening and why (same thing can be helpful on the MAX_WRITE const to explain the reason of the "-1" part of its defined value).

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:170
(Diff revision 2)
> +  yield OS.File.writeAtomic(jsonPath, JSON.stringify(manifest));
> +
> +  function background() {
> +    let port = browser.runtime.connectNative("test");
> +    port.onDisconnect.addListener(() => {
> +      browser.test.sendMessage("result", "disconnected");

Errors related to the native app connection (both startup errors or errors that could happen during the message exchange) should be probably reported by setting `browser.runtime.lastError` accordingly when the disconnect listener is called (or it should be listed in the current chrome incompatibilities)

e.g. follows a link to a Chromium connectNative example:
https://chromium.googlesource.com/chromium/src/+/master/chrome/common/extensions/docs/examples/api/nativeMessaging/app/main.js#43

Comment 6

[gorf4673]

https://reviewboard.mozilla.org/r/52347/#review49435

> I think that a small inline comment here that briefly explains that "based on the connectNative protocol conventions, the first byte is the length of the data that is going to be sent to the native application" could be helpful to immediately recognize what is happening and why (same thing can be helpful on the MAX_WRITE const to explain the reason of the "-1" part of its defined value).

small correction "the first byte" - should be "the first 4 bytes"

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/2-3/

Comment 8

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review49323

> Conern about testing binary stream for stdin/stdout
> Which python version is in use?  
> Does Python 2 need binary mode (-u) when native.py is run.
> Otherwise Python 3 shouldn't this be using buffer - 
> sys.stdout.buffer.read()/write().

I think this is only an issue on windows?  I'll address it while working on bug 1270359.

> Does this need to be extended to test issues with binary channel, UTF-8 chars that 
>  - would cause issue with ascii.
>  - of different lengths
> On windows handling of \\r\\n

Added a test with non-ascii character, windows line ending things will be handled in bug 1270359.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

https://reviewboard.mozilla.org/r/52347/#review50116

::: toolkit/components/extensions/NativeMessaging.jsm:14
(Diff revision 3)
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/AppConstants.jsm");
> +/* global EventEmitter */

I'd prefer if we just put a single `/* globals ... */` comment near the top of the file.

::: toolkit/components/extensions/NativeMessaging.jsm:44
(Diff revision 3)
> +// the write limit is imposed by the "wire protocol" in which message
> +// boundaries are defined by preceding each message with its length as
> +// 4-byte unsigned integer so this is the largest value that can be
> +// represented.  Good luck generating a serialized message that large,
> +// the practical write limit is likely to be dictated by available memory.
> +const MAX_READ = 1024 * 1024;

Nit: You can use `1024 ** 2` and `1024 ** 3` for these. Although I honestly don't care whether or not you do.

::: toolkit/components/extensions/NativeMessaging.jsm:45
(Diff revision 3)
> +// boundaries are defined by preceding each message with its length as
> +// 4-byte unsigned integer so this is the largest value that can be
> +// represented.  Good luck generating a serialized message that large,
> +// the practical write limit is likely to be dictated by available memory.
> +const MAX_READ = 1024 * 1024;
> +const MAX_WRITE = 4 * 1024 * 1024 * 1024 - 1;

Can you maybe make this `0xffffffff` instead, just so it's more obvious that it's the max u32?

::: toolkit/components/extensions/NativeMessaging.jsm:49
(Diff revision 3)
> +const MAX_READ = 1024 * 1024;
> +const MAX_WRITE = 4 * 1024 * 1024 * 1024 - 1;
> +
> +// Preferences that can lower the message size limits above,
> +// used for testing the limits.
> +const PREF_MAX_READ = "webextensions.native-messaing.max-read";

Maybe "max-input-message-bytes"? Same for output?

::: toolkit/components/extensions/NativeMessaging.jsm:151
(Diff revision 3)
> +    this.readPromise = null;
> +    this.sendQueue = [];
> +    this.writePromise = null;
> +
> +    // Grab these once at startup
> +    this.maxRead = Math.min(MAX_READ, Preferences.get(PREF_MAX_READ, MAX_READ));

We should cache these values. See XPCOMUtils.defineLazyPreferenceGetter.

::: toolkit/components/extensions/NativeMessaging.jsm:171
(Diff revision 3)
> +          arguments: [hostInfo.path],
> +          workdir: OS.Path.dirname(hostInfo.manifest.path),
> +        };
> +        return Subprocess.call(subprocessOpts);
> +      })
> +      .then(proc => {

Please move to the previous line.

::: toolkit/components/extensions/NativeMessaging.jsm:177
(Diff revision 3)
> +        this.startupPromise = null;
> +        this.proc = proc;
> +        this._startRead();
> +        this._startWrite();
> +      })
> +      .catch(err => {

Please move to the previous line.

::: toolkit/components/extensions/NativeMessaging.jsm:185
(Diff revision 3)
> +      });
> +  }
> +
> +  _startRead() {
> +    if (this.readPromise) {
> +      let err = new Error("Entered _startRead() while readPromise is non-null");

No need to assign this to a variable.

Is there any reason you don't just throw?

::: toolkit/components/extensions/NativeMessaging.jsm:196
(Diff revision 3)
> +        if (len > this.maxRead) {
> +          throw new Error(`Native application tried to send a message of ${len} bytes, which exceeds the limit of ${this.maxRead} bytes.`);
> +        }
> +        return this.proc.stdout.readJSON(len);
> +      })
> +      .then(msg => {

Please move to previous line.

::: toolkit/components/extensions/NativeMessaging.jsm:210
(Diff revision 3)
> +
> +  _writeStdin(buffer) {
> +    let len = buffer.byteLength;
> +    return this.proc.stdin.write(buffer)
> +      .then(result => {
> +        if (result.bytesWritten != len) {

This is guaranteed not to happen. In fact, this function doesn't seem necessary at all.

::: toolkit/components/extensions/NativeMessaging.jsm:211
(Diff revision 3)
> +  _writeStdin(buffer) {
> +    let len = buffer.byteLength;
> +    return this.proc.stdin.write(buffer)
> +      .then(result => {
> +        if (result.bytesWritten != len) {
> +          throw new Error(`tried to write ${len} bytes but write returned ${result.bytesWritten}`);

Please capitalize.

::: toolkit/components/extensions/NativeMessaging.jsm:220
(Diff revision 3)
> +  }
> +
> +  _startWrite() {
> +    if (this.sendQueue.length == 0) {
> +      return;
> +    }

We should assert that there's no current writePromise here.

::: toolkit/components/extensions/NativeMessaging.jsm:222
(Diff revision 3)
> +  _startWrite() {
> +    if (this.sendQueue.length == 0) {
> +      return;
> +    }
> +    let buffer = this.sendQueue.shift();
> +    let uintArray = new Uint32Array(1);

`Uint32Array.of(buffer.byteLength)`

::: toolkit/components/extensions/NativeMessaging.jsm:231
(Diff revision 3)
> +      .then(written => this._writeStdin(buffer))
> +      .then(() => {
> +        this.writePromise = null;
> +        this._startWrite();
> +      })
> +      .catch(err => {

Please move to previous line.

::: toolkit/components/extensions/NativeMessaging.jsm:244
(Diff revision 3)
> +      throw new this.context.cloneScope.Error("Attempt to postMessage on disconnected port");
> +    }
> +
> +    let json;
> +    try {
> +      json = JSON.stringify(msg);

We should use the JSON object belonging to the scope. There are side-effects to using ours.

It might actually be worth writing a utility function in Cu to handle this properly...

::: toolkit/components/extensions/NativeMessaging.jsm:267
(Diff revision 3)
> +  // be an Error, it will be provided to the extension via runtime.lastError
> +  // in the onDisconnect callback
> +  _cleanup(err) {
> +    this.context.forgetOnClose(this);
> +    if (this.proc) {
> +      this.proc.kill();

We should try closing stdin and allowing the process to exit gracefully first, and then kill after a timeout.

::: toolkit/components/extensions/NativeMessaging.jsm:287
(Diff revision 3)
> +  portAPI() {
> +    let portObj = Cu.createObjectIn(this.context.cloneScope);
> +
> +    let publicAPI = {
> +      name: this.name,
> +      disconnect: () => {

Please add a blank line before each method.

::: toolkit/components/extensions/NativeMessaging.jsm:295
(Diff revision 3)
> +      postMessage: msg => {
> +        this.send(msg);
> +      },
> +      onDisconnect: new SingletonEventManager(this.context, "native.onDisconnect", fire => {
> +        let listener = (what, err) => {
> +          let handler = () => runSafeSync(this.context, fire);

`this.context.runSafe(fire)`

::: toolkit/components/extensions/NativeMessaging.jsm:297
(Diff revision 3)
> +      },
> +      onDisconnect: new SingletonEventManager(this.context, "native.onDisconnect", fire => {
> +        let listener = (what, err) => {
> +          let handler = () => runSafeSync(this.context, fire);
> +          if (err) {
> +            this.context.withLastError({message: err.message}, handler);

Does Chrome ever set `lastError` when it calls this handler?

::: toolkit/components/extensions/NativeMessaging.jsm:309
(Diff revision 3)
> +          this.off("disconnect", listener);
> +        };
> +      }).api(),
> +      onMessage: new SingletonEventManager(this.context, "native.onMessage", fire => {
> +        let listener = (what, msg) => {
> +          runSafeSync(this.context, fire, msg);

`this.context.runSafe(fire, msg)`

::: toolkit/components/extensions/NativeMessaging.jsm:318
(Diff revision 3)
> +          this.off("message", listener);
> +        };
> +      }).api(),
> +    };
> +
> +    injectAPI(publicAPI, portObj);

We should probably just use `cloneInto` with `cloneFunctions` for this.

::: toolkit/components/extensions/ext-runtime.js:5
(Diff revision 3)
>  "use strict";
>  
>  var {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Please either sort by name, or add a blank line after the XPCOMUtils import.

::: toolkit/components/extensions/ext-runtime.js:57
(Diff revision 3)
>                                       responseCallback);
>          }
>          return context.messenger.sendMessage(Services.cpmm, message, recipient, responseCallback);
>        },
>  
> +      connectNative: function(application) {

`connectNative(application) {`

::: toolkit/components/extensions/ext-runtime.js:59
(Diff revision 3)
>          return context.messenger.sendMessage(Services.cpmm, message, recipient, responseCallback);
>        },
>  
> +      connectNative: function(application) {
> +        if (!extension.hasPermission("nativeMessaging")) {
> +          throw new context.cloneScope.Error(

Please `return Promise.reject({message: ...})` instead.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:21
(Diff revision 3)
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +/* global OS */
> +Cu.import("resource://gre/modules/osfile.jsm");

I'd generally prefer that we have a single `/* globals ... */` comment near the top of the file.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:40
(Diff revision 3)
> +    return null;
> +  },
> +};
> +
> +Services.dirsvc.registerProvider(dirProvider);
> +function* cleanup() {

Any particular reason you don't use `do_register_cleanup` for this?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:75
(Diff revision 3)
> +  allowed_extensions: [ID],
> +};
> +
> +add_task(function* setup_scripts() {
> +  const PERMS = {unixMode: parseInt("777", 8)};
> +  const ECHO_SCRIPT = `#!/usr/bin/env python

Hm. This is problematic.

We need to use the PYTHON executable defined by the build config, and I can't think of a way to get it here other than creating a preprocessed file.

We also need to pass `-U`, especially on Windows. Which means we probably need to wrap this in a batch file there.

We should probably use `String.raw` for all of these, too.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:76
(Diff revision 3)
> +};
> +
> +add_task(function* setup_scripts() {
> +  const PERMS = {unixMode: parseInt("777", 8)};
> +  const ECHO_SCRIPT = `#!/usr/bin/env python
> +import sys, struct

One import per line, sorted by module name, in python code. Blank line after the last import. Same for the ones below.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:123
(Diff revision 3)
> +  }
> +
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background: `(${background})()`,
> +    manifest: {
> +      applications: {gecko: {id: ID}},

No need for this. Just pass the ID as the second argument to `loadExtension`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:148
(Diff revision 3)
> +    let waitForEcho = extension.awaitMessage("message")
> +                               .then(response => {
> +                                 isDeeply(response, payload, `Echoed a $message of type ${typeof payload}`);
> +                               });
> +    extension.sendMessage("send", payload);
> +    yield waitForEcho;

This would be much simpler as:

    extension.sendMessage("send", payload);
    let response = yield extension.awaitMessage("message");
    isDeeply(response, payload, `Echoed a message of type ${typeof payload}`);

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:156
(Diff revision 3)
> +  yield extension.unload();
> +});
> +
> +// Test the limit on message size for writing
> +add_task(function* test_write_limit() {
> +  Services.prefs.setIntPref(PREF_MAX_WRITE, 10);

Please register a cleanup function to clear this as soon as you set it, in case something throws.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:178
(Diff revision 3)
> +      permissions: ["nativeMessaging"],
> +    },
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("result").then(errmsg => {

`let errmsg = yield extension.awaitMessage("result"); ...`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:214
(Diff revision 3)
> +      permissions: ["nativeMessaging"],
> +    },
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("result").then(result => {

`let result = yield extension.awaitMessage("result"); ...`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:257
(Diff revision 3)
> +add_task(function* test_app_permission() {
> +  let manifest = Object.assign({}, ECHO_MANIFEST);
> +  manifest.allowed_extensions = ["[e-mail]"];
> +
> +  let jsonPath = getPath("badperms.json");
> +  yield OS.File.writeAtomic(jsonPath, JSON.stringify(manifest));

Can we just use the same manifest but an add-on with a different ID?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:282
(Diff revision 3)
> +      permissions: ["nativeMessaging"],
> +    },
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("result").then(result => {

`let result = yield ...`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:308
(Diff revision 3)
> +      permissions: ["nativeMessaging"],
> +    },
> +  });
> +
> +  yield extension.startup();
> +  yield extension.awaitMessage("result").then(msg => {

`let msg = yield ...`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:315
(Diff revision 3)
> +    is(msg.args[1], INFO_MANIFEST_PATH, "Command line argument is the path to the native host manifest");
> +    is(msg.cwd, OS.Path.dirname(INFO_PATH), "Working directory is the directory containing the native appliation");
> +  });
> +  yield extension.unload();
> +});
> +

We should test that the processes are actually shut down after disconnect() or extension shutdown.

Comment 10

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review50116

> I'd prefer if we just put a single `/* globals ... */` comment near the top of the file.

Why?  I find it useful to have it here to make it clear where that global is actually coming from.

> Please `return Promise.reject({message: ...})` instead.

I would like to do that, but this function isn't asynchronous (which creates hassles in other places too)

> Any particular reason you don't use `do_register_cleanup` for this?

mostly because this isn't an xpcshell test but I'll use registerCleanupFunction()

> Hm. This is problematic.
> 
> We need to use the PYTHON executable defined by the build config, and I can't think of a way to get it here other than creating a preprocessed file.
> 
> We also need to pass `-U`, especially on Windows. Which means we probably need to wrap this in a batch file there.
> 
> We should probably use `String.raw` for all of these, too.

We're not running this test on Windows yet (since there are a bunch of unimplemented pieces like searching the registry for host manifests, etc).  I'd like to address all the Windows-related things as part of bug 1270359.

Comment 11

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review50116

> Why?  I find it useful to have it here to make it clear where that global is actually coming from.

Mainly because that's the prevailing style at this point, but partly because this is a hack that I'd like to rely on as little as possible. If you want to make it easier to tell where the global came from, I'd rather use `defineLazyModuleGetter`, or something along the lines of:

    function require(url) {
       let obj = {};
       Cu.import(url, obj);
       return obj;
    }

    const {EventEmitter} = require("resource://devtools/shared/event-emitter.js");

> I would like to do that, but this function isn't asynchronous (which creates hassles in other places too)

Hm... quite

> We're not running this test on Windows yet (since there are a bunch of unimplemented pieces like searching the registry for host manifests, etc).  I'd like to address all the Windows-related things as part of bug 1270359.

OK, but the other issues still apply, and either way it would be nice to start off running with `-U` so things are consistent from the outset.

Comment 12

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review50116

> OK, but the other issues still apply, and either way it would be nice to start off running with `-U` so things are consistent from the outset.

Yeah, I'll address the other issues.  You mean -u instead of -U?  the default python install on my mac doesn't have a documented -U option and it raises an exception if i supply it.

Comment 13

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review49435

> It is not an issue, but it seems a bit strange that HostManifestManager.lookupApplication doesn't reject when no native application is found.

I think this is a pretty common convention for "lookup" function, returning null if the thing being looked up cannot be found, and let the caller turn that into an exception if appropriate...

Comment 14

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review50116

> Yeah, I'll address the other issues.  You mean -u instead of -U?  the default python install on my mac doesn't have a documented -U option and it raises an exception if i supply it.

Sorry, yeah, `-u`

Comment 15

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1270357 Expose PYTHON from build config to test r?kmag

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

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

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

Comment 17

[Andrew Swan [:aswan]]

The latest revision of the bigger patch addresses the low hanging fruit from Kris's feedback, still working on some of the other items so no need to review it yet.

Comment 18

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review50116

> We should use the JSON object belonging to the scope. There are side-effects to using ours.
> 
> It might actually be worth writing a utility function in Cu to handle this properly...

Can you give me some pointers here?  What are the side-effects?  And I'm not immediately seeing how this would fit into Cu.

> We should try closing stdin and allowing the process to exit gracefully first, and then kill after a timeout.

Okay, any suggestions for the timeout?  3 seconds?

> Does Chrome ever set `lastError` when it calls this handler?

I'm not sure but are you saying we shouldn't set it if they don't?

> We should probably just use `cloneInto` with `cloneFunctions` for this.

Sure, but for my own background can you explain the difference?

Comment 19

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review50116

> Can you give me some pointers here?  What are the side-effects?  And I'm not immediately seeing how this would fit into Cu.

When we pass an object from an unprivileged scope to our JSON.stringify function, what we wind up stringifying is a security wrapper. For simple objects, the results are the same. For things like wrapped natives, and objects with `toJSON` methods, the results can be significantly different, and likely unexpected.

> Okay, any suggestions for the timeout?  3 seconds?

That sounds fine to me, but it will require an AsyncShutdown blocker: http://gecko.readthedocs.io/en/latest/toolkit/modules/toolkit_modules/AsyncShutdown.html

> I'm not sure but are you saying we shouldn't set it if they don't?

I'm not a fan of `lastError` in general. In the case of callbacks, where people aren't using promises, we're stuck with it. If we need to pass error information to event listeners, we should pass it as an argument.

> Sure, but for my own background can you explain the difference?

The only real difference is that it's more efficient for simple cases like this.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

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

Comment 21

[Andrew Swan [:aswan]]

Attachment: Bug 1270357 Add isDeeply to mochitest eslintrc

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

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

https://reviewboard.mozilla.org/r/54136/#review50836

This needs to be reviewed by a build peer, but in any case, I think the better option would be to add `TESTING_PP_JS_MODULES` and/or `TEST_HARNESS_PP_FILES` config variables rather than using `OBJDIR_PP_FILES` here.

::: toolkit/components/extensions/test/mochitest/test_constants.js:5
(Diff revision 1)
> +#filter substitution
> +
> +"use strict";
> +
> +/* global PYTHON */

This isn't necessary.

Comment 23

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review50116

> When we pass an object from an unprivileged scope to our JSON.stringify function, what we wind up stringifying is a security wrapper. For simple objects, the results are the same. For things like wrapped natives, and objects with `toJSON` methods, the results can be significantly different, and likely unexpected.

Much blood, sweat, and tears has already been spilled over this.  Taking it over to bug 1274708.

> I'm not a fan of `lastError` in general. In the case of callbacks, where people aren't using promises, we're stuck with it. If we need to pass error information to event listeners, we should pass it as an argument.

Verified that Chrome does set lastError at least in the specific case where the native application exits unexpectedly.
I think that we should keep the lastError behavior.  We can either leave it at that or also add another argument to the event listener (it already gets the Port object as an argument, which I'll add here) and document that as the preferred way to get the error with the intention of removing the lastError support some day.

Comment 24

[Andrew Swan [:aswan]]

Attachment: Bug 1270357 Add a test hook to Subprocess.jsm

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

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54136/diff/1-2/

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

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

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

https://reviewboard.mozilla.org/r/54136/#review51080

I'm not sure what happened to this commit. It seems to be a mix of two things.

The JSON part looks OK, but the moz.build changes still need to be reviewed by a buildconfig peer. Maybe ping glandium.

::: toolkit/components/extensions/ExtensionUtils.jsm:151
(Diff revision 2)
>      this.checkedLastError = false;
>      this._lastError = null;
>      this.contextId = ++gContextId;
>      this.unloaded = false;
>      this.extensionId = extensionId;
> +    this.sandbox = null;

I think this is meant to be `jsonSandbox`

::: toolkit/components/extensions/ExtensionUtils.jsm:358
(Diff revision 2)
> +  // method get handled properly.
> +  jsonStringify(data) {
> +    if (this.jsonSandbox == null) {
> +      this.jsonSandbox = new Cu.Sandbox(this.cloneScope, {sameZoneAs: this.cloneScope});
> +//      this.jsonSandbox = new Cu.Sandbox(this.principal, {sameZoneAs: this.cloneScope});
> +//      this.jsonSandbox = new Cu.Sandbox(this.principal, {wantXrays: false});

Please remove comments.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

https://reviewboard.mozilla.org/r/54354/#review51082

Let's revisit this after Subprocess.jsm lands. There are already some conflicts with my local changes, and I introduced some functionality for testing internal state that might make this easier.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

https://reviewboard.mozilla.org/r/52347/#review51084

This mostly looks good, but it's going to need some changes after the final version of Subprocess.jsm lands, and it may require changes to handle the PYTHON build config substitution, so I'm holding off giving it r+ for now.

::: toolkit/components/extensions/NativeMessaging.jsm:225
(Diff revision 6)
> +
> +    let buffer = this.sendQueue.shift();
> +    let uintArray = Uint32Array.of(buffer.byteLength);
> +
> +    this.writePromise = this.proc.stdin.write(uintArray.buffer)
> +      .then(() => this.proc.stdin.write(buffer))

We shouldn't wait for the first write promise to resolve before calling `.write()` a second time. Writes always happen in order, so this just adds unnecessary latency. Something like:

    Promise.all([
      stdin.write(uintArray.buffer),
      stdin.write(buffer),
    ]).then(...

should do.

::: toolkit/components/extensions/NativeMessaging.jsm:271
(Diff revision 6)
> +      // Set a timer to kill the process gracefully after one timeout
> +      // interval and kill it forcefully after two intervals.
> +      let timer = setTimeout(() => {
> +        this.proc.kill();
> +        timer = setTimeout(() => {
> +          this.proc.kill(true);

`.kill()` accepts a timeout rather than a boolean in the current version of the patch, so this isn't necessary.

::: toolkit/components/extensions/NativeMessaging.jsm:280
(Diff revision 6)
> +      let promise = this.proc.stdin.close()
> +        .catch(err => {
> +          if (err.errorCode != Subprocess.ERROR_END_OF_FILE) {
> +            throw err;
> +          }
> +        }).then(() => this.proc.wait())

There's no need to wait for the `.close()` promise to resolve before calling `.wait()`

::: toolkit/components/extensions/NativeMessaging.jsm:303
(Diff revision 6)
> +      doCleanup();
> +    } else if (this.startupPromise) {
> +      this.startupPromise.then(doCleanup);
> +    }
> +
> +    this.emit("disconnect", err);

We shouldn't emit "disconnect" if neither of the above conditionals are true.

::: toolkit/components/extensions/NativeMessaging.jsm:316
(Diff revision 6)
> +  portAPI() {
> +    let api = {
> +      name: this.name,
> +
> +      disconnect: () => {
> +        this._cleanup();

This should probably throw if the port is already disconnected.

::: toolkit/components/extensions/NativeMessaging.jsm:327
(Diff revision 6)
> +
> +      onDisconnect: new ExtensionUtils.SingletonEventManager(this.context, "native.onDisconnect", fire => {
> +        let listener = (what, err) => {
> +          let handler = () => this.context.runSafe(fire);
> +          if (err) {
> +            this.context.withLastError({message: err.message}, handler);

I still don't really like this... especially since Chrome does not document the behavior. Are there any extensions that actually rely on this?

I think I'd rather we just add a status member to the Port object.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:17
(Diff revision 6)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +/* globals OS, PYTHON, isDeeply */

Can we just add `isDeeply` to the mochitest eslintrc instead?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:121
(Diff revision 6)
> +  type: "stdio",
> +  allowed_extensions: [ID],
> +};
> +
> +add_task(function* setup_scripts() {
> +  const PERMS = {unixMode: parseInt("777", 8)};

`{unixMode: 0o777}`

Although, if it comes to it, it should probably be `0o755` at most.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:122
(Diff revision 6)
> +  allowed_extensions: [ID],
> +};
> +
> +add_task(function* setup_scripts() {
> +  const PERMS = {unixMode: parseInt("777", 8)};
> +  const ECHO_SCRIPT = String.raw`#!${PYTHON} -u

The PYTHON config variable isn't usually a full path, so we should probably do a path search for it.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:164
(Diff revision 6)
> +import signal
> +import time
> +
> +signal.signal(signal.SIGTERM, signal.SIG_IGN)
> +while True:
> +    time.sleep(5)

`signal.pause()`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:184
(Diff revision 6)
> +    });
> +    browser.test.onMessage.addListener((what, payload) => {
> +      if (what == "send") {
> +        if (payload._json) {
> +          let json = payload._json;
> +          payload.toJSON = () => json;

Does this work with the current implementation?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_native_messaging.html:446
(Diff revision 6)
> +  yield waitForSubprocessStart();
> +  is(Subprocess._liveProcCount, 1, "subprocess is still running");
> +  let exitPromise = waitForSubprocessExit();
> +  yield extension.unload();
> +  yield exitPromise;
> +});

We should probably add an xpcshell test with a mock AsyncShutdown cycle to make sure an app actually gets killed if it doesn't exit in time.

Comment 30

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/54354/#review51082

Can we revisit this now in light of your recent changes or do you want to wait for it to actually land?
I'm not seeing the new testing functionality, it looks like you just import SubprocessImpl and reach into the worker to do the equivalent testing there.

Comment 31

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #30)
> Can we revisit this now in light of your recent changes or do you want to
> wait for it to actually land?

It will land sometime today, so I guess this question is moot.

> I'm not seeing the new testing functionality, it looks like you just import
> SubprocessImpl and reach into the worker to do the equivalent testing there.

Yes, the changes I made allow us to add private methods to the worker side to query the current state, without changing any public interfaces.

Comment 32

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #31)
> > I'm not seeing the new testing functionality, it looks like you just import
> > SubprocessImpl and reach into the worker to do the equivalent testing there.
> 
> Yes, the changes I made allow us to add private methods to the worker side
> to query the current state, without changing any public interfaces.

Okay, so just to be clear, I'd like to be able to get a callback or promise resolution when there's a change (eg in the number of running processes).  There's not currently a way to do this, you prefer that I rewrite this patch to follow the pattern of putting that code in the worker and reaching down to it from the test?

Comment 33

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review51084

> We shouldn't emit "disconnect" if neither of the above conditionals are true.

They will both be false if an exception occurs while launching the subprocess.  Since `connectNative()` is synchronous, it immediately returns a Port object, but that Port object isn't connected to a live native process, so I think it does make sense to signal disconnect in that case.  If I was designing this from scratch I would make `connectNative()` asynchronous but for now I think preserving compatibility makes sense.

> I still don't really like this... especially since Chrome does not document the behavior. Are there any extensions that actually rely on this?
> 
> I think I'd rather we just add a status member to the Port object.

This seems like a separate and larger issue, if we're going to do this, shouldn't we do this across the board with every webextensions API that references lastError?  If we do this, that already creates a Chrome incompatibility which is a minor nuisance for extension authors, to have that incompatibility in some APIs but not others compounds that nuisance.  I would fully support a plan for doing it everywhere, but I think that it would make sense to retain compatibility here for now and then do the lastError work as part of a separate issue.

> The PYTHON config variable isn't usually a full path, so we should probably do a path search for it.

It is a full path in my local build on MacOS and I just dug up a Windows 8 try build and looked through the logs and it is a full path there.
On what platform do you think it is not a full path?

Comment 34

[Andrew Swan [:aswan]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54136/diff/2-3/

Comment 35

[Andrew Swan [:aswan]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54354/diff/1-2/

Comment 36

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/6-7/

Comment 37

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review51084

> They will both be false if an exception occurs while launching the subprocess.  Since `connectNative()` is synchronous, it immediately returns a Port object, but that Port object isn't connected to a live native process, so I think it does make sense to signal disconnect in that case.  If I was designing this from scratch I would make `connectNative()` asynchronous but for now I think preserving compatibility makes sense.

It seems like there are cases where we may wind up sending `onDisconnect` twice, so I'd rather be as safe as possible.

> This seems like a separate and larger issue, if we're going to do this, shouldn't we do this across the board with every webextensions API that references lastError?  If we do this, that already creates a Chrome incompatibility which is a minor nuisance for extension authors, to have that incompatibility in some APIs but not others compounds that nuisance.  I would fully support a plan for doing it everywhere, but I think that it would make sense to retain compatibility here for now and then do the lastError work as part of a separate issue.

I don't think so. This behavior isn't documented, for event listeners in general, or for this particular listener. We support `lastError` in API callbacks, since it is documented and extensions definitely rely on it, but prefer the Promise-based version of the API. The same scenario doesn't hold here.

> It is a full path in my local build on MacOS and I just dug up a Windows 8 try build and looked through the logs and it is a full path there.
> On what platform do you think it is not a full path?

At least for me locally, it's not. I had to change the subprocess tests to do an explicit path search after I stopped doing it automatically for the `command` option.

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

https://reviewboard.mozilla.org/r/54354/#review53286

Can we just use the `getProcesses` method from `subprocess_worker_common` to get the process counts, and then wait on them if necessary?

Comment 39

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/54354/#review53286

I'm not sure I follow, what would we be waiting on?  I think tapping process create and then chaining a handler to the new process's exitPromise is preferrable to adding a new type of message from the worker or any other technique I could imagine to detect when processes come and go.

Comment 40

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/54354/#review53286

I mainly just don't want any more test code in the normal code flow than we can avoid. We already have information about the current active processes in the worker thread, along with a method to access it. If we need to wait for all processes to exit, it would be easy enough to add another method containing something like `Promises.all(Array.from(io.processes.values(), proc => proc.exitPromise))`.

If we really do need events in the parent process, I'd rather just send observer notifications from the `BaseProcess` constructor, when it's called, and when its exit promise resolves, rather than adding tracking code to each implementation.

Comment 41

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review51084

> It seems like there are cases where we may wind up sending `onDisconnect` twice, so I'd rather be as safe as possible.

Right, in order to preserve the onDisconnect for startup errors, I just added a flag to indicate if we've already triggered the event so we only do it once.
I don't love it, but with `_cleanup` attached as an error handler for the read and write promises I think its the simplest solution.

> I don't think so. This behavior isn't documented, for event listeners in general, or for this particular listener. We support `lastError` in API callbacks, since it is documented and extensions definitely rely on it, but prefer the Promise-based version of the API. The same scenario doesn't hold here.

Going back to your original question about whether extensions rely on this, the very first non-obfuscated one I looked at that has this permission is https://chrome.google.com/webstore/detail/uniface-anywhere/camcjdhmakkedecangafdibaekmfjici and although it appears to have other issues, it certainly has code that references lastError from the onDisconnected handler.
I really think that doing this would be taking an empty stand at the cost of unneded compatibility hassles for extension authors.

Comment 42

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review51084

> Going back to your original question about whether extensions rely on this, the very first non-obfuscated one I looked at that has this permission is https://chrome.google.com/webstore/detail/uniface-anywhere/camcjdhmakkedecangafdibaekmfjici and although it appears to have other issues, it certainly has code that references lastError from the onDisconnected handler.
> I really think that doing this would be taking an empty stand at the cost of unneded compatibility hassles for extension authors.

I don't think that our behavior is even compatible with the behavior that that add-on relies on. It seems to rely on `lastError` always being set when `onDisconnected` is called, and it also attempts to parse the error message, which Chrome explicitly documents as unsupported.

I'm still not convinced that we have a good reason to implement this kind of undocumented behavior.

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

https://reviewboard.mozilla.org/r/54136/#review53384

::: toolkit/components/extensions/test/mochitest/test_constants.js:5
(Diff revision 3)
> +#filter substitution
> +
> +"use strict";
> +
> +this.PYTHON = String.raw`@PYTHON@`;

This is not useful. The python path will be different between the environment that preprocesses the file and the environmnt that runs the test.

Comment 44

[Andrew Swan [:aswan]]

(In reply to Mike Hommey [:glandium] from comment #43)
> This is not useful. The python path will be different between the
> environment that preprocesses the file and the environmnt that runs the test.

okay can you suggest a reliable way to get a valid path to a python interpreter from a mochitest then?

Comment 45

[Andrew Swan [:aswan]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

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

Comment 46

[Andrew Swan [:aswan]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54354/diff/2-3/

Comment 47

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/7-8/

Comment 48

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review51084

> I don't think that our behavior is even compatible with the behavior that that add-on relies on. It seems to rely on `lastError` always being set when `onDisconnected` is called, and it also attempts to parse the error message, which Chrome explicitly documents as unsupported.
> 
> I'm still not convinced that we have a good reason to implement this kind of undocumented behavior.

I wasn't trying to say that that particular add-on would break without this, but rather that extension authors are well aware of this particular behavior and doing something different would just be unnecessary inconvenience for them.  On the flip side, I don't see what we gain by not supporting it.  If you feel really strongly, how about making the error be a parameter to the onDisconnected handler and I'll refer any frustrated extension authors to you :-)

> We should probably add an xpcshell test with a mock AsyncShutdown cycle to make sure an app actually gets killed if it doesn't exit in time.

Sorry I didn't follow what you're suggesting, but `test_unresponsive_native_app()` is meant to cover the case I think you're describing?

Comment 49

[Mike Hommey [:glandium]]

(In reply to Andrew Swan [:aswan] from comment #44)
> (In reply to Mike Hommey [:glandium] from comment #43)
> > This is not useful. The python path will be different between the
> > environment that preprocesses the file and the environmnt that runs the test.
> 
> okay can you suggest a reliable way to get a valid path to a python
> interpreter from a mochitest then?

Find a python executable you like in $PATH?

Comment 50

[Kris Maglione [:kmag]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

https://reviewboard.mozilla.org/r/54136/#review53282

Comment 51

[Kris Maglione [:kmag]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

https://reviewboard.mozilla.org/r/54354/#review54606

Comment 52

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review51084

> I wasn't trying to say that that particular add-on would break without this, but rather that extension authors are well aware of this particular behavior and doing something different would just be unnecessary inconvenience for them.  On the flip side, I don't see what we gain by not supporting it.  If you feel really strongly, how about making the error be a parameter to the onDisconnected handler and I'll refer any frustrated extension authors to you :-)

I don't see what we gain by doing something roughly similar but ultimately incompatible either.

I really don't like the idea of setting `lastError` in event handlers. It's not used for any existing event handlers. The fact that it's set for this event in Chrome is not documented, and the results don't seem to be especially deterministic. The one add-on that you point to that uses it was using it in a way that isn't valid, even in Chrome. If we get a clamor from extension authors saying that we should implement it, then it may be worth thinking about. Otherwise, I don't think we should.

> Sorry I didn't follow what you're suggesting, but `test_unresponsive_native_app()` is meant to cover the case I think you're describing?

Not really. I want to test that we wait for the app to fully exit, or possibly kill it, before we complete the shutdown process. This means hooking into the async shutdown code to make sure that our blockers are working correctly, and that we don't quit the browser before we've force-killed the helper.

That's the kind of thing that we can't really test properly from a mochitest, so it would be nice to have an xpcshell that triggers our shutdown barriers.

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

https://reviewboard.mozilla.org/r/52347/#review53284

::: toolkit/components/extensions/NativeMessaging.jsm:196
(Diff revisions 6 - 7)
>  
> +  // A port is definitely "alive" if this.proc is non-null.  But we have
> +  // to provide a live port object immediately when connecting so we also
> +  // need to consider a port alive if proc is null but the startupPromise
> +  // is still pending.
> +  _isDisconnected() {

This should probably just be a getter.

Comment 54

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/52347/#review51084

> Not really. I want to test that we wait for the app to fully exit, or possibly kill it, before we complete the shutdown process. This means hooking into the async shutdown code to make sure that our blockers are working correctly, and that we don't quit the browser before we've force-killed the helper.
> 
> That's the kind of thing that we can't really test properly from a mochitest, so it would be nice to have an xpcshell that triggers our shutdown barriers.

To be clear, this would be an xpcshell test that just imports NativeMessaging.jsm and works directly with that object rather than trying to actually load a complete extension right?

Comment 55

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/8-9/

Comment 56

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/52347/#review51084

> To be clear, this would be an xpcshell test that just imports NativeMessaging.jsm and works directly with that object rather than trying to actually load a complete extension right?

Yeah, I think that makes the most sense.

Comment 57

[Andrew Swan [:aswan]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

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

Comment 58

[Andrew Swan [:aswan]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

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

Comment 59

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/9-10/

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f9b54febc8
Add isDeeply to mochitest eslintrc r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1979d928c7c
Add a test hook to Subprocess.jsm r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e602689376d
Implement runtime.connectNative() r=kmag

Comment 61

[Andrew Swan [:aswan]]

Comment on attachment 8754654 [details]
Bug 1270357 Add isDeeply to mochitest eslintrc

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

Comment 62

[Andrew Swan [:aswan]]

Comment on attachment 8755040 [details]
Bug 1270357 Add a test hook to Subprocess.jsm

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

Comment 63

[Andrew Swan [:aswan]]

Comment on attachment 8751980 [details]
Bug 1270357 Implement runtime.connectNative()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52347/diff/10-11/

Comment 64

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8987c204d2920c40e6bd6bd79d9626591df58a33 for https://treeherder.mozilla.org/logviewer.html#?job_id=29839082&repo=mozilla-inbound

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9443d05727b9
Add isDeeply to mochitest eslintrc r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c9638056d9f
Add a test hook to Subprocess.jsm r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f82506b965
Implement runtime.connectNative() r=kmag

Comment 66

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9443d05727b9
https://hg.mozilla.org/mozilla-central/rev/4c9638056d9f
https://hg.mozilla.org/mozilla-central/rev/a1f82506b965