Report text output and exit code for command-line operations.

apps/app_host/operation_launcher.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Creates an anonymous pipe for communicating an exit code from an operation
+// implemented by one or more subprocesses. If the original subprocess exits
+// without writing its result and without duplicating its output HANDLE into
+// some other process a failure is assumed to have occurred. Otherwise the
+// client closes its copy of the output HANDLE and the operation is considered
+// to be in progress until either its result has been read or no more valid
+// output HANDLEs remain.
+
+#include "operation_launcher.h"
+
+#include "base/command_line.h"
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/process_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/platform_thread.h"
+#include "base/win/scoped_handle.h"
+#include "chrome/common/chrome_switches.h"
+
+namespace app_host {
+
+namespace {
+
+// Implements a background thread to read the exit code of the child process.
+class PipeReader : public base::PlatformThread::Delegate {

[gab, 2013/03/28 03:06:06]

**I have no idea what I'm doing** (never wrote a background thread in
Chromium...), but according to platform_thread.h, you should use Thread from
base/threading/thread.h instead:

"WARNING: You should *NOT* be using this class directly."

[gab, 2013/03/28 03:06:06]

PipeReader sounds a bit too generic for a class with such a specific function
imo

[erikwright (departed), 2013/04/18 17:43:04]

That's a good warning, but all classes exist to be used and I think this is an
appropriate place to use that class.

This code runs outside of Chrome and the world of message loops. Furthermore,
this code is intimately tied to the platform-specific behaviour of threads and
anonymous pipes. And finally, this code needs to do something that is only
possible with this closer-to-the-metal abstraction, which is to
WaitOnMultipleObjects.

The alternatives to using PlatformThread would be:
1) Break encapsulation of a higher-level construct such as SimpleThread or
Thread and expose the PlatformThreadHandle
2) Use the raw Win32 ::CreateThread API and friends.

+ public:
+  PipeReader(base::win::ScopedHandle exit_code_read)

[gab, 2013/03/28 03:06:06]

"read" feels like the past-tense of "read" (aaah English!) which doesn't sound
like a handle at all to me.

I understood why you choose that name when making it to line 70, but please
consider my naming suggestion there.

+      : success_(false), exit_code_(0) {
+    exit_code_read_ = exit_code_read.Pass();

[gab, 2013/03/28 03:06:06]

Why can't this also be done in the initializer list?

[erikwright (departed), 2013/04/18 17:43:04]

It doesn't compile. I forget the exact reason why, but I believe there are
ambiguous single-argument constructors or something. I'll try to get the error
output if I have a moment.

+  }
+
+  virtual void ThreadMain() OVERRIDE {
+    DWORD bytes_read = 0;
+    BOOL read_success = ::ReadFile(
+        exit_code_read_, &exit_code_, sizeof(exit_code_), &bytes_read, NULL);
+    if (!read_success || bytes_read != sizeof(exit_code_)) {
+        LOG(ERROR) << "Failed to read exit code.";
+    } else {
+      success_ = true;
+    }
+  }
+
+  // Only call these accessors after the PipeReader thread has completed.
+
+  DWORD exit_code() {
+    return exit_code_;

[gab, 2013/03/28 03:06:06]

Such simple getters are usually written on a single-line.

In fact I feel like lines 49-56 could be shrunk to two lines:

DWORD exit_code() { return exit_code_; }
bool success() { return success_; }

[erikwright (departed), 2013/04/18 17:43:04]

Done.

+  }
+
+  bool success() {
+    return success_;
+  }
+
+ private:
+  bool success_;
+  DWORD exit_code_;
+
+  base::win::ScopedHandle exit_code_read_;
+};
+
+}  // namespace
+
+bool LaunchOperation(const CommandLine& command_line,
+                     HANDLE output_write,
+                     DWORD* exit_code) {
+  base::win::ScopedHandle exit_code_read;
+  base::win::ScopedHandle exit_code_write;

[gab, 2013/03/28 03:06:06]

How about exit_code_in/exit_code_out (see comment on line 32 for justification).

+
+  if (::CreatePipe(exit_code_read.Receive(), exit_code_write.Receive(),
+                   NULL, 0) == 0) {

[robertshield, 2013/03/28 02:38:03]

I'm curious: how does this work? I had understood that in order for the returned
handles to be inheritable, the security attributes in the 3rd param had to be
non-null. Specifically, should not the bInheritHandle in the SECURITY_ATTRIBUTES
structure be set to true so that the child process gets the handle to
exit_code_write?

[erikwright (departed), 2013/03/28 02:49:53]

They are not inherited. Making them inherited would be dangerous because Chrome
could quite feasibly start any number of child processes that would also inherit
them. Then they would never close, which would make it hard to detect the end of
the stream.

There would still be the question, furthermore, of how the new chrome.exe would
pass them into the pre-existing chrome.exe (ProcessSingleton rendezvous case).
Presumably process singleton would need to duplicate them into the pre-existing
chrome process and then change the command line passed to that process.

Furthermore, if they were inherited, I'm not sure we would even know the right
HANDLE to pass to the child process. Is it the same as the HANDLE in the parent
process?

Anyway, what we _do_ do is pass the parent process ID and handles valid in the
parent process. The child can use that info to duplicate the handles into its
own process.

[gab, 2013/03/28 03:06:06]

How about using '!' instead of '== 0' check? Feels more natural to me.

[robertshield, 2013/03/28 03:39:34]

Ah, sorry should have read the other end first ;-) I saw passing a handle on the
command line to the child process and assumed you were trying inheritance.
Anyhow, carry on. 

Since I wondered about it, perhaps also add a comment that the exit_code_write
handles is intentionally not being inherited.  

[erikwright (departed), 2013/04/18 17:43:04]

I would defer to a senior Windows developer on this. The current form seems more
correct but I don't feel too strongly.

+    PLOG(ERROR) << "CreatePipe failed.";

[gab, 2013/03/28 03:06:06]

DPLOG (avoids shipping a ton of log strings in the binary; unless you feel
strongly this should make it in user's logs...?)

Same goes for LOG->DLOG all over this CL.

[erikwright (departed), 2013/04/18 17:43:04]

Done.

+    return false;
+  }
+
+  CommandLine full_command_line(command_line);
+  full_command_line.AppendSwitchASCII(
+      switches::kTaskOutputHandle,
+      base::UintToString(reinterpret_cast<int>(output_write)));
+  full_command_line.AppendSwitchASCII(
+      switches::kTaskResultHandle,
+      base::UintToString(reinterpret_cast<int>(exit_code_write.Get())));
+  full_command_line.AppendSwitchASCII(
+      switches::kTaskRemoteProcessId,
+      base::UintToString(::GetCurrentProcessId()));
+
+  base::win::ScopedHandle child_process;
+  base::win::ScopedHandle background_thread;
+
+  bool launch_result = base::LaunchProcess(full_command_line,
+                                           base::LaunchOptions(),
+                                           child_process.Receive());
+  if (!launch_result) {
+    LOG(INFO) << "Failed to execute "
+              << full_command_line.GetCommandLineString();
+    return false;
+  }
+
+  LOG(INFO) << "Executed " << full_command_line.GetCommandLineString();
+
+  PipeReader pipe_reader(exit_code_read.Pass());
+  base::PlatformThread::Create(0, &pipe_reader, background_thread.Receive());
+
+  HANDLE handles[] = {child_process.Get(), background_thread.Get()};
+  DWORD wait_result = WaitForMultipleObjects(2, handles, FALSE, INFINITE);
+  switch (wait_result) {
+    case WAIT_FAILED:
+      LOG(ERROR) << "Wait for operation failed.";
+      break;
+    case WAIT_OBJECT_0:
+      // The child process exited. If the operation is still in progress
+      // (delegated to a tertiary process) that process is required to have
+      // duplicated the write handles by now; the closing of all outstanding
+      // handles to exit_code_write will be our signal that the operation has
+      // completed.
+      exit_code_write.Close();

[robertshield, 2013/03/28 02:38:03]

Why do we Close() here, rather than doing so when the ScopedHandle goes out of
scope?

[erikwright (departed), 2013/03/28 02:49:53]

We will detect the termination of the operation when we can no longer ReadFile()
on the read-end of the pipe.

As long as there is an open HANDLE to the write-end of the pipe, ReadFile will
block. By closing our HANDLE to the write-end, we ensure that we will be able to
detect when the operation handler closes its HANDLE to the write-end.

+      break;
+    case WAIT_OBJECT_0 + 1:
+      // The reader thread has completed. We have read (or permanently failed to
+      // read) the operation exit code. The child process may or may not exit at
+      // this point, but we already have everything we need.
+      break;
+    default:
+      NOTREACHED();
+      return false;
+      break;

[gab, 2013/03/28 03:06:06]

Remove break;, already preceded by a return statement.

[erikwright (departed), 2013/04/18 17:43:04]

Done.

+  }
+
+  // Whether or not the thread is still running, allow Join to do whatever
+  // cleanup is required.
+  base::PlatformThread::Join(background_thread.Take());
+
+  if (pipe_reader.success())
+    *exit_code = pipe_reader.exit_code();
+  return pipe_reader.success();
+}
+
+}  // namespace app_host