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 462 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 525 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.

[gab, 2015/08/04 19:40:24]

This comment is now obsolete.

[sebsg, 2015/08/06 21:11:47]

Done.

-  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 these calls.

[gab, 2015/08/04 19:40:24]

s/these calls/this call/

[sebsg, 2015/08/06 21:11:47]

Done.

+  if (visible_widgets_ == 0)
     return;
 
-  DCHECK_EQ(backgrounded_, (visible_widgets_ == 0));
-  visible_widgets_--;
-  DCHECK_GE(visible_widgets_, 0);
-  if (visible_widgets_ == 0) {
-    DCHECK(!backgrounded_);
-    SetBackgrounded(true);
+  DCHECK_GT(visible_widgets_, 0);

[miu, 2015/08/04 20:30:20]

Might as well remove this DCHECK since it can never trigger (due to the
if-statement just above).

[sebsg, 2015/08/06 21:11:47]

Done.

+  if (--visible_widgets_ == 0) {

[gab, 2015/08/04 19:40:24]

In Chromium we typically prefer not having inline self-operators inside
conditionals.

i.e., doing the decrement first then the check on its own would be the prefered
style.

[sebsg, 2015/08/06 21:11:47]

Done.

+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 }
 
 int RenderProcessHostImpl::VisibleWidgetCount() const {
   return visible_widgets_;
 }
 
+void RenderProcessHostImpl::AudioStateChanged() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);

[gab, 2015/08/04 19:40:24]

As Nick said, this class is not thread safe (i.e. all entry points should only
be called on the UI thread and therefore it's overkill to document it here).

(FYI, the way we'd check this typically in Chromium is that we'd have
base::ThreadChecker thread_checker_; member and every method would call
thread_checker_.CalledOnValidThread() to both document the lack of thread safety
and enforce it -- not that you should adopt this paradigm in this CL, just
letting you know how other non-thread-safe classes that do check for thread
safety do it).

[sebsg, 2015/08/06 21:11:47]

Done.

+  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 102 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() {
+  const base::CommandLine* command_line =
+      base::CommandLine::ForCurrentProcess();

[gab, 2015/08/04 19:40:24]

inline this below since you only need it once

[sebsg, 2015/08/06 21:11:47]

Done.

+  bool process_exists =

[gab, 2015/08/04 19:40:24]

const

[sebsg, 2015/08/06 21:11:47]

Done.

+      child_process_launcher_.get() && !child_process_launcher_->IsStarting();
+
+  // 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
+  // foreground priority, so when the process doesn't exist.
+  bool should_background =

[gab, 2015/08/04 19:40:24]

const

[sebsg, 2015/08/06 21:11:47]

Done.

+      process_exists && visible_widgets_ == 0 &&
+      !audio_renderer_host_->HasActiveAudio() &&
+      !command_line->HasSwitch(switches::kDisableRendererBackgrounding);
+
+  if (!child_process_launcher_.get() || child_process_launcher_->IsStarting()) {

[miu, 2015/08/04 20:30:20]

nit: This if-statement should be at the top of the method, since evaluating the
booleans (i.e., |process_exists| and |should_background|) is unnecessary.

[sebsg, 2015/08/06 21:11:47]

Done.

+    is_process_backgrounded_ = false;
+    return;
+  }

[gab, 2015/08/04 19:40:24]

This is essentially:

if (!process_exists) {
  is_process_backgrounded_ = false;
  return;
}

right?

So shouldn't this check go right after initializing |process_exists| and then
there is no need to check whether process_exists later in this method?
In which case we should inline |process_exists| in the if and get rid of that
variable?

[ncarter (slow), 2015/08/04 22:23:23]

The only choice here is whether you want the TRACE_EVENT to log the false->true
transition when the process crashes or not. If you want it to log (which was my
preference, but it's kind of arbitrary) then you should just delete these four
lines. If you don't want to log it, you should do what gab suggests.

[gab, 2015/08/06 16:35:02]

Hmm, won't the TRACE_EVENT code below be hit in the exact same circumstances
whether this check is here or at the top as suggested?

[ncarter (slow), 2015/08/06 18:16:39]

Yes, it will. Sorry my comment wasn't clearer. I was saying that the choice you
wanted to make was whether, in the !process_exists case, to return before or
after the TRACE_EVENT.

It depends on what you want the TRACE_EVENT for. If you're using it to
understand the state transitions of is_process_backgrounded_ over time, it's
probably better to return after TRACE_EVENT. (Remember that process_exists can
transition from true to false and then later back to true). If the TRACE_EVENT
is to log the average runtime expense of
GetModuleHandle/SetProcessBackgrounded(), then it makes sense to not include the
!process_exists case.

Having said that: Looking at the code history, I think this TRACE_EVENT was
concerned with the latter, so the latest patch set is acceptable to me.

[sebsg, 2015/08/06 21:11:47]

Done.

[sebsg, 2015/08/06 21:11:47]

Done.

[sebsg, 2015/08/06 21:11:47]

Done.

[sebsg, 2015/08/06 21:11:47]

Done.

+
+  if (is_process_backgrounded_ == should_background)
     return;
 
-  // Don't background processes which have active audio streams.
-  if (backgrounded_ && audio_renderer_host_->HasActiveAudio())
-    return;
+  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",
+               "should_background", should_background);
+  is_process_backgrounded_ = should_background;
 
-  const base::CommandLine* command_line =
-      base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kDisableRendererBackgrounding))
-    return;
+  if (!process_exists)
+    return;  // Nothing to 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);
 #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