Bug 1675332 - Pre: Make `VALUE: null` remove env vars in Subprocess.jsm. r=kmag

This small tweak makes it ergonomic to remove variables from the
inherited environment.  Using `null` to signal "removal", distinct
from `undefined` for "ignored", seems to be an accepted idiom in these
types of JS interfaces.

Differential Revision: https://phabricator.services.mozilla.com/D95896

toolkit/modules/subprocess/Subprocess.jsm

@@ -75,16 +75,17 @@ var Subprocess = {
    * @param {string[]} [options.arguments]
    * A list of strings to pass as arguments to the process.
    *
-   * @param {object} [options.environment]
-   * An object containing a key and value for each environment variable
-   * to pass to the process. Only the object's own, enumerable properties
-   * are added to the environment.
+   * @param {object} [options.environment] An object containing a key
+   * and value for each environment variable to pass to the
+   * process. Values that are `=== null` are ignored. Only the
+   * object's own, enumerable properties are added to the environment.
    *
-   * @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 {boolean} [options.environmentAppend] If true, append the
+   * environment variables passed in `environment` to the existing set
+   * of environment variables. Values that are `=== null` are removed
+   * from the environment. 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:
@@ -134,9 +135,9 @@ var Subprocess = {
       Object.assign(environment, options.environment);
     }
 
-    options.environment = Object.entries(environment).map(([key, val]) =>
-      encodeEnvVar(key, val)
-    );
+    options.environment = Object.entries(environment)
+      .map(([key, val]) => (val !== null ? encodeEnvVar(key, val) : null))
+      .filter(s => s);
 
     options.arguments = Array.from(options.arguments || []);
 

toolkit/modules/subprocess/test/xpcshell/data_test_script.py

@@ -35,7 +35,7 @@ elif cmd == "exit":
     sys.exit(int(sys.argv[2]))
 elif cmd == "env":
     for var in sys.argv[2:]:
-        output(os.environ.get(var, ""))
+        output(os.environ.get(var, "!"))
 elif cmd == "pwd":
     output(os.path.abspath(os.curdir))
 elif cmd == "print_args":

toolkit/modules/subprocess/test/xpcshell/test_subprocess.js

@@ -683,6 +683,8 @@ add_task(async function test_subprocess_arguments() {
 add_task(async function test_subprocess_environment() {
   let environment = {
     FOO: "BAR",
+    EMPTY: "",
+    IGNORED: null,
   };
 
   // Our Windows environment can't handle launching python without
@@ -698,15 +700,19 @@ add_task(async function test_subprocess_environment() {
 
   let proc = await Subprocess.call({
     command: PYTHON,
-    arguments: ["-u", TEST_SCRIPT, "env", "FOO", "BAR"],
+    arguments: ["-u", TEST_SCRIPT, "env", "FOO", "BAR", "EMPTY", "IGNORED"],
     environment,
   });
 
   let foo = await read(proc.stdout);
   let bar = await read(proc.stdout);
+  let empty = await read(proc.stdout);
+  let ignored = await read(proc.stdout);
 
   equal(foo, "BAR", "Got expected $FOO value");
-  equal(bar, "", "Got expected $BAR value");
+  equal(bar, "!", "Got expected $BAR value");
+  equal(empty, "", "Got expected $EMPTY value");
+  equal(ignored, "!", "Got expected $IGNORED value");
 
   let { exitCode } = await proc.wait();
 
@@ -715,6 +721,8 @@ add_task(async function test_subprocess_environment() {
 
 add_task(async function test_subprocess_environmentAppend() {
   env.set("VALUE_FROM_BASE_ENV", "untouched");
+  env.set("VALUE_FROM_BASE_ENV_EMPTY", "untouched");
+  env.set("VALUE_FROM_BASE_ENV_REMOVED", "untouched");
 
   let proc = await Subprocess.call({
     command: PYTHON,
@@ -723,15 +731,21 @@ add_task(async function test_subprocess_environmentAppend() {
       TEST_SCRIPT,
       "env",
       "VALUE_FROM_BASE_ENV",
+      "VALUE_FROM_BASE_ENV_EMPTY",
+      "VALUE_FROM_BASE_ENV_REMOVED",
       "VALUE_APPENDED_ONCE",
     ],
     environmentAppend: true,
     environment: {
+      VALUE_FROM_BASE_ENV_EMPTY: "",
+      VALUE_FROM_BASE_ENV_REMOVED: null,
       VALUE_APPENDED_ONCE: "soon empty",
     },
   });
 
   let valueFromBaseEnv = await read(proc.stdout);
+  let valueFromBaseEnvEmpty = await read(proc.stdout);
+  let valueFromBaseEnvRemoved = await read(proc.stdout);
   let valueAppendedOnce = await read(proc.stdout);
 
   equal(
@@ -739,6 +753,16 @@ add_task(async function test_subprocess_environmentAppend() {
     "untouched",
     "Got expected $VALUE_FROM_BASE_ENV value"
   );
+  equal(
+    valueFromBaseEnvEmpty,
+    "",
+    "Got expected $VALUE_FROM_BASE_ENV_EMPTY value"
+  );
+  equal(
+    valueFromBaseEnvRemoved,
+    "!",
+    "Got expected $VALUE_FROM_BASE_ENV_REMOVED value"
+  );
   equal(
     valueAppendedOnce,
     "soon empty",
@@ -769,7 +793,7 @@ add_task(async function test_subprocess_environmentAppend() {
     "untouched",
     "Got expected $VALUE_FROM_BASE_ENV value"
   );
-  equal(valueAppendedOnce, "", "Got expected $VALUE_APPENDED_ONCE value");
+  equal(valueAppendedOnce, "!", "Got expected $VALUE_APPENDED_ONCE value");
 
   ({ exitCode } = await proc.wait());