Fixed the audio backgrounding bug

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 3471e3c087df2e43716a1bd742defd22d7326c79..b921915bd267eed33fe1702d772f1ca0f5b5a648 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -480,7 +480,7 @@ RenderProcessHostImpl::RenderProcessHostImpl(
       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),
@@ -518,11 +518,6 @@ RenderProcessHostImpl::RenderProcessHostImpl(
   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
@@ -1069,22 +1064,20 @@ void RenderProcessHostImpl::ShutdownForBadMessage() {
 
 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();
   }
 }
 
@@ -1092,6 +1085,11 @@ 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_;
 }
@@ -2114,6 +2112,8 @@ void RenderProcessHostImpl::ProcessDied(bool already_dead,
     delete queued_messages_.front();
     queued_messages_.pop();
   }
+  UpdateProcessPriority();
+  DCHECK(!is_process_backgrounded_);
 
   within_process_died_observer_ = true;
   NotificationService::current()->Notify(
@@ -2236,24 +2236,35 @@ 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.

 
-  // Don't background processes which have active audio streams.
-  if (backgrounded_ && audio_renderer_host_->HasActiveAudio())
+  if (is_process_backgrounded_ == should_background)
     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 (!process_exists)
+    return;  // Nothing to background.
 
 #if defined(OS_WIN)
   // The cbstext.dll loads as a global GetMessage hook in the browser process
@@ -2274,7 +2285,7 @@ void RenderProcessHostImpl::SetBackgrounded(bool backgrounded) {
       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
@@ -2286,7 +2297,7 @@ void RenderProcessHostImpl::SetBackgrounded(bool 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() {
@@ -2309,7 +2320,8 @@ void RenderProcessHostImpl::OnProcessLaunched() {
         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