Re-fix http://crbug.com/87176, and add a test.

content/browser/renderer_host/render_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.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 369 matching lines @@
     bool is_guest)
         : fast_shutdown_started_(false),
           deleting_soon_(false),
           pending_views_(0),
           visible_widgets_(0),
           backgrounded_(true),
           cached_dibs_cleaner_(
               FROM_HERE, base::TimeDelta::FromSeconds(5),
               this, &RenderProcessHostImpl::ClearTransportDIBCache),
           is_initialized_(false),
+          child_process_launcher_factory_(base::Bind(&NewChildProcessLauncher)),
           id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
           browser_context_(browser_context),
           storage_partition_impl_(storage_partition_impl),
           sudden_termination_allowed_(true),
           ignore_input_events_(false),
 #if defined(OS_ANDROID)
           dummy_shutdown_event_(false, false),
 #endif
           supports_browser_plugin_(supports_browser_plugin),
           is_guest_(is_guest),
@@ skipping 132 matching lines @@
     // first so the process type argument will appear first.
     CommandLine* cmd_line = new CommandLine(renderer_path);
     if (!renderer_prefix.empty())
       cmd_line->PrependWrapper(renderer_prefix);
     AppendRendererCommandLine(cmd_line);
     cmd_line->AppendSwitchASCII(switches::kProcessChannelID, channel_id);
 
     // Spawn the child process asynchronously to avoid blocking the UI thread.
     // As long as there's no renderer prefix, we can use the zygote process
     // at this stage.
-    child_process_launcher_.reset(new ChildProcessLauncher(
+    child_process_launcher_ = child_process_launcher_factory_.Run(
 #if defined(OS_WIN)
         new RendererSandboxedProcessLauncherDelegate,
 #elif defined(OS_POSIX)
         renderer_prefix.empty(),
         base::EnvironmentVector(),
         channel_->TakeClientFileDescriptor(),
 #endif
         cmd_line,
         GetID(),
-        this));
+        this);
 
     fast_shutdown_started_ = false;
   }
 
   if (!gpu_observer_registered_) {
     gpu_observer_registered_ = true;
     GpuDataManagerImpl::GetInstance()->AddObserver(this);
   }
 
   is_initialized_ = true;
@@ skipping 33 matching lines @@
       GetID(), PROCESS_TYPE_RENDERER, resource_context,
       storage_partition_impl_->GetAppCacheService(),
       ChromeBlobStorageContext::GetFor(browser_context),
       storage_partition_impl_->GetFileSystemContext(),
       new RendererURLRequestContextSelector(browser_context, GetID()));
 
   channel_->AddFilter(resource_message_filter);
   media::AudioManager* audio_manager = BrowserMainLoop::GetAudioManager();
   MediaStreamManager* media_stream_manager =
       BrowserMainLoop::GetMediaStreamManager();
   channel_->AddFilter(new AudioInputRendererHost(audio_manager,

[Charlie Reis, 2013/06/05 19:07:10]

Does this also need to be behind the if?

[Jeffrey Yasskin, 2013/06/05 21:23:27]

The test passes with it outside the if, likely because I'm not sending any
messages that the AudioInputRendererHost wants to process using its arguments.
It looks like I could actually remove the 'if' entirely by removing the two
DCHECKs in AudioRendererHost()
(https://code.google.com/p/chromium/codesearch/#chromium/src/content/browser/r...),
which would cause it to fail only when a unittest actually sends a message that
needs to go to this filter.

I think it's slightly better to put the 'if' here, since that reduces the
dependencies of the unittest, and a unittest that's trying to test the
AudioRendererHost should probably just instantiate that and send messages
through it directly. The downside is that a test author who expects to send
messages to the RPH and have the AudioRendererHost process them will find their
bug faster if the first message they send crashes, rather than just being
ignored.

                                                  media_stream_manager));
-  channel_->AddFilter(new AudioRendererHost(
-      GetID(), audio_manager, BrowserMainLoop::GetAudioMirroringManager(),
-      media_internals, media_stream_manager));
+  if (audio_manager) {
+    channel_->AddFilter(new AudioRendererHost(
+        GetID(), audio_manager, BrowserMainLoop::GetAudioMirroringManager(),
+        media_internals, media_stream_manager));
+  }
   channel_->AddFilter(new VideoCaptureHost());
   channel_->AddFilter(new AppCacheDispatcherHost(
       storage_partition_impl_->GetAppCacheService(),
       GetID()));
   channel_->AddFilter(new ClipboardMessageFilter);
   channel_->AddFilter(
       new DOMStorageMessageFilter(
           GetID(),
           storage_partition_impl_->GetDOMStorageContext()));
   channel_->AddFilter(
@@ skipping 1055 matching lines @@
 void RenderProcessHostImpl::EndFrameSubscription(int route_id) {
   if (!gpu_message_filter_)
     return;
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
       &GpuMessageFilter::EndFrameSubscription,
       gpu_message_filter_,
       route_id));
 }
 
 void RenderProcessHostImpl::OnShutdownRequest() {
   // Don't shut down if there are more active RenderViews than the one asking

[Charlie Reis, 2013/06/05 19:07:10]

Please update this comment:
s/more active RenderViews than the one asking to close/active RenderViews/

[Jeffrey Yasskin, 2013/06/05 21:23:27]

Thanks, done.

   // to close, or if there are pending RenderViews being swapped back in.
   // In single process mode, we never shutdown the renderer.
   int num_active_views = GetActiveViewCount();
-  if (pending_views_ || num_active_views > 1 || run_renderer_in_process())
+  if (pending_views_ || num_active_views > 0 || run_renderer_in_process())
     return;
 
   // Notify any contents that might have swapped out renderers from this
   // process. They should not attempt to swap them back in.
   NotificationService::current()->Notify(
       NOTIFICATION_RENDERER_PROCESS_CLOSING,
       Source<RenderProcessHost>(this),
       NotificationService::NoDetails());
 
   Send(new ChildProcessMsg_Shutdown());
@@ skipping 94 matching lines @@
       continue;
 
     RenderViewHost* rvh =
         RenderViewHost::From(const_cast<RenderWidgetHost*>(widget));
 
     rvh->UpdateWebkitPreferences(rvh->GetWebkitPreferences());
   }
 }
 
 }  // namespace content