Fixed the audio backgrounding bug

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 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 477 matching lines @@
     StoragePartitionImpl* storage_partition_impl,
     bool is_for_guests_only)
     : fast_shutdown_started_(false),
       deleting_soon_(false),
 #ifndef NDEBUG
       is_self_deleted_(false),
 #endif
       pending_views_(0),
       mojo_application_host_(new MojoApplicationHost),
       visible_widgets_(0),
-      backgrounded_(true),
+      is_process_backgrounded_(false),
       is_initialized_(false),
       id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
       browser_context_(browser_context),
       storage_partition_impl_(storage_partition_impl),
       sudden_termination_allowed_(true),
       ignore_input_events_(false),
       is_for_guests_only_(is_for_guests_only),
       gpu_observer_registered_(false),
       delayed_cleanup_needed_(false),
       within_process_died_observer_(false),
@@ skipping 17 matching lines @@
   if (!GetBrowserContext()->IsOffTheRecord() &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableGpuShaderDiskCache)) {
     BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                             base::Bind(&CacheShaderInfo, GetID(),
                                        storage_partition_impl_->GetPath()));
   }
   subscribe_uniform_enabled_ =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableSubscribeUniformExtension);
-
-  // Note: When we create the RenderProcessHostImpl, it's technically
-  //       backgrounded, because it has no visible listeners.  But the process
-  //       doesn't actually exist yet, so we'll Background it later, after
-  //       creation.
 }
 
 // static
 void RenderProcessHostImpl::ShutDownInProcessRenderer() {
   DCHECK(g_run_renderer_in_process_);
 
   switch (g_all_hosts.Pointer()->size()) {
     case 0:
       return;
     case 1: {
@@ skipping 531 matching lines @@
     // In single process mode it is better if we don't suicide but just
     // crash.
     CHECK(false);
   }
   // We kill the renderer but don't include a NOTREACHED, because we want the
   // browser to try to survive when it gets illegal messages from the renderer.
   Shutdown(RESULT_CODE_KILLED_BAD_MESSAGE, false);
 }
 
 void RenderProcessHostImpl::WidgetRestored() {
-  // Verify we were properly backgrounded.
-  DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
   visible_widgets_++;
-  SetBackgrounded(false);
+  UpdateProcessPriority();
+  DCHECK(!is_process_backgrounded_);
 }
 
 void RenderProcessHostImpl::WidgetHidden() {
-  // On startup, the browser will call Hide
-  if (backgrounded_)
+  // On startup, the browser will call Hide. We ignore this call.
+  if (visible_widgets_ == 0)
     return;
 
-  DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
-  visible_widgets_--;
-  DCHECK_GE(visible_widgets_, 0);
+  --visible_widgets_;
   if (visible_widgets_ == 0) {
-    DCHECK(!backgrounded_);
-    SetBackgrounded(true);
+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 }
 
 int RenderProcessHostImpl::VisibleWidgetCount() const {
   return visible_widgets_;
 }
 
+void RenderProcessHostImpl::AudioStateChanged() {
+  UpdateProcessPriority();
+}
+
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
 
 StoragePartition* RenderProcessHostImpl::GetStoragePartition() const {
   return storage_partition_impl_;
 }
 
 static void AppendCompositorCommandLineFlags(base::CommandLine* command_line) {
   if (IsPropertyTreeVerificationEnabled())
@@ skipping 1002 matching lines @@
 
   RendererClosedDetails details(status, exit_code);
   mojo_application_host_->WillDestroySoon();
 
   child_process_launcher_.reset();
   channel_.reset();
   while (!queued_messages_.empty()) {
     delete queued_messages_.front();
     queued_messages_.pop();
   }
+  UpdateProcessPriority();
+  DCHECK(!is_process_backgrounded_);
 
   within_process_died_observer_ = true;
   NotificationService::current()->Notify(
       NOTIFICATION_RENDERER_PROCESS_CLOSED,
       Source<RenderProcessHost>(this),
       Details<RendererClosedDetails>(&details));
   FOR_EACH_OBSERVER(RenderProcessHostObserver,
                     observers_,
                     RenderProcessExited(this, status, exit_code));
   within_process_died_observer_ = false;
@@ skipping 97 matching lines @@
 
   mojo_application_host_->WillDestroySoon();
 
   Send(new ChildProcessMsg_Shutdown());
 }
 
 void RenderProcessHostImpl::SuddenTerminationChanged(bool enabled) {
   SetSuddenTerminationAllowed(enabled);
 }
 
-void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) {
-  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::SetBackgrounded",
-               "backgrounded", backgrounded);
-  // Note: we always set the backgrounded_ value.  If the process is NULL
-  // (and hence hasn't been created yet), we will set the process priority
-  // later when we create the process.
-  backgrounded_ = backgrounded;
-  if (!child_process_launcher_.get() || child_process_launcher_->IsStarting())
+void RenderProcessHostImpl::UpdateProcessPriority() {
+  if (!child_process_launcher_.get() || child_process_launcher_->IsStarting()) {
+    is_process_backgrounded_ = false;
+    return;
+  }
+
+  // We background a process as soon as it hosts no active audio streams and no
+  // visible widgets -- the callers must call this function whenever we
+  // transition in/out of thoses states. Processes are initially launched with

[Avi (use Gerrit), 2015/08/19 00:30:29]

"those states"

[sebsg, 2015/08/19 15:22:21]

Done.

+  // foreground priority, so when the process doesn't exist.

[Avi (use Gerrit), 2015/08/19 00:30:29]

"so when the process doesn't exist..." what? Complete that sentence.

[sebsg, 2015/08/19 15:22:21]

Thanks for noticing that, the comment was made when that logic used the process
existence state. Forgot to update the comment when I modified the code and left
it incomplete. 

+  const bool should_background =
+      visible_widgets_ == 0 && !audio_renderer_host_->HasActiveAudio() &&
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableRendererBackgrounding);
+
+  if (is_process_backgrounded_ == should_background)
     return;
 
-  // Don't background processes which have active audio streams.
-  if (backgrounded_ && audio_renderer_host_->HasActiveAudio())
-    return;
-
-  const base::CommandLine* command_line =
-      base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDisableRendererBackgrounding))
-    return;
+  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",
+               "should_background", should_background);
+  is_process_backgrounded_ = should_background;
 
 #if defined(OS_WIN)
   // The cbstext.dll loads as a global GetMessage hook in the browser process
   // and intercepts/unintercepts the kernel32 API SetPriorityClass in a
   // background thread. If the UI thread invokes this API just when it is
   // intercepted the stack is messed up on return from the interceptor
   // which causes random crashes in the browser process. Our hack for now
   // is to not invoke the SetPriorityClass API if the dll is loaded.
   if (GetModuleHandle(L"cbstext.dll"))
     return;
 #endif  // OS_WIN
 
 #if defined(OS_WIN) || defined(OS_MACOSX)
   // Same as below, but bound to an experiment (http://crbug.com/458594 on
   // Windows, http://crbug.com/398103 on the Mac). Enabled by default in the
   // absence of field trials to get coverage on the perf waterfall.
   base::FieldTrial* trial =
       base::FieldTrialList::Find("BackgroundRendererProcesses");
   if (!trial || !base::StartsWith(trial->group_name(), "Disallow",
                                   base::CompareCase::SENSITIVE)) {
-    child_process_launcher_->SetProcessBackgrounded(backgrounded);
+    child_process_launcher_->SetProcessBackgrounded(should_background);
   }
 #else
   // Control the background state from the browser process, otherwise the task
   // telling the renderer to "unbackground" itself may be preempted by other
   // tasks executing at lowered priority ahead of it or simply by not being
   // swiftly scheduled by the OS per the low process priority
   // (http://crbug.com/398103).
-  child_process_launcher_->SetProcessBackgrounded(backgrounded);
+  child_process_launcher_->SetProcessBackgrounded(should_background);
 #endif  // OS_WIN
 
   // Notify the child process of background state.
-  Send(new ChildProcessMsg_SetProcessBackgrounded(backgrounded));
+  Send(new ChildProcessMsg_SetProcessBackgrounded(should_background));
 }
 
 void RenderProcessHostImpl::OnProcessLaunched() {
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/465841
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465841 RenderProcessHostImpl::OnProcessLaunched::Start"));
   // No point doing anything, since this object will be destructed soon.  We
   // especially don't want to send the RENDERER_PROCESS_CREATED notification,
   // since some clients might expect a RENDERER_PROCESS_TERMINATED afterwards to
   // properly cleanup.
   if (deleting_soon_)
     return;
 
   if (child_process_launcher_) {
     // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/465841
     // is fixed.
     tracked_objects::ScopedTracker tracking_profile2(
         FROM_HERE_WITH_EXPLICIT_FUNCTION(
             "465841 RenderProcessHostImpl::OnProcessLaunched::Backgrounded"));
     DCHECK(child_process_launcher_->GetProcess().IsValid());
-    SetBackgrounded(backgrounded_);
+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 
   // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/465841
   // is fixed.
   tracked_objects::ScopedTracker tracking_profile3(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "465841 RenderProcessHostImpl::OnProcessLaunched::Notify"));
   // NOTE: This needs to be before sending queued messages because
   // ExtensionService uses this notification to initialize the renderer process
   // with state that must be there before any JavaScript executes.
@@ skipping 189 matching lines @@
 void RenderProcessHostImpl::GetAudioOutputControllers(
     const GetAudioOutputControllersCallback& callback) const {
   audio_renderer_host()->GetOutputControllers(callback);
 }
 
 BluetoothDispatcherHost* RenderProcessHostImpl::GetBluetoothDispatcherHost() {
   return bluetooth_dispatcher_host_.get();
 }
 
 }  // namespace content