1269501 - Implement async IPC API

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Kris Maglione [:kmag]]

We need to be able to launch and communicate with external processes in order to implement the connectNative extension API.

The closest thing that we currently have to an API that would allow this is the add-on SDK's child_process/subprocess.js module. That module doesn't meet our needs, though. Its communication API is not flexible enough, it has a lot of extreme built-in inefficiencies, and is generally just not well-engineered enough to be trustworthy.

My plan is instead to implement a Subprocess.jsm module with a similar API, but more similar in feel and implementation to OS.File.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

This still has some rough edges, but it's mostly complete at this point.

Comment 3

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50235/#review47073

Whew, got through the Unix bits but not the test.
I don't think I'll be very useful on the windows bits but will come back to the test soon.  But I want to publish this now before I manage to lose it.

Just one high level comment that this is the first ctypes code I've seen and it makes me think that writing a native xpcom component would have to be easier.  Is dealing with threading in a portable way the argument against implementing in C++?  (this question is more for my own comprehension, I'm not suggesting we change this after all the effort you've put in to get this working)

::: toolkit/modules/subprocess/subprocess_common.jsm:149
(Diff revision 1)
> +
> +/**
> + * Represents an input or output pipe connected to a subprocess.
> + */
> +class Pipe {
> +  constructor(process, fd, id) {

it looks like id is actually the fd and fd is unused?

::: toolkit/modules/subprocess/subprocess_common.jsm:203
(Diff revision 1)
> +
> +    if (Cu.getClassName(buffer, true) !== "ArrayBuffer") {
> +      if (buffer.byteLength === buffer.buffer.byteLength) {
> +        buffer = buffer.buffer;
> +      } else {
> +        buffer = buffer.buffer.slice(buffer.byteOffset, buffer.byteLength);

shouldn't the second arg be `byteOffset + byteLength` ?

::: toolkit/modules/subprocess/subprocess_common.jsm:222
(Diff revision 1)
> +    this.buffers = [];
> +    this.dataAvailable = 0;
> +
> +    this.decoder = new TextDecoder();
> +
> +    this.pendingReads = [];

On first reading, I found the similarity between this and `_pendingRead` confusing.  I don't have a great idea for a better name that isn't overly long though.  Perhaps leave this one as-is and rename `_pendingRead` to something like `_pipeReadPromise`?  bah, that's not great...

::: toolkit/modules/subprocess/subprocess_common.jsm:294
(Diff revision 1)
> +   * filled from the current input buffer.
> +   *
> +   * @private
> +   */
> +  checkPendingReads() {
> +    this.fillBuffer();

This seems strangely placed here.  For one thing, this is called from `onInput()` in which case we may be about to resolve `pendingReads[0]` (and possibly more).  This will do a `_read` based on the size of `pendingReads[0]` but that read really ought to be based on the first unsatisfied read.

Moving this call to the end of this function would address that problem.  But I think it would be more clear (though more total lines of code) for trigger a read to not be a side effect of `checkPendingReads()` but something that callers should do themselves at the proper time.

::: toolkit/modules/subprocess/subprocess_common.jsm:377
(Diff revision 1)
> +  /**
> +   * Reads exactly `length` bytes of UTF-8 data from the input stream, and
> +   * converts them to a JavaScript string.
> +   *
> +   * @param {integer} length
> +   *        The number of bytes to read.

options isn't documented

::: toolkit/modules/subprocess/subprocess_common.jsm:406
(Diff revision 1)
> + *
> + * @property {integer} pid
> + *           The process ID of the process, assigned by the operating system.
> + */
> +class BaseProcess {
> +  constructor(worker, processId, fds, pid) {

what's the difference between processId and pid?  pid seems to be unused, but actually the right name for the thing you call id.

::: toolkit/modules/subprocess/subprocess_common.jsm:421
(Diff revision 1)
> +    this.worker.call("wait", [this.id]).then(({exitCode}) => {
> +      this.resolveExit(Object.freeze({exitCode}));
> +      this.exitCode = exitCode;
> +    });
> +
> +    if (fds[0] !== undefined) {

those kids these days don't know their unix file descriptor numbers, perhaps this would be clearer as an object with properties named `stdin`, `stdout`, `stderr` instead.

::: toolkit/modules/subprocess/subprocess_shared.js:9
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +if (!ArrayBuffer.transfer) {
> +  ArrayBuffer.transfer = function(buffer, size = undefined) {

some documentation here would be good.

also why not just make the default for size be buffer.byteLength instead of the test below?

::: toolkit/modules/subprocess/subprocess_shared.js:9
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +if (!ArrayBuffer.transfer) {
> +  ArrayBuffer.transfer = function(buffer, size = undefined) {

documentation would be good.
also, why not just make the default for size be buffer.byteLength instead of undefined plus the check below?

::: toolkit/modules/subprocess/subprocess_shared_unix.js:12
(Diff revision 1)
> +
> +/* exported libc */
> +
> +const LIBC = OS.Constants.libc;
> +
> +const LIBC_CHOICES = ["libc.so", "libSystem.B.dylib", "a.out"];

Why is a.out in this list?
In any case, seems like we should only try libc.so on linux and only try libSystem.... on macos.

::: toolkit/modules/subprocess/subprocess_unix.jsm:39
(Diff revision 1)
> +  try {
> +    let info = yield OS.File.stat(path);
> +
> +    // FIXME: We really want access(path, X_OK) here, but OS.File does not
> +    // support it.
> +    return !info.isDir && (info.unixMode & 0o111);

This just checks that the file has an x bit somewhere, not that the current user actually has permission.  I guess that's basically what your comment above is about and the cases where this could fail are definitely uncommon.

::: toolkit/modules/subprocess/subprocess_worker_common.js:12
(Diff revision 1)
> +
> +/* exported BasePipe, BaseProcess, debug */
> +/* globals Process, io */
> +
> +function debug(message) {
> +  self.postMessage({msg: "debug", message});

self?

::: toolkit/modules/subprocess/subprocess_worker_common.js:40
(Diff revision 1)
> +  }
> +}
> +
> +let lastProcessId = 0;
> +
> +class BaseProcess {

Using the same name for this class and the one in subprocess_common is confusing me...

::: toolkit/modules/subprocess/subprocess_worker_unix.js:250
(Diff revision 1)
> +
> +  /**
> +   * Initializes the IO pipes for use as standard input, output, and error
> +   * descriptors in the spawned process.
> +   *
> +   * @returns {unix.Fd[]}

document the parameter / stderr

::: toolkit/modules/subprocess/subprocess_worker_unix.js:315
(Diff revision 1)
> +    try {
> +      if (options.workdir) {
> +        cwd = ctypes.char.array(LIBC.PATH_MAX)();
> +        libc.getcwd(cwd, cwd.length);
> +
> +        if (libc.chdir(options.workdir) < 0) {

Ugh, this seems fraught with problems but `spawn()` doesn't seem to offer any other way...

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review47073

I mainly went the ctypes route for the sake of similarity with the OS.File module.

That said, I don't think it would have been easier to implement in C++. It's much easier to write ctypes bindings for a C API than it is to write JS bindings in C++, and having the bulk of the code written in JS gives us a lot more flexibility in how we change it in the future.

> it looks like id is actually the fd and fd is unused?

The `id` is an internal ID that doesn't correspond to a real file descriptor. The `fd` is the file descriptor number in the child process. It's not used internally, but it's meant for consumers (and is also useful for debugging).

> shouldn't the second arg be `byteOffset + byteLength` ?

Yup.

> On first reading, I found the similarity between this and `_pendingRead` confusing.  I don't have a great idea for a better name that isn't overly long though.  Perhaps leave this one as-is and rename `_pendingRead` to something like `_pipeReadPromise`?  bah, that's not great...

Yeah, I'll try to think of a better name. It confused me a few times too.

> This seems strangely placed here.  For one thing, this is called from `onInput()` in which case we may be about to resolve `pendingReads[0]` (and possibly more).  This will do a `_read` based on the size of `pendingReads[0]` but that read really ought to be based on the first unsatisfied read.
> 
> Moving this call to the end of this function would address that problem.  But I think it would be more clear (though more total lines of code) for trigger a read to not be a side effect of `checkPendingReads()` but something that callers should do themselves at the proper time.

At this point, if we're coming from an onInput call, we've already added the buffer and updated our total buffer size, but we don't know if we have enough data to satisfy our most recent pending read, and we don't have any pending read operations.

If we don't have enough data to satisfy the last read, we're going to need to dispatch a new read operation, and the sooner we do it, the better. If we do, then this is a no-op unless we're below our baseline buffer size.

In either case, if we're at a point where we want to check if we can satisfy pending reads, we're also at a point where we want to check if we need to fill the buffer.

> what's the difference between processId and pid?  pid seems to be unused, but actually the right name for the thing you call id.

`processId` is an internal ID of the Process object. `pid` is the OS process ID. It's unused, but it's meant for use by consumers.

> some documentation here would be good.
> 
> also why not just make the default for size be buffer.byteLength instead of the test below?

This is an experimental web API that we don't currently implement, so it's externally documented:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/transfer

I don't remember why I didn't use byteLength as the default value. I'm pretty sure there was a reason, but it probably became unnecessary after I changed the code.

> Why is a.out in this list?
> In any case, seems like we should only try libc.so on linux and only try libSystem.... on macos.

Because "a.out" magically resolves to libc on Linux and other Unixes.

I copied this list and behavior from OS.File, so I'd rather not change it.

> This just checks that the file has an x bit somewhere, not that the current user actually has permission.  I guess that's basically what your comment above is about and the cases where this could fail are definitely uncommon.

Yeah. It's not ideal, but I think it's going to be an uncommon enough problem to not be worth fixing at this point.

> self?

`self` is a reference to the global scope in workers. It's not strictly necessary, but I don't really like bare `postMessage` calls.

> Using the same name for this class and the one in subprocess_common is confusing me...

All of the Pipe and Process classes have worker-side and browser-side variants with the same name. I suppose I could add `Child` to all of them, or something, but I don't really like the idea...

> Ugh, this seems fraught with problems but `spawn()` doesn't seem to offer any other way...

Yeah, there really isn't another option. I'd usually only chdir after I fork, but I don't want to fork/exec directly from JS if I can avoid it.

The original subprocess.js module actually just `chdir`s and never changes back, so... *shrug*

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

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

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andrew Swan [:aswan] from comment #3)
> Just one high level comment that this is the first ctypes code I've seen and
> it makes me think that writing a native xpcom component would have to be
> easier.  Is dealing with threading in a portable way the argument against
> implementing in C++?  (this question is more for my own comprehension, I'm
> not suggesting we change this after all the effort you've put in to get this
> working)

My two cents: my experience writing OS.File with js-ctypes was not too positive. The first problem was due to the fact that JS is not as good at writing C than, well, C/C++: way too many difficulties popped from everywhere (constants that have different values on different variants of Unix, libc functions that are not implemented on Android, having to emulate C macros in JS, etc.). The second problem was performance – js, workers and js-ctypes are pretty memory hungry, in addition to which ChromeWorker are extremely slow to start during Firefox startup (I'm talking about telemetry reports indicating 5s or more). Oh, and passing errors between threads in JS is really painful.

I haven't looked at this source code, but if I had to rewrite OS.File, I'd probably do it in C++ (or better, Rust).

Comment 8

[Kris Maglione [:kmag]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #7)
> My two cents: my experience writing OS.File with js-ctypes was not too
> positive. The first problem was due to the fact that JS is not as good at
> writing C than, well, C/C++: way too many difficulties popped from
> everywhere (constants that have different values on different variants of
> Unix, libc functions that are not implemented on Android, having to emulate
> C macros in JS, etc.). The second problem was performance – js, workers and
> js-ctypes are pretty memory hungry, in addition to which ChromeWorker are
> extremely slow to start during Firefox startup (I'm talking about telemetry
> reports indicating 5s or more). Oh, and passing errors between threads in JS
> is really painful.

I'm not sure all of the same concerns apply here. I was initially worried
about the values of constants, and the sizes of types, but OSFileConstants
already defines most of the ones I need, so I think you've pretty much taken
care of that problem for me.

I'm not especially worried about worker startup time during Firefox startup,
since I don't expect this API to be used that early. Or, at least, certainly
not as critically as OS.File is.

If memory becomes an issue, I'd be willing to rewrite some or all of this in
C++. It would be easy enough to get rid of the worker by just offloading the
IO polling to a thread in native code.

> I haven't looked at this source code, but if I had to rewrite OS.File, I'd
> probably do it in C++ (or better, Rust).

I'd love to implement this in Rust, but as I understand it, we're pretty far
from the point where we can write JS bindings in Rust, and I don't really want
to have to write three levels of language bindings for something this simple.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

Matt,

bsmedberg suggested you to review the Windows portions of this. Would you mind looking it over? I'll probably submit it for final review by the end of the week if there are no major issues.

I'll note that this currently polls on a pretty short interval in the worker thread, rather than blocking. I'm planning to change that in a follow-up bug, but it requires adding some fiddly machinery to signal the worker thread to yield every time the it sends a message, so I decided to keep to the simpler method for the first iteration.

Thanks!

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

https://reviewboard.mozilla.org/r/50235/#review47357

Looking good.  One thing that I don't think is likely to get test coverage right now is some of the different cases in checkPendingReads().  Its hard to get full coverage deterministically but I think you could generally get good coverage by having the python script send part of a message then flush stdout then send the rest of the message, and also by having it write multiple messages with a single write to stdout, etc.

::: toolkit/modules/subprocess/subprocess_common.jsm:129
(Diff revision 3)
> +   * @param {Array} [transferList]
> +   *        A list of objects to transfer to the worker, rather than cloning.
> +   * @returns {Promise}
> +   */
> +  call(method, args, transferList = []) {
> +    let msgId = lastResponseId++;

nitpick: i think the way you're using this, it's really the "next" response id, not the last one.

::: toolkit/modules/subprocess/subprocess_common.jsm:378
(Diff revision 3)
> +   *
> +   * @param {integer} length
> +   *        The number of bytes to read.
> +   * @returns {Promise<object>}
> +   */
> +  readJSON(length) {

So the promise that this returns can be rejected either because the underlying pipe has closed or because of a decoding error with the string or the json.  Is the idea that `.closed` is part of the public interface and we just consult that upon a rejected promise to distinguish those cases?

::: toolkit/modules/subprocess/subprocess_common.jsm:438
(Diff revision 3)
> +    this.id = processId;
> +    this.worker = worker;
> +    this.pid = pid;
> +
> +    this.exitPromise = new Promise(resolve => {
> +      this.resolveExit = resolve;

nit: i don't think you need to actually keep resolve in a property on this, you can just put the call to wait inside this block.

::: toolkit/modules/subprocess/subprocess_common.jsm:475
(Diff revision 3)
> +
> +  static get WORKER_URL() {
> +    throw new Error("Not implemented");
> +  }
> +
> +  static get worker() {

I realize this is static but the overlap with the worker property on instances of this class threw me for a moment...

::: toolkit/modules/subprocess/subprocess_worker_unix.js:252
(Diff revision 3)
> +}
> +
> +class Process extends BaseProcess {
> +  /**
> +   * A bit mask of poll() events which we currently wish to be notified of on
> +   * this file discriptor.

typo in descriptor, but i'm confused by this, this class represents a process, not a file descriptor?
ah, i see the code below where you create another pipe...
i can certainly understand wanting to stay miles away from SIGCHLD but isn't eof on the stdin pipe sufficient to detect when the process has exited?

::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js:34
(Diff revision 3)
> +  const LINE2 = "Watch how I soar.\n";
> +
> +
> +  let outputPromise = read(proc.stdout);
> +
> +  yield new Promise(resolve => setTimeout(resolve, 100));

is the point here that you want to test the read coming before the write and vice versa?  if so, can you add comments explaining that?

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

bah reviewboard

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review47357

I had to switch to unbuffered IO to prevent Python from inserting CRs on Windows, so all of the messages should actually be sent as separate IO operations at the moment.

You have a point, though. I suspect that, even though they're separate IO operations, they're only triggering one read operation. I'll see what I can do with timeouts and code coverage tools. Maybe there's a way I can instrument things to make sure we're hitting all of the important cases.

> nitpick: i think the way you're using this, it's really the "next" response id, not the last one.

Yeah, good point.

> So the promise that this returns can be rejected either because the underlying pipe has closed or because of a decoding error with the string or the json.  Is the idea that `.closed` is part of the public interface and we just consult that upon a rejected promise to distinguish those cases?

Right now, I think the closest correct solution is to check the error message. I was initially planning to add error codes to the rejection errors, so I think I'll go back and do that.

Closed is part of the public interface, though, yes. I forgot to document it.

> nit: i don't think you need to actually keep resolve in a property on this, you can just put the call to wait inside this block.

Good point. This made more sense before I refactored the `wait` method.

> I realize this is static but the overlap with the worker property on instances of this class threw me for a moment...

Hm. Yeah. They actually return the same thing in practice, but I'll rename it.

> typo in descriptor, but i'm confused by this, this class represents a process, not a file descriptor?
> ah, i see the code below where you create another pipe...
> i can certainly understand wanting to stay miles away from SIGCHLD but isn't eof on the stdin pipe sufficient to detect when the process has exited?

I'd actually have preferred to use SIGCHLD, but it's not really feasible. At the moment, NSPR disables SIGCHLD and uses its own wait loop to detect process exit, but doesn't have any way to notify when processes exit that it didn't create.

As for why we can't just wait for EOF on another pipe, we don't get notified of EOF on stdin unless we're currently polling it. And we generally want to be able to close stdin to signal to the process that it should exit, after which we can't poll it any more regardless of whether we want to.

I was originally using the poll of stdout and stderr to detect exit, but if we don't redirect stderr, and then close stdout, we're back to not being able to detect exit (which is why my test that closed stdout initially needed to enable an stderr pipe).

> is the point here that you want to test the read coming before the write and vice versa?  if so, can you add comments explaining that?

Yeah, pretty much that.

Comment 13

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50235/#review47563

::: toolkit/modules/subprocess/subprocess_worker_common.js:138
(Diff revision 3)
> +
> +  write(pipeId, buffer) {
> +    let pipe = io.getPipe(pipeId);
> +
> +    return pipe.write(buffer).then(bytesWritten => {
> +      return {data: {bytesWritten}};

is `bytesWritten` intended to be wrapped in an object?  The OutputPipe documentation says write() returns a promise that resolves to a number but with this here, it actually resolves to an object with a property called bytesWritten.  (or is the idea that this should be an object here but that code elsewhere should pull out the actual return value before resolving the original promise?)
The same issue applies for read/buffer.

Comment 14

[Molly Howell (she/her) (no longer active)]

https://reviewboard.mozilla.org/r/50235/#review47595

The Windows stuff looks all right, just a couple really minor nits.

::: toolkit/modules/subprocess/subprocess_shared_win.js:28
(Diff revision 3)
> +
> +  UINT: ctypes.unsigned_int,
> +  UCHAR: ctypes.unsigned_char,
> +
> +  BOOL: ctypes.bool,
> +  HANDLE: ctypes.size_t,

nit: I don't see how it would make any difference, but HANDLE is defined as void pointer in the Windows SDK headers.

::: toolkit/modules/subprocess/subprocess_win.jsm:80
(Diff revision 3)
> +  /**
> +   * Searches for the given executable file in the system executable
> +   * file paths as specified by the PATH environment variable.
> +   *
> +   * On Windows, if the unadorned filename cannot be found, the
> +   * extensions in the semicolon-separated list in the PATHSEP

nit: PATHEXT, not PATHSEP. The code is correct, just need to fix this comment.

Comment 15

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review47595

Thanks!

> nit: I don't see how it would make any difference, but HANDLE is defined as void pointer in the Windows SDK headers.

Ah. Quite.

> nit: PATHEXT, not PATHSEP. The code is correct, just need to fix this comment.

Good catch. Not the first time I've been bitten by identifier completion...

Comment 16

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review47563

> is `bytesWritten` intended to be wrapped in an object?  The OutputPipe documentation says write() returns a promise that resolves to a number but with this here, it actually resolves to an object with a property called bytesWritten.  (or is the idea that this should be an object here but that code elsewhere should pull out the actual return value before resolving the original promise?)
> The same issue applies for read/buffer.

I went back and forth on that a few times. I'm leaning toward having all methods resolve to objects with properties, rather than primitives. It makes it clearer exactly what's being returned, and gives us the opportunity to add more data in the future, without breaking existing code, if we need to.

I'm open to alternative arguments, though. For now, I'll update the documentation.

Comment 17

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1269501: Part 1 - Add $PYTHON variable to environment for subprocess xpcshell tests. r=glandium

I'm not especially happy with this method, but the DMD tests are the only
other tests I can find doing anything like this, and I don't have a better
solution than doing it the same way.

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

Comment 18

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1269501: Part 2 - Add additional libc constants to OS.Constants.libc. r=aswan

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

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

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

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

Andrew, can you please review the common and Unix portions, Matt the Windows portions, and Benjamin the code layout and overall API design?

There have only been minor changes since the last feedback request.

Comment 21

[Molly Howell (she/her) (no longer active)]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

https://reviewboard.mozilla.org/r/50235/#review48193

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8750457 [details]
MozReview Request: Bug 1269501: Part 2 - Add additional libc constants to OS.Constants.libc. r=aswan

https://reviewboard.mozilla.org/r/51427/#review48279

This looks fine to me but did you mean to request review from me and not a dom peer?

Comment 23

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50235/#review47073

> At this point, if we're coming from an onInput call, we've already added the buffer and updated our total buffer size, but we don't know if we have enough data to satisfy our most recent pending read, and we don't have any pending read operations.
> 
> If we don't have enough data to satisfy the last read, we're going to need to dispatch a new read operation, and the sooner we do it, the better. If we do, then this is a no-op unless we're below our baseline buffer size.
> 
> In either case, if we're at a point where we want to check if we can satisfy pending reads, we're also at a point where we want to check if we need to fill the buffer.

Arg, the multiple meanings of "pending read" strikes again.  To be clear, I think the first use of "pending read" in the first sentence refers to requests that we're handling and the second use means a pending request to the worker, right?
I think I mis-read this the first time through, I overlooked the fact that this is a no-op if we have enough dataAvailable to satisfy the request at the head of the line.
So, that means that if we have multiple pending requests (the ones we're handling, not the ones we're making to the worker), that fillBuffer() should be called again once we exit the main loop in checkPendingReads().  onInput() doesn't call fillBuffer().  Ah, but it does eventually happen from the loop inside fillBuffer().  So I see how this works, but I'm finding the flow confusing.
I still think it would be a little easier to follow if fillBuffer() moved to the end of checkPendingReads() and just got removed from onInput().  The argument about dispatching the (worker) read as soon as possible doesn't seem that compelling since checkPendingReads() just does a single check of the read size against dataAvailable and then never enters the loop in the case that we don't have enough data to satisfy the current request.

Comment 24

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/51427/#review48279

This isn't the kind of patch that generally requires review by a DOM peer. In any case, the most likely candidate reviewers are all Firefox peers.

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

https://reviewboard.mozilla.org/r/50235/#review48281

Looking good, nice work!

::: toolkit/modules/subprocess/subprocess_shared.js:77
(Diff revisions 1 - 4)
> +  /**
> +   * @property {integer} ERROR_END_OF_FILE
> +   *           The operation failed because the end of the file was reached.
> +   *           @constant
> +   */
> +  ERROR_END_OF_FILE: 0xff7a0001,

mostly for my own curiosity, is there any significance to the upper 16 bits?

::: toolkit/modules/subprocess/subprocess_worker_unix.js:128
(Diff revisions 1 - 4)
>    readBuffer(count) {
>      let buffer = new ArrayBuffer(count);
>  
>      let read = libc.read(this.fd, buffer, buffer.byteLength);
>      if (read < 0 && ctypes.errno != LIBC.EAGAIN) {
> -      this.kill();
> +      this.onError();

Could we log a message that includes errno in this case?  This could be exceptional things like running out of memory but things like EINVAL would imply bugs somewhere.  Finding such a (theoretical) bug would be very difficult but having a log with the error could make it slightly less so.
Likewise with related cases below.

::: toolkit/modules/subprocess/subprocess_worker_unix.js:325
(Diff revisions 1 - 4)
> -    their_pipes[1] = pipe(true);
> +    their_pipes.set(1, pipe(true));
>  
>      if (stderr == "pipe") {
> -      their_pipes[2] = pipe(true);
> +      their_pipes.set(2, pipe(true));
>      } else if (stderr == "stdout") {
> -      their_pipes[2] = their_pipes[1];
> +      their_pipes.set(2, their_pipes[1]);

You need `.get()` here

::: toolkit/modules/subprocess/subprocess_worker_unix.js:398
(Diff revisions 1 - 4)
> +   */
> +  onReady() {
> +    // We're not actually expecting any input on this pipe. If we get any, we
> +    // can't poll the pipe any further without reading it.
> +    if (this.wait() == undefined) {
> +      this.kill(9);

Log something if this actually happens?
I think this could happen if a child process happened to write to fd 3.  If somebody writes a native messaging application that does that and then wonders why it keeps dying, it would be nice to have a marker in the logs to help diagnose this case.

::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js:89
(Diff revisions 1 - 4)
> +  const BUFFER_SIZE = 4096;
> +
> +  // Create a message that's ~3/4 the input buffer size.
> +  let msg = Array(BUFFER_SIZE * .75 / 16 | 0).fill("0123456789abcdef").join("") + "\n";
> +
> +  // This sequence of writes and reads crosses several buffer size

I'm curious what the coverage tool reports with this test?  Getting good coverage relies on the python script getting to complete multiple iterations through echo_loop before xpcshell wakes up and does that first 4KB read.  I guess as long as that does happen at least occassionally that means we get the desired coverage...?

Comment 26

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50235/#review47563

> I went back and forth on that a few times. I'm leaning toward having all methods resolve to objects with properties, rather than primitives. It makes it clearer exactly what's being returned, and gives us the opportunity to add more data in the future, without breaking existing code, if we need to.
> 
> I'm open to alternative arguments, though. For now, I'll update the documentation.

Whoops, just realized I never replied to this.  I don't feel strongly, just as long as we document the actual behavior, which is now the case.

Comment 27

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review48281

> mostly for my own curiosity, is there any significance to the upper 16 bits?

No, they were generated by Math.random, just because `errorCode` is a generic name, and could theoretically be added to any unrelated exception.

> Log something if this actually happens?
> I think this could happen if a child process happened to write to fd 3.  If somebody writes a native messaging application that does that and then wonders why it keeps dying, it would be nice to have a marker in the logs to help diagnose this case.

I guess... but it seems like an extremely unlikely corner case, and they wouldn't be any better off if they happened to write to a nonexistent fd 3 that we didn't have a pipe connected to.

> I'm curious what the coverage tool reports with this test?  Getting good coverage relies on the python script getting to complete multiple iterations through echo_loop before xpcshell wakes up and does that first 4KB read.  I guess as long as that does happen at least occassionally that means we get the desired coverage...?

I just added log statements to all of the relevant branches. It would be nice to have a way to verify this, but I don't think it's worth the trouble it would take.

As for timing issues, this actually works regardless of timing, since we attempt to refill the buffer before we've sliced off the buffer from the first read request, when the max buffer size isn't large enough to hold the entire next request. Even if that wasn't the case, the timing is so much in our favor that we'll get the desired behavior nearly all of the time. And since the undesired behavior doesn't have any ill effects, we still get the coverage we need without any real issues.

Comment 28

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review47073

> Arg, the multiple meanings of "pending read" strikes again.  To be clear, I think the first use of "pending read" in the first sentence refers to requests that we're handling and the second use means a pending request to the worker, right?
> I think I mis-read this the first time through, I overlooked the fact that this is a no-op if we have enough dataAvailable to satisfy the request at the head of the line.
> So, that means that if we have multiple pending requests (the ones we're handling, not the ones we're making to the worker), that fillBuffer() should be called again once we exit the main loop in checkPendingReads().  onInput() doesn't call fillBuffer().  Ah, but it does eventually happen from the loop inside fillBuffer().  So I see how this works, but I'm finding the flow confusing.
> I still think it would be a little easier to follow if fillBuffer() moved to the end of checkPendingReads() and just got removed from onInput().  The argument about dispatching the (worker) read as soon as possible doesn't seem that compelling since checkPendingReads() just does a single check of the read size against dataAvailable and then never enters the loop in the case that we don't have enough data to satisfy the current request.

On balance, I think you're probably right.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8750456 [details]
MozReview Request: Bug 1269501: Part 1 - Add $PYTHON variable to environment for subprocess xpcshell tests. r=glandium

https://reviewboard.mozilla.org/r/51425/#review48619

Comment 30

[Benjamin Smedberg]

I'm traveling today so I won't be able to look at this until tomorrow.

Comment 31

[Andrew Swan [:aswan]]

Attachment: MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r?aswan r?mhowell r?bsmedberg

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

Comment 32

[Andrew Swan [:aswan]]

sorry that was me fat-fingering a push to reviewboard

Comment 33

[Benjamin Smedberg]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

https://reviewboard.mozilla.org/r/50235/#review49433

And I'm really sorry it took this long.

::: toolkit/modules/subprocess/Subprocess.jsm:43
(Diff revision 4)
> +  /**
> +   * Launches a process, and returns a handle to it.
> +   *
> +   * @param {object} options
> +   *        An object describing the process to launch.
> +   * @param {string} options.command

I think we should either not support PATH searching at all, or require that to be an explicitly opt-in behavior. It's a footgun in many ways, especially on Windows where the current directory is in the search path by default.

::: toolkit/modules/subprocess/Subprocess.jsm:48
(Diff revision 4)
> +   * @param {string} options.command
> +   *        The name of the execuable to launch. If not a full path, it is
> +   *        resolved to an executable by searching the directories in $PATH.
> +   * @param {string[]} [options.arguments]
> +   *        A list of strings to pass as arguments to the process.
> +   * @param {object} [options.environment]

Are we still using Object for map-like structures instead of JS Map()? If so, you should define how we get the properties off of this object (all enumerable props? or enumerable+own?)

I'd kinda suggest a Map() is the better data structure for this, but pre-existing API surface may already use Object and if so we should stay consistent.

::: toolkit/modules/subprocess/Subprocess.jsm:56
(Diff revision 4)
> +   * @param {boolean} [options.environmentAppend]
> +   *        If true, append the environment variables passed in `environment` to
> +   *        the existing set of environment variables. Otherwise, the values in
> +   *        'environment' constitute the entire set of environment variables
> +   *        passed to the new process.
> +   * @param {string} [options.stderr]

Where are the docs for the Process API surface? I see separate docs in the unix/windows implementation files, but it's not clear whether/how those would wrap into public-API docs.

Are these set of doccomments meant to be public API docs? If not, have you considered writing one or more .rst files to document the public API of this module?

Things that need to be documented: what happens to launched subprocesses at Firefox shutdown? Are they killed or do we wait for them? Are clients expected to watch for shutdown and kill their own in-progress processes?

::: toolkit/modules/subprocess/Subprocess.jsm:57
(Diff revision 4)
> +   *        If true, append the environment variables passed in `environment` to
> +   *        the existing set of environment variables. Otherwise, the values in
> +   *        'environment' constitute the entire set of environment variables
> +   *        passed to the new process.
> +   * @param {string} [options.stderr]
> +   *        Defines how the process's stderr output is handled. One of:

Why is there no options.stdin ? Do launched processes always get a buffer/pipe stdin stream, or can they inherit the browser stdin? This should be documented.

::: toolkit/modules/subprocess/Subprocess.jsm:58
(Diff revision 4)
> +   *        the existing set of environment variables. Otherwise, the values in
> +   *        'environment' constitute the entire set of environment variables
> +   *        passed to the new process.
> +   * @param {string} [options.stderr]
> +   *        Defines how the process's stderr output is handled. One of:
> +   *

Is stdout always a pipe or can it inherit the browser stdout?

::: toolkit/modules/subprocess/Subprocess.jsm:70
(Diff revision 4)
> +   * @param {string} [options.workdir]
> +   *        The working directory in which to launch the new process.
> +   *
> +   * @returns {Promise<Process>}
> +   *
> +   * @rejects {Error}

You should clarify in these docs that if the process launches successfully but then returns a non-zero exit code, that still resolves this promise and doesn't reject it.

::: toolkit/modules/subprocess/Subprocess.jsm:122
(Diff revision 4)
> +    }
> +    return environment;
> +  },
> +
> +  /**
> +   * Searches for the given executable file in the system executable

This feels like a footgun.

1) Do we need to support PATH search at all for the Subprocess API?
2) If we do, do we need to expose pathSearch as its own supported API surface?

I haven't read the implementation of this yet, but to the extent that we can rely on the OS APIs to do this for us instead of implementing this ourself, that is a huge security relief. If we are using the OS APIs, we should document that!

::: toolkit/modules/subprocess/subprocess_common.jsm:394
(Diff revision 4)
> +   *          May be rejected with an Error object, or an object with similar
> +   *          properties. The object will include an `errorCode` property with
> +   *          one of the following values if it was rejected for the
> +   *          corresponding reason:
> +   *
> +   *            - Subprocess.ERROR_END_OF_FILE: The pipe was closed before

In this case, will the rejection object include the data that was already read at the time the pipe was closed? Or is that data gone? Does that include "normal shutdown" cases?

I don't see addressed in this documentation any concern for the potential to block I/O operations on full pipes. Is that because that is automatically handled by the worker? The python subprocess module discovered this the hard way and had to invent the communicate() API to get around deadlocks, and I want to make sure we aren't in the same situation.

::: toolkit/modules/subprocess/subprocess_common.jsm:585
(Diff revision 4)
> +
> +  /**
> +   * Kills the process.
> +   *
> +   * @param {boolean} [force=false]
> +   *        If true, forcibly terminates the process. Otherwise, attempts to

On *nix this appears to map to SIGTERM and SIGKILL, right? Is there any reason to hide that? I see in the test that force=false doesn't do the same thing: what is the behavior on Windows?

Is it worth keeping the force distinction here given that we can't implement SIGTERM on Windows?

What happens if you try to kill() a process that has already quit on its own? Is this promise resolved or rejected? Do we have tests to ensure that this behaves consistently?

::: toolkit/modules/subprocess/subprocess_worker_win.js:18
(Diff revision 4)
> +              "resource://gre/modules/subprocess/subprocess_worker_common.js");
> +
> +const POLL_INTERVAL = 50;
> +const POLL_TIMEOUT = 0;
> +
> +const TERMINATE_EXIT_CODE = 0x7f;

This seems like an unfortunate magic constant. Why did you choose this value? And why do you then try and translate this back into 9/15 for the wait() functions?

::: toolkit/modules/subprocess/subprocess_worker_win.js:251
(Diff revision 4)
> +      this.writeNext();
> +    });
> +  }
> +
> +  /**
> +   * Initializes an overapped IO read operation to write the data in `buffer` to

overlapped?

::: toolkit/modules/subprocess/subprocess_worker_win.js:393
(Diff revision 4)
> +   * Quotes a string for use as a single command argument, using Windows quoting
> +   * conventions.
> +   *
> +   * @see https://msdn.microsoft.com/en-us/library/17w5ykft(v=vs.85).aspx
> +   */
> +  quoteString(str) {

It's really unfortunate that we have to reimplement this. We have done it wrong at least once before and ended up with security bugs. I don't see any unit tests for this code in this patch: are there tests elsewhere?

I'm guessing there isn't a windows standard library function anywhere which can do this for us?

::: toolkit/modules/subprocess/test/xpcshell/test_subprocess.js:427
(Diff revision 4)
> +  equal(foo, "", "Got expected $FOO value");
> +
> +  ({exitCode} = yield proc.wait());
> +
> +  equal(exitCode, 0, "Got expected exit code");
> +});

I may be missing something in another test file, but it seems that this file is missing a bunch of testcases to handle error conditions. For instance:

* executable file missing
* executable file not marked executable
* executable quits unexpectedly, in particular having written a partial response on stdout and an error message on stderr
* * is the handling of read() on stdin and stderr correct?
* Handling of large reads and writes, especially those that would fill OS buffers or cause deadlocks

For this kind of module it's as important to test that the error conditions are handled correctly and consistently across platforms as the success conditions.

Comment 34

[Benjamin Smedberg]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

sr- primarily for some docs and better failure tests. Most of the rest are just questions, but some should end up being clarified in docs.

Comment 35

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review49433

> I think we should either not support PATH searching at all, or require that to be an explicitly opt-in behavior. It's a footgun in many ways, especially on Windows where the current directory is in the search path by default.

I'd be happy to make it opt-in, and just require callers to call `pathSearch` manually if they need it. The reason that I did this manually rather than relying on OS APIs is that the only portable OS APIs for path searching are built into the spawn/execvp, which don't make error handling easy.

> Are we still using Object for map-like structures instead of JS Map()? If so, you should define how we get the properties off of this object (all enumerable props? or enumerable+own?)
> 
> I'd kinda suggest a Map() is the better data structure for this, but pre-existing API surface may already use Object and if so we should stay consistent.

I mainly went with an object because that's what was used in the original subprocess.jsm API. I'd be OK with making this a map, but I'm not sure that it's necessary, given that all keys are strings, and the most natural way to use this is to pass an object literal.

I'd rather stick with an object, though, and document that only own, enumerable properties will be used.

> Where are the docs for the Process API surface? I see separate docs in the unix/windows implementation files, but it's not clear whether/how those would wrap into public-API docs.
> 
> Are these set of doccomments meant to be public API docs? If not, have you considered writing one or more .rst files to document the public API of this module?
> 
> Things that need to be documented: what happens to launched subprocesses at Firefox shutdown? Are they killed or do we wait for them? Are clients expected to watch for shutdown and kill their own in-progress processes?

The front-end docs are. The worker-side docs are mainly for the people working in the code.

I'm still working on getting the JSDoc docs into a publishable state: https://people.mozilla.org/~kmaglione/subprocess-jsm/Subprocess.html

After that, they should probably be published to our readthedocs site, or MDN.

I'll work on adding those details.

> Why is there no options.stdin ? Do launched processes always get a buffer/pipe stdin stream, or can they inherit the browser stdin? This should be documented.

stdin and stdout are always pipes. I'll document this.

> This feels like a footgun.
> 
> 1) Do we need to support PATH search at all for the Subprocess API?
> 2) If we do, do we need to expose pathSearch as its own supported API surface?
> 
> I haven't read the implementation of this yet, but to the extent that we can rely on the OS APIs to do this for us instead of implementing this ourself, that is a huge security relief. If we are using the OS APIs, we should document that!

I'm not sure whether we need to support it or not. We don't need it for Andrew's current work, but it's something I'd normally expect to see in an API like this. For the time being, I think we should just keep the pathSearch method, but not apply it by default.

> In this case, will the rejection object include the data that was already read at the time the pipe was closed? Or is that data gone? Does that include "normal shutdown" cases?
> 
> I don't see addressed in this documentation any concern for the potential to block I/O operations on full pipes. Is that because that is automatically handled by the worker? The python subprocess module discovered this the hard way and had to invent the communicate() API to get around deadlocks, and I want to make sure we aren't in the same situation.

Any pending reads that can be fulfilled with the data in the buffer will be fulfilled before any read requests are rejected. Any remaining data that isn't enough to fulfill the current pending read is lost. I'll make that clearer in the docs.

All of our read and write operations are asynchronous, so there's no risk of deadlocks because of full pipes. A caller could get into its own deadlock if it waited for a `write` promise to fulfill before trying a `read`, which I suppose deserves a comment.

> On *nix this appears to map to SIGTERM and SIGKILL, right? Is there any reason to hide that? I see in the test that force=false doesn't do the same thing: what is the behavior on Windows?
> 
> Is it worth keeping the force distinction here given that we can't implement SIGTERM on Windows?
> 
> What happens if you try to kill() a process that has already quit on its own? Is this promise resolved or rejected? Do we have tests to ensure that this behaves consistently?

It's unfortunate that we don't have a gentle way to kill processes on Windows, but I still think it should be the default behavior on Unix. I went with force=true/false partly because that was in the original subprocess.jsm API, and partially because there's really nothing similar to most signals on Windows. So if we want to support them where they're supported, I think we should add a separate `signal` method.

I'll add docs about Windows support and the various corner cases. I think that maybe the best solution is to change the default to "false on Unix sytems, true on Windows", and make `false` explicitly unsupported on Windows.

> This seems like an unfortunate magic constant. Why did you choose this value? And why do you then try and translate this back into 9/15 for the wait() functions?

It is an unfortunate magic constant. It's an arbitrary value that we can use to detect that we force killed the process. I translate it back to -9 in the wait function so the return value of a killed process is consistent across platforms, but unfortunately it only works here when we're the ones who kill the process.

> It's really unfortunate that we have to reimplement this. We have done it wrong at least once before and ended up with security bugs. I don't see any unit tests for this code in this patch: are there tests elsewhere?
> 
> I'm guessing there isn't a windows standard library function anywhere which can do this for us?

There are tests for it here: https://reviewboard-hg.mozilla.org/gecko/file/307bef4a6f93/toolkit/modules/subprocess/test/xpcshell/test_subprocess.js#l346

And, unfortunately, no, there's no function in the standard library to do this. There's a standard library to split the arg string, but none to create it, or quote individual arguments.

> I may be missing something in another test file, but it seems that this file is missing a bunch of testcases to handle error conditions. For instance:
> 
> * executable file missing
> * executable file not marked executable
> * executable quits unexpectedly, in particular having written a partial response on stdout and an error message on stderr
> * * is the handling of read() on stdin and stderr correct?
> * Handling of large reads and writes, especially those that would fill OS buffers or cause deadlocks
> 
> For this kind of module it's as important to test that the error conditions are handled correctly and consistently across platforms as the success conditions.

I'll add tests for these things.

Comment 36

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/50235/#review49433

> I'm not sure whether we need to support it or not. We don't need it for Andrew's current work, but it's something I'd normally expect to see in an API like this. For the time being, I think we should just keep the pathSearch method, but not apply it by default.

This version of pathSearch isn't usable for native messaing but I'll end up building my own very limited version of it since the Chrome docs about the "path" property in the native host manifest say:
> On Windows it can be relative to the directory in which the manifest file is located

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8750456 [details]
MozReview Request: Bug 1269501: Part 1 - Add $PYTHON variable to environment for subprocess xpcshell tests. r=glandium

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

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8750457 [details]
MozReview Request: Bug 1269501: Part 2 - Add additional libc constants to OS.Constants.libc. r=aswan

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

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

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

Comment 40

[Benjamin Smedberg]

Comment on attachment 8748359 [details]
MozReview Request: Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell r?bsmedberg

https://reviewboard.mozilla.org/r/50235/#review52464

::: toolkit/modules/subprocess/Subprocess.jsm:154
(Diff revisions 4 - 5)
>     *        An object containing a key for each environment variable to be used
> -   *        in the search.
> +   *        in the search. If not provided, full the current process environment
> +   *        is used.
>     * @returns {Promise<string>}
>     */
> -  pathSearch(command, environment) {
> +  pathSearch(command, environment = this.getEnvironment()) {

Because I don't know JS behavior very well: are function defaults evaluated eagerly, or evaluated at the time the function is called?

Changes in the base environment should be reflected into processes launched after the change.

Comment 41

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/50235/#review52464

> Because I don't know JS behavior very well: are function defaults evaluated eagerly, or evaluated at the time the function is called?
> 
> Changes in the base environment should be reflected into processes launched after the change.

They're evaluated every time the function is called. That's one footgun we learned about from Python and didn't inherit.

Comment 42

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/ed35c5f4d756aa5b073832e21c103ce4faf282bd
Bug 1269501: Part 1 - Add $PYTHON variable to environment for subprocess xpcshell tests. r=glandium

https://hg.mozilla.org/integration/fx-team/rev/1cbbf940ec35031f3ac435dcef16a0ca5f5a7fc7
Bug 1269501: Part 2 - Add additional libc constants to OS.Constants.libc. r=aswan

https://hg.mozilla.org/integration/fx-team/rev/6438b11898cf5775f446ac0ddcf13f9e8506b26a
Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell rs=bsmedberg

Comment 43

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/167e407cb1fcbb08f4f44d40acb2b8bc5f8de382
Bug 1269501: Follow-up: Fix build error on Android. r=bustage

Comment 44

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/3e1d7c5d1a58cb1763a2a78de02f96c6bcd2f06d
Bug 1269501: Follow-up: Fix merge botch in Windows code. r=me

Comment 45

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/1bd815acf6d3d51ded73a0ffbe05bd8a2c515784
Bug 1269501: Follow-up: Fix more win32 bustage that wasn't in the last try run. r=bustage

Comment 46

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing the added test_subprocess.js on Windows.

https://hg.mozilla.org/integration/fx-team/rev/2e2c3435f648e36080c49c46416b710e0b52d55a

Latest push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=1bd815acf6d3d51ded73a0ffbe05bd8a2c515784
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=9607673&repo=fx-team

18:36:56     INFO -  toolkit/modules/subprocess/test/xpcshell/test_subprocess.js | Starting test_subprocess_environment
18:36:56     INFO -  (xpcshell/head.js) | test test_subprocess_environment pending (2)
18:36:56     INFO -  (xpcshell/head.js) | test run_next_test 20 finished (2)
18:36:56     INFO -  Unexpected exception File closed at resource://gre/modules/subprocess/subprocess_worker_win.js:79
18:36:56     INFO -  close@resource://gre/modules/subprocess/subprocess_worker_win.js:79:19
18:36:56     INFO -  onError@resource://gre/modules/subprocess/subprocess_worker_win.js:107:5
18:36:56     INFO -  onReady@resource://gre/modules/subprocess/subprocess_worker_win.js:196:7
18:36:56     INFO -  poll@resource://gre/modules/subprocess/subprocess_worker_win.js:571:11
18:36:56     INFO -  _run_next_test@C:\slave\test\build\tests\xpcshell\head.js:1540:9
18:36:56     INFO -  do_execute_soon/<.run@C:\slave\test\build\tests\xpcshell\head.js:692:9
18:36:56     INFO -  _do_main@C:\slave\test\build\tests\xpcshell\head.js:209:5
18:36:56     INFO -  _execute_test@C:\slave\test\build\tests\xpcshell\head.js:533:5

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Didn't fold one backout into the previous before, pushed it now: https://hg.mozilla.org/integration/fx-team/rev/199230f44725

Comment 48

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/942a381dd8ee20ffb8dc15860efbebc63d3c4eac
Bug 1269501: Part 1 - Add $PYTHON variable to environment for subprocess xpcshell tests. r=glandium

https://hg.mozilla.org/integration/fx-team/rev/4245f854e1511bde9b2cdbc928f5460e821ee3b4
Bug 1269501: Part 2 - Add additional libc constants to OS.Constants.libc. r=aswan

https://hg.mozilla.org/integration/fx-team/rev/1cafe268e0149021d977aca6bf5af1478e52324d
Bug 1269501: Part 3 - Add new Subprocess IPC module. r=aswan r=mhowell rs=bsmedberg

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/942a381dd8ee
https://hg.mozilla.org/mozilla-central/rev/4245f854e151
https://hg.mozilla.org/mozilla-central/rev/1cafe268e014