Proposed new approach for SetBackgrounded.

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 528 matching lines @@
     // 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 these calls.
+  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_GE(visible_widgets_, 1);

[gab, 2015/07/08 18:50:56]

DCHECK_GT(visible_wigets, 0); 

slightly better IMO?

+  if (--visible_widgets_ == 0) {
+    DCHECK(!is_process_backgrounded_);
+    UpdateProcessPriority();
   }
 }
 
 int RenderProcessHostImpl::VisibleWidgetCount() const {
   return visible_widgets_;
 }
 
 bool RenderProcessHostImpl::IsForGuestsOnly() const {
   return is_for_guests_only_;
 }
@@ skipping 1009 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() {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();

[gab, 2015/07/08 18:50:56]

const

+  bool process_exists =
+      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 those states. Processes are initially launched with
+  // foreground priority, so when the process doesn't exist.
+  bool backgrounded =

[gab, 2015/07/08 18:50:56]

s/backgrounded/should_background/ ?

+      process_exists && visible_widgets_ == 0 &&
+      !audio_renderer_host_->HasActiveAudio() &&
+      !command_line->HasSwitch(switches::kDisableRendererBackgrounding);
+
+  if (is_process_backgrounded_ == backgrounded)
     return;

[gab, 2015/07/08 18:50:56]

I would just keep this early return, I find it easier to follow then molding it
into the boolean and then checking it independently for early return anyways
later.

[ncarter (slow), 2015/07/08 19:07:19]

Good point.

Though, it's important to actually set is_process_backgrounded_ to false in the
!child_process_launcher case (otherwise, I expect that when the process that was
backgrounded is re-created after crash, we might do the wrong thing). That's why
I structured it as I did. However, I think what we can do is keep the early
return but also assign the is_process_backgrounded_ = false.

 
-  // Don't background processes which have active audio streams.
-  if (backgrounded_ && audio_renderer_host_->HasActiveAudio())
-    return;
+  TRACE_EVENT1("renderer_host", "RenderProcessHostImpl::UpdateProcessPriority",

[gab, 2015/07/08 18:50:56]

I think the trace should remain at the top (it's not filled in UMA so early
returns don't affect it, having a TRACE_EVENT in the middle is unusual IMO).

[ncarter (slow), 2015/07/08 19:07:19]

Because we're now computing |backgrounded| in the function, it has to go after
that computation. My thinking was that skipping the trivial/early return cases
where we don't actually do any work would make the about:tracing results more
interesting.

+               "backgrounded", backgrounded);
+  is_process_backgrounded_ = backgrounded;
 
-  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;
@@ skipping 35 matching lines @@
   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