Notify observers if a running browser child process is shut down by deleting the BrowserChildProces…

content/browser/browser_child_process_host_impl.cc

Index: content/browser/browser_child_process_host_impl.cc
diff --git a/content/browser/browser_child_process_host_impl.cc b/content/browser/browser_child_process_host_impl.cc
index cde59de50797ccf1dc912af4b48496494076d046..c47a32d36357ae3cd9616c879e98ca45db4f1075 100644
--- a/content/browser/browser_child_process_host_impl.cc
+++ b/content/browser/browser_child_process_host_impl.cc
@@ -126,7 +126,8 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
     : data_(process_type),
       delegate_(delegate),
       power_monitor_message_broadcaster_(this),
-      is_channel_connected_(false) {
+      is_channel_connected_(false),
+      notify_child_disconnected_(false) {
   data_.id = ChildProcessHostImpl::GenerateChildProcessUniqueId();
 
 #if USE_ATTACHMENT_BROKER
@@ -156,6 +157,11 @@ BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
 
 BrowserChildProcessHostImpl::~BrowserChildProcessHostImpl() {
   g_child_process_list.Get().remove(this);
+
+  if (notify_child_disconnected_) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(&NotifyProcessHostDisconnected, data_));
+  }
 }
 
 // static
@@ -193,6 +199,7 @@ void BrowserChildProcessHostImpl::Launch(
   cmd_line->CopySwitchesFrom(browser_command_line, kForwardSwitches,
                              arraysize(kForwardSwitches));
 
+  notify_child_disconnected_ = true;

[ncarter (slow), 2016/03/31 16:06:15]

Actually, is this line necessary? If we're only notifying on disconnect, then I
think we only need to set the bit to true when the channel connects.

[Anand Mistry (off Chromium), 2016/03/31 18:51:33]

I believe so. I thought about this, but then I notice this little comment in
OnChildDisconnected:
// OnChildDisconnected may be called without OnChannelConnected, so stop the
// early exit watcher so GetTerminationStatus can close the process handle.

[ncarter (slow), 2016/03/31 20:17:31]

That comment is correct: OnProcessLaunched() can occur either before, or after,
OnChannelConnected(). But when OnProcessLaunched() happens first, we don't
notify the observers about anything until OnChannelConnected() happens.

We're just trying to make it so that NotifyProcessHostConnected is always
balanced with NotifyProcessHostDisconnected, so it seems like this needs to be
set to true only from OnChannelConnected().

[Anand Mistry (off Chromium), 2016/03/31 22:14:10]

It seems that the current behaviour isn't balanced. OnChildDisconnected() always
runs NotifyProcessHostDisconnected, regardless of whether or not
OnChannelConnected() was run. Maybe this is a bug, I don't know. I'm just trying
to preserve existing behaviour.

There's another detail which might help explain why I've done this change as it
is... in-process unit tests. In this case, I believe OnChannelConnected() is run
without OnProcessLauncher() being run or Launch() being called.

   child_process_.reset(new ChildProcessLauncher(
       delegate,
       cmd_line,
@@ -280,6 +287,7 @@ void BrowserChildProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   is_channel_connected_ = true;
+  notify_child_disconnected_ = true;
 
 #if defined(OS_WIN)
   // From this point onward, the exit of the child process is detected by an
@@ -387,8 +395,6 @@ void BrowserChildProcessHostImpl::OnChildDisconnected() {
     }
 #endif
   }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(&NotifyProcessHostDisconnected, data_));
   delete delegate_;  // Will delete us
 }
 
@@ -398,6 +404,7 @@ bool BrowserChildProcessHostImpl::Send(IPC::Message* message) {
 
 void BrowserChildProcessHostImpl::OnProcessLaunchFailed() {
   delegate_->OnProcessLaunchFailed();
+  notify_child_disconnected_ = false;
   delete delegate_;  // Will delete us
 }