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

content/browser/browser_child_process_host_impl.cc

 // Copyright (c) 2012 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.
 
 #include "content/browser/browser_child_process_host_impl.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/dump_without_crashing.h"
@@ skipping 108 matching lines @@
   // TODO(phajdan.jr): Check thread after fixing http://crbug.com/167126.
   g_observers.Get().RemoveObserver(observer);
 }
 
 BrowserChildProcessHostImpl::BrowserChildProcessHostImpl(
     content::ProcessType process_type,
     BrowserChildProcessHostDelegate* delegate)
     : 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
   // Construct the privileged attachment broker early in the life cycle of a
   // child process. This ensures that when a test is being run in one of the
   // single process modes, the global attachment broker is the privileged
   // attachment broker, rather than an unprivileged attachment broker.
 #if defined(OS_MACOSX)
   IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded(
       MachBroker::GetInstance());
 #else
   IPC::AttachmentBrokerPrivileged::CreateBrokerIfNeeded();
 #endif  // defined(OS_MACOSX)
 #endif  // USE_ATTACHMENT_BROKER
 
   child_process_host_.reset(ChildProcessHost::Create(this));
   AddFilter(new TraceMessageFilter(data_.id));
   AddFilter(new ProfilerMessageFilter(process_type));
   AddFilter(new HistogramMessageFilter);
   AddFilter(new MemoryMessageFilter(this, process_type));
 
   g_child_process_list.Get().push_back(this);
   GetContentClient()->browser()->BrowserChildProcessHostCreated(this);
 
   power_monitor_message_broadcaster_.Init();
 }
 
 BrowserChildProcessHostImpl::~BrowserChildProcessHostImpl() {
   g_child_process_list.Get().remove(this);
+
+  if (notify_child_disconnected_) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(&NotifyProcessHostDisconnected, data_));
+  }
 }
 
 // static
 void BrowserChildProcessHostImpl::TerminateAll() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   // Make a copy since the BrowserChildProcessHost dtor mutates the original
   // list.
   BrowserChildProcessList copy = g_child_process_list.Get();
   for (BrowserChildProcessList::iterator it = copy.begin();
        it != copy.end(); ++it) {
@@ skipping 17 matching lines @@
     switches::kEnableLogging,
     switches::kIPCConnectionTimeout,
     switches::kLoggingLevel,
     switches::kTraceToConsole,
     switches::kV,
     switches::kVModule,
   };
   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,
       data_.id,
       this,
       terminate_on_shutdown));
 }
 
 const ChildProcessData& BrowserChildProcessHostImpl::GetData() const {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ skipping 67 matching lines @@
 
 bool BrowserChildProcessHostImpl::OnMessageReceived(
     const IPC::Message& message) {
   return delegate_->OnMessageReceived(message);
 }
 
 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
   // error on the IPC channel.
   early_exit_watcher_.StopWatching();
 #endif
 
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           base::Bind(&NotifyProcessHostConnected, data_));
 
@@ skipping 87 matching lines @@
                               data_.process_type,
                               PROCESS_TYPE_MAX);
 #if defined(OS_CHROMEOS)
     if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM) {
       UMA_HISTOGRAM_ENUMERATION("ChildProcess.Killed2.OOM",
                                 data_.process_type,
                                 PROCESS_TYPE_MAX);
     }
 #endif
   }
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(&NotifyProcessHostDisconnected, data_));
   delete delegate_;  // Will delete us
 }
 
 bool BrowserChildProcessHostImpl::Send(IPC::Message* message) {
   return child_process_host_->Send(message);
 }
 
 void BrowserChildProcessHostImpl::OnProcessLaunchFailed() {
   delegate_->OnProcessLaunchFailed();
+  notify_child_disconnected_ = false;
   delete delegate_;  // Will delete us
 }
 
 void BrowserChildProcessHostImpl::OnProcessLaunched() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   const base::Process& process = child_process_->GetProcess();
   DCHECK(process.IsValid());
 
   mojo::edk::ScopedPlatformHandle client_pipe =
@@ skipping 29 matching lines @@
 
 #if defined(OS_WIN)
 
 void BrowserChildProcessHostImpl::OnObjectSignaled(HANDLE object) {
   OnChildDisconnected();
 }
 
 #endif
 
 }  // namespace content