Fix three Windows subprocess issues

Summary:
Fixes T13504. This fixes three issues:

  # In ExecFuture, "proc_open()" on an invalid binary could fail with an unconditional exception.
  # In ExecPassthru, "proc_open()" on an invalid binary could fail with an unconditional exception.
  # In "arc browse", "start <url>" does not work when the shell is bypassed.

In (1) and (2), the desired behavior is to fail with an exit code which is sometimes upgraded to an exception depending on calling convention.

Issue (1) most commonly manifested as "find" failing when run via "cmd.exe".

Issue (2) most commonly manifested as "arc browse" failing.

Issue (3) was entangled with issue (2).

In cases (1) and (2), assume "proc_open()" failures under Windows are because of bad binaries and treat them like bogus commands on Linux/Mac.

In case (3), use "cmd /c start" instead of "start" as a default browser on Windows.

Test Plan:
  - On Windows, did mime type detection in cmd.exe. Before patch: proc_open() exception in "find". After patch: clean (albeit not terribly useful) detection.
  - On Windows, did "arc browse ...". Before patch: proc_open() exception in "start". After patch: clean browser execution.

Maniphest Tasks: T13504

Differential Revision: https://secure.phabricator.com/D21047

src/future/exec/ExecFuture.php

@@ -605,6 +605,9 @@ final class ExecFuture extends PhutilExecutableFuture {
         $trap->destroy();
       } else {
         $err = error_get_last();
+        if ($err) {
+          $err = $err['message'];
+        }
       }
 
       if ($is_windows) {
@@ -618,18 +621,21 @@ final class ExecFuture extends PhutilExecutableFuture {
         // it fails to resolve the command.
 
         // When you run an invalid command on a Windows system, we bypass the
-        // shell and the "proc_open()" itself fails. Throw a "CommandException"
+        // shell and the "proc_open()" itself fails. See also T13504. Fail the
-        // here for consistency with the Linux behavior in this common failure
+        // future immediately, acting as though it exited with an error code
-        // case.
+        // for consistency with Linux.
 
-        throw new CommandException(
+        $result = array(
+          1,
+          '',
           pht(
             'Call to "proc_open()" to open a subprocess failed: %s',
             $err),
-          $this->getCommand(),
+        );
-          1,
+
-          '',
+        $this->setResult($result);
-          '');
+
+        return true;
       }
 
       if ($is_windows) {

src/future/exec/PhutilExecPassthru.php

@@ -68,15 +68,22 @@ final class PhutilExecPassthru extends PhutilExecutableFuture {
     $trap->destroy();
 
     if (!is_resource($proc)) {
-      throw new Exception(
+      // See T13504. When "proc_open()" is given a command with a binary
-        pht(
+      // that isn't available on Windows with "bypass_shell", the function
-          'Failed to passthru %s: %s',
+      // fails entirely.
-          'proc_open()',
+      if (phutil_is_windows()) {
-          $errors));
+        $err = 1;
+      } else {
+        throw new Exception(
+          pht(
+            'Failed to passthru %s: %s',
+            'proc_open()',
+            $errors));
+      }
+    } else {
+      $err = proc_close($proc);
     }
 
-    $err = proc_close($proc);
-
     return $err;
   }
 

src/workflow/ArcanistWorkflow.php

@@ -2069,13 +2069,19 @@ abstract class ArcanistWorkflow extends Phobject {
 
   protected function openURIsInBrowser(array $uris) {
     $browser = $this->getBrowserCommand();
+
+    // The "browser" may actually be a list of arguments.
+    if (!is_array($browser)) {
+      $browser = array($browser);
+    }
+
     foreach ($uris as $uri) {
-      $err = phutil_passthru('%s %s', $browser, $uri);
+      $err = phutil_passthru('%LR %R', $browser, $uri);
       if ($err) {
         throw new ArcanistUsageException(
           pht(
-            "Failed to open '%s' in browser ('%s'). ".
+            'Failed to open URI "%s" in browser ("%s"). '.
-            "Check your 'browser' config option.",
+            'Check your "browser" config option.',
             $uri,
             $browser));
       }
@@ -2089,19 +2095,29 @@ abstract class ArcanistWorkflow extends Phobject {
     }
 
     if (phutil_is_windows()) {
-      return 'start';
+      // See T13504. We now use "bypass_shell", so "start" alone is no longer
+      // a valid binary to invoke directly.
+      return array(
+        'cmd',
+        '/c',
+        'start',
+      );
     }
 
-    $candidates = array('sensible-browser', 'xdg-open', 'open');
+    $candidates = array(
+      'sensible-browser' => array('sensible-browser'),
+      'xdg-open' => array('xdg-open'),
+      'open' => array('open', '--'),
+    );
 
     // NOTE: The "open" command works well on OS X, but on many Linuxes "open"
     // exists and is not a browser. For now, we're just looking for other
     // commands first, but we might want to be smarter about selecting "open"
     // only on OS X.
 
-    foreach ($candidates as $cmd) {
+    foreach ($candidates as $cmd => $argv) {
       if (Filesystem::binaryExists($cmd)) {
-        return $cmd;
+        return $argv;
       }
     }