Windows: BrowserChildProcessHostImpl reports OnProcessCrashed if the child process crashes before…

Windows: BrowserChildProcessHostImpl reports OnProcessCrashed if the child process crashes before connecting the IPC channel.

When a child process crashes, this is generally detected by an error on the IPC channel, i.e. OnChannelError being called. However, if the child process exits or crashes before this IPC channel has been established, the crash is lost. The browser process sees the child process being launched but never learns of its termination.

This patch uses a WaitableEventWatcher to track the lifetime of the child process up to the point the IPC channel is connected. If the child process crashes in the interim, the usual OnProcessCrashed handler is invoked.

This is important for the GPU child process because it is prone to crashing before the IPC channel is established; it has to initialize Direct3D before the sandbox is turned on. This would lead to a browser hang if Direct3D or the driver crashed during initialization.

BUG=177611
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186498

[cpu_(ooo_6.6-7.5), 2013-03-04 22:46:59]

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:94:
process_waitable_event_->Release();
call GetWatchedEvent to obtain this?

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:220:
early_exit_watcher_.StopWatching();
maybe this ahead of the delegate_ call?

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:286:
process_waitable_event_.reset(
do we really need the process_waitable_event_ as a member?

can we just new it and pass it to the watcher?

[apatrick_chromium, 2013-03-05 00:22:41]

PTAL

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:94:
process_waitable_event_->Release();
On 2013/03/04 22:46:59, cpu wrote:
> call GetWatchedEvent to obtain this?

Done.

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:220:
early_exit_watcher_.StopWatching();
On 2013/03/04 22:46:59, cpu wrote:
> maybe this ahead of the delegate_ call?

Done.

https://codereview.chromium.org/12374061/diff/4002/content/browser/browser_ch...
content/browser/browser_child_process_host_impl.cc:286:
process_waitable_event_.reset(
On 2013/03/04 22:46:59, cpu wrote:
> do we really need the process_waitable_event_ as a member?
> 
> can we just new it and pass it to the watcher?

Done.

[cpu_(ooo_6.6-7.5), 2013-03-05 00:42:40]

lgtm

[apatrick_chromium, 2013-03-05 00:44:06]

+sky for owners

[sky, 2013-03-05 03:52:15]

LGTM

[commit-bot: I haz the power, 2013-03-05 19:53:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/12374061/1007

[commit-bot: I haz the power, 2013-03-05 19:53:01]

Failed to apply patch for content/browser/browser_child_process_host_impl.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file content/browser/browser_child_process_host_impl.cc
  Hunk #2 succeeded at 110 (offset 23 lines).
  Hunk #3 FAILED at 209.
  Hunk #4 succeeded at 295 (offset 26 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
content/browser/browser_child_process_host_impl.cc.rej

Patch:       content/browser/browser_child_process_host_impl.cc
Index: content/browser/browser_child_process_host_impl.cc
===================================================================
--- content/browser/browser_child_process_host_impl.cc	(revision 184931)
+++ content/browser/browser_child_process_host_impl.cc	(working copy)
@@ -15,6 +15,7 @@
 #include "base/process_util.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
+#include "base/synchronization/waitable_event.h"
 #include "content/browser/histogram_message_filter.h"
 #include "content/browser/loader/resource_message_filter.h"
 #include "content/browser/profiler_message_filter.h"
@@ -86,6 +87,10 @@
 
 BrowserChildProcessHostImpl::~BrowserChildProcessHostImpl() {
   g_child_process_list.Get().remove(this);
+
+#if defined(OS_WIN)
+  DeleteProcessWaitableEvent(early_exit_watcher_.GetWatchedEvent());
+#endif
 }
 
 // static
@@ -204,6 +209,13 @@
 }
 
 void BrowserChildProcessHostImpl::OnChannelConnected(int32 peer_pid) {
+#if defined(OS_WIN)
+  // From this point onward, the exit of the child process is detected by an
+  // error on the IPC channel.
+  DeleteProcessWaitableEvent(early_exit_watcher_.GetWatchedEvent());
+  early_exit_watcher_.StopWatching();
+#endif
+
   Notify(NOTIFICATION_CHILD_PROCESS_HOST_CONNECTED);
   delegate_->OnChannelConnected(peer_pid);
 }
@@ -264,8 +276,43 @@
     delete delegate_;  // Will delete us
     return;
   }
+
+#if defined(OS_WIN)
+  // Start a WaitableEventWatcher that will invoke OnProcessExitedEarly if the
+  // child process exits. This watcher is stopped once the IPC channel is
+  // connected and the exit of the child process is detecter by an error on the
+  // IPC channel thereafter.
+  DCHECK(!early_exit_watcher_.GetWatchedEvent());
+  early_exit_watcher_.StartWatching(
+      new base::WaitableEvent(child_process_->GetHandle()),
+      base::Bind(&BrowserChildProcessHostImpl::OnProcessExitedEarly,
+                 base::Unretained(this)));
+#endif
+
   data_.handle = child_process_->GetHandle();
   delegate_->OnProcessLaunched();
 }
 
+#if defined(OS_WIN)
+
+void BrowserChildProcessHostImpl::DeleteProcessWaitableEvent(
+    base::WaitableEvent* event) {
+  if (!event)
+    return;
+
+  // The WaitableEvent does not own the process handle so ensure it does not
+  // close it.
+  event->Release();
+
+  delete event;
+}
+
+void BrowserChildProcessHostImpl::OnProcessExitedEarly(
+    base::WaitableEvent* event) {
+  DeleteProcessWaitableEvent(event);
+  OnChildDisconnected();
+}
+
+#endif
+
 }  // namespace content

[commit-bot: I haz the power, 2013-03-05 22:56:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/12374061/22002

[commit-bot: I haz the power, 2013-03-06 01:21:07]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-06 20:01:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/12374061/22002

[commit-bot: I haz the power, 2013-03-06 20:47:51]

Change committed as 186498