[RenderProcessHost/Desktop] Update process priority on process launch and fix...

[RenderProcessHost/Desktop] Update process priority on process launch and fix navigation priority inversion

This is a second take on
https://chromium-review.googlesource.com/c/chromium/src/+/754299

It's under an experiment to (1) be able to see the side-effects and (2)
make merge to M69 easier. Intentionally kept old (and buggy) behavior
intact under the non-experimental branch for comparison.

This fixes all aspects of crbug.com/560446:
 1) Fixes the OP better than r385608 did.
 2) Fixes the side-effect of r385608 which was to default all tabs
    created backgrounded to run at foreground priority until visiblity
    was toggle on/off again.
 3) Fixes a major priority inversion on foreground navigation which
    could cause them to background their associated renderer while
    navigation (which reintroduced the OP but worse since all background
    tabs were running foregrounded per r385608)...
    See https://crbug.com/560446#c74 for details.

Updated ChildProcessLauncherPriority::is_background() to consistently
reflect backgrounding decision to all callers and renamed
|ChildProcessLauncherPriority::foreground| to
|ChildProcessLauncherPriority::visible| to better reflect that it's a
property but not the resulting foreground/background state.

See in code comment on ShouldBoostPriorityForPendingViews() for more
details.

TBR=haraken@chromium.org (side-effect in third_party/blink)

Bug: 560446
Change-Id: I901b702506a44704b53c007bcdd498fb60824e94
Reviewed-on: https://chromium-review.googlesource.com/1142593
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585918}

chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc

@@ -2,12 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/path_service.h"
 #include "base/process/process.h"
 #include "base/run_loop.h"
 #include "base/test/test_timeouts.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/devtools/devtools_window.h"
@@ -313,23 +317,35 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, MAYBE_ProcessPerTab) {
   EXPECT_EQ(host_count, RenderProcessHostCount());
 }
 
-// We don't change process priorities on Posix because the user lacks the
-// permission to raise a process' priority even after lowering it.
-#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
-IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
-  if (!base::Process::CanBackgroundProcesses()) {
-    LOG(ERROR) << "Can't background processes";
-    return;
+class ChromeRenderProcessHostBackgroundingTest
+    : public ChromeRenderProcessHostTest,
+      public testing::WithParamInterface<std::string> {
+ public:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ChromeRenderProcessHostTest::SetUpCommandLine(command_line);
+
+    command_line->AppendSwitch(switches::kProcessPerTab);
+
+    command_line->AppendSwitchASCII(
+        switches::kForceFieldTrials,
+        "BoostRendererPriorityForPendingViews/" + GetParam());
+  }
+
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
   }
+};
+
+IN_PROC_BROWSER_TEST_P(ChromeRenderProcessHostBackgroundingTest, MultipleTabs) {
+  // This test is invalid on platforms that can't background.
+  if (!base::Process::CanBackgroundProcesses())
+    return;
+
 #if defined(OS_MACOSX)
   base::PortProvider* port_provider =
       content::BrowserChildProcessHost::GetPortProvider();
 #endif  //  defined(OS_MACOSX)
 
-  base::CommandLine& parsed_command_line =
-      *base::CommandLine::ForCurrentProcess();
-  parsed_command_line.AppendSwitch(switches::kProcessPerTab);
-
   // Change the first tab to be the omnibox page (TYPE_WEBUI).
   GURL omnibox(chrome::kChromeUIOmniboxURL);
   ui_test_utils::NavigateToURL(browser(), omnibox);
@@ -369,9 +385,9 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
   EXPECT_TRUE(process1.IsProcessBackgrounded(port_provider));
   EXPECT_FALSE(process2.IsProcessBackgrounded(port_provider));
   // TODO(gab): The new background tab should be backgrounded but it currently
-  // intentionally isn't per a workaround to https://crbug.com/560446 in
-  // RenderProcessHostImpl::OnProcessLaunched().
-  EXPECT_FALSE(process3.IsProcessBackgrounded(port_provider));
+  // only is under an experiment.
+  EXPECT_EQ(process3.IsProcessBackgrounded(port_provider),
+            GetParam() == "Enabled");
 
   // Navigate back to the first page. Its renderer should be in foreground
   // again while the other renderers should be backgrounded.
@@ -380,11 +396,13 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
   EXPECT_FALSE(process1.IsProcessBackgrounded(port_provider));
   EXPECT_TRUE(process2.IsProcessBackgrounded(port_provider));
   // TODO(gab): Same as above.
-  EXPECT_FALSE(process3.IsProcessBackgrounded(port_provider));
+  EXPECT_EQ(process3.IsProcessBackgrounded(port_provider),
+            GetParam() == "Enabled");
 
   // TODO(gab): Remove this when https://crbug.com/560446 is fixed, but for now
-  // confirm that the correct state is at least achieved when tab #3 is
-  // explicitly foregrounded and re-backgrounded.
+  // confirm that the correct state is at least achieved, regardless of the
+  // experiment state, when tab #3 is explicitly foregrounded and
+  // re-backgrounded.
   EXPECT_EQ(process3.Pid(), ShowSingletonTab(page3).Pid());
   EXPECT_EQ(process1.Pid(), ShowSingletonTab(page1).Pid());
   EXPECT_FALSE(process1.IsProcessBackgrounded(port_provider));
@@ -394,9 +412,8 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
   EXPECT_TRUE(process1.IsProcessBackgrounded());
   EXPECT_FALSE(process2.IsProcessBackgrounded());
   // TODO(gab): The new background tab should be backgrounded but it currently
-  // intentionally isn't per a workaround to https://crbug.com/560446 in
-  // RenderProcessHostImpl::OnProcessLaunched().
-  EXPECT_FALSE(process3.IsProcessBackgrounded());
+  // only is under an experiment.
+  EXPECT_EQ(process3.IsProcessBackgrounded(), GetParam() == "Enabled");
 
   // Navigate back to the first page. Its renderer should be in foreground
   // again while the other renderers should be backgrounded.
@@ -405,11 +422,12 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
   EXPECT_FALSE(process1.IsProcessBackgrounded());
   EXPECT_TRUE(process2.IsProcessBackgrounded());
   // TODO(gab): Same as above.
-  EXPECT_FALSE(process3.IsProcessBackgrounded());
+  EXPECT_EQ(process3.IsProcessBackgrounded(), GetParam() == "Enabled");
 
   // TODO(gab): Remove this when https://crbug.com/560446 is fixed, but for now
-  // confirm that the correct state is at least achieved when tab #3 is
-  // explicitly foregrounded and re-backgrounded.
+  // confirm that the correct state is at least achieved, regardless of the
+  // experiment state, when tab #3 is explicitly foregrounded and
+  // re-backgrounded.
   EXPECT_EQ(process3.Pid(), ShowSingletonTab(page3).Pid());
   EXPECT_EQ(process1.Pid(), ShowSingletonTab(page1).Pid());
   EXPECT_FALSE(process1.IsProcessBackgrounded());
@@ -417,7 +435,131 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest, Backgrounding) {
   EXPECT_TRUE(process3.IsProcessBackgrounded());
 #endif
 }
+
+namespace {
+
+// A helper to post a recurring check that a renderer is foregrounded. The
+// recurring check uses WeakPtr semantic and will die when this class goes out
+// of scope.
+class AssertForegroundHelper {
+ public:
+  AssertForegroundHelper() : weak_ptr_factory_(this) {}
+
+#if defined(OS_MACOSX)
+  // Asserts that |renderer_process| isn't backgrounded and reposts self to
+  // check again shortly. |renderer_process| must outlive this
+  // AssertForegroundHelper instance.
+  void AssertForegroundAndRepost(const base::Process& renderer_process,
+                                 base::PortProvider* port_provider) {
+    ASSERT_FALSE(renderer_process.IsProcessBackgrounded(port_provider));
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AssertForegroundHelper::AssertForegroundAndRepost,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       base::ConstRef(renderer_process), port_provider),
+        base::TimeDelta::FromMicroseconds(10));
+  }
+#else   // defined(OS_MACOSX)
+  // Same as above without the Mac specific base::PortProvider.
+  void AssertForegroundAndRepost(const base::Process& renderer_process) {
+    ASSERT_FALSE(renderer_process.IsProcessBackgrounded());
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&AssertForegroundHelper::AssertForegroundAndRepost,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       base::ConstRef(renderer_process)),
+        base::TimeDelta::FromMicroseconds(10));
+  }
+#endif  // defined(OS_MACOSX)
+
+ private:
+  base::WeakPtrFactory<AssertForegroundHelper> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AssertForegroundHelper);
+};
+
+}  // namespace
+
+#if defined(ADDRESS_SANITIZER)
+// TODO(gab): Fix ASAN issue and re-enable this test (landing the related CL and
+// ensuring it worked was very high priority and this test not passing ASAN,
+// while important too, wasn't critical enough to block -- simple investigation
+// was blocked by https://crbug.com/877205).
+#define MAYBE_ForegroundNavigationIsNeverBackgrounded \
+  DISABLED_ForegroundNavigationIsNeverBackgrounded
+#else
+#define MAYBE_ForegroundNavigationIsNeverBackgrounded \
+  ForegroundNavigationIsNeverBackgrounded
+#endif
+
+// This is a regression test for https://crbug.com/560446 (ensures a foreground
+// navigation isn't backgrounded prior to its navigation committing a "visible"
+// widget). TODO(gab): This test should move to
+// content/browser/renderer_host/render_process_host_browsertest.cc after the
+// experiment (there's nothing chrome specific about it really).
+IN_PROC_BROWSER_TEST_P(ChromeRenderProcessHostBackgroundingTest,
+                       MAYBE_ForegroundNavigationIsNeverBackgrounded) {
+#if defined(OS_MACOSX)
+  base::PortProvider* port_provider =
+      content::BrowserChildProcessHost::GetPortProvider();
+#endif  //  defined(OS_MACOSX)
+
+  GURL url(embedded_test_server()->GetURL("/title1.html"));
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  content::TestNavigationManager navigation_manager(web_contents, url);
+
+  content::NavigationController::LoadURLParams load_params(url);
+  web_contents->GetController().LoadURLWithParams(load_params);
+
+  const base::Process& renderer_process =
+      web_contents->GetMainFrame()->GetProcess()->GetProcess();
+  ASSERT_TRUE(renderer_process.IsValid());
+
+  // Kick off an infinite check against self that the tab under navigation is
+  // never backgrounded (the upcoming WaitforResponse() will wait inside a
+  // RunLoop() and hence perform this check regularly throughout the
+  // navigation).
+  AssertForegroundHelper assert_foreground_helper;
+#if defined(OS_MACOSX)
+  assert_foreground_helper.AssertForegroundAndRepost(renderer_process,
+                                                     port_provider);
+#else
+  assert_foreground_helper.AssertForegroundAndRepost(renderer_process);
+#endif
+
+  // Wait until the response (while performing the above check).
+  ASSERT_TRUE(navigation_manager.WaitForResponse());
+
+  // Then perform the check once more after making sure the queues have been
+  // flushed.
+  WaitForLauncherThread();
+  WaitForMessageProcessing(web_contents);
+  ASSERT_EQ(renderer_process.Handle(),
+            web_contents->GetMainFrame()->GetProcess()->GetProcess().Handle());
+#if defined(OS_MACOSX)
+  ASSERT_FALSE(renderer_process.IsProcessBackgrounded(port_provider));
+#else
+  ASSERT_FALSE(renderer_process.IsProcessBackgrounded());
+#endif
+
+  // And same thing until the navigation is complete.
+  navigation_manager.WaitForNavigationFinished();
+  WaitForLauncherThread();
+  WaitForMessageProcessing(web_contents);
+  ASSERT_EQ(renderer_process.Handle(),
+            web_contents->GetMainFrame()->GetProcess()->GetProcess().Handle());
+#if defined(OS_MACOSX)
+  ASSERT_FALSE(renderer_process.IsProcessBackgrounded(port_provider));
+#else
+  ASSERT_FALSE(renderer_process.IsProcessBackgrounded());
 #endif
+}
+
+INSTANTIATE_TEST_CASE_P(BoostRendererPriorityForPendingViews,
+                        ChromeRenderProcessHostBackgroundingTest,
+                        testing::Values("Enabled", "Default"));
 
 // TODO(nasko): crbug.com/173137
 // Disable on Windows and Mac due to ongoing flakiness. (crbug.com/442785)
@@ -599,10 +741,10 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
 // audio in playing state. Also sets up the variables containing the process
 // associated with each tab, the urls of the two pages and the WebContents of
 // the "audio" page.
-class ChromeRenderProcessHostBackgroundingTest
+class ChromeRenderProcessHostBackgroundingTestWithAudio
     : public ChromeRenderProcessHostTest {
  public:
-  ChromeRenderProcessHostBackgroundingTest() {}
+  ChromeRenderProcessHostBackgroundingTestWithAudio() {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ChromeRenderProcessHostTest::SetUpCommandLine(command_line);
@@ -680,12 +822,12 @@ class ChromeRenderProcessHostBackgroundingTest
   base::PortProvider* port_provider_;
 #endif
 
-  DISALLOW_COPY_AND_ASSIGN(ChromeRenderProcessHostBackgroundingTest);
+  DISALLOW_COPY_AND_ASSIGN(ChromeRenderProcessHostBackgroundingTestWithAudio);
 };
 
 // Test to make sure that a process is backgrounded when the audio stops playing
 // from the active tab and there is an immediate tab switch.
-IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTest,
+IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTestWithAudio,
                        ProcessPriorityAfterStoppedAudio) {
   // This test is invalid on platforms that can't background.
   if (!base::Process::CanBackgroundProcesses())
@@ -709,7 +851,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTest,
 
 // Test to make sure that a process is backgrounded automatically when audio
 // stops playing from a hidden tab.
-IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTest,
+IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTestWithAudio,
                        ProcessPriorityAfterAudioStopsOnNotVisibleTab) {
   // This test is invalid on platforms that can't background.
   if (!base::Process::CanBackgroundProcesses())
@@ -730,7 +872,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTest,
 // Test to make sure that a process is un-backgrounded automatically when
 // audio
 // starts playing from a backgrounded tab.
-IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTest,
+IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostBackgroundingTestWithAudio,
                        ProcessPriorityAfterAudioStartsFromBackgroundTab) {
   // This test is invalid on platforms that can't background.
   if (!base::Process::CanBackgroundProcesses())

content/browser/child_process_launcher.cc

@@ -168,7 +168,11 @@ ChildProcessLauncher::Client* ChildProcessLauncher::ReplaceClientForTest(
 
 bool ChildProcessLauncherPriority::operator==(
     const ChildProcessLauncherPriority& other) const {
-  return foreground == other.foreground &&
+  // |should_boost_for_pending_views| is temporary and constant for all
+  // ChildProcessLauncherPriority throughout a session (experiment driven).
+  DCHECK_EQ(should_boost_for_pending_views,
+            other.should_boost_for_pending_views);
+  return visible == other.visible &&
          has_media_stream == other.has_media_stream &&
          frame_depth == other.frame_depth &&
          intersects_viewport == other.intersects_viewport &&

content/browser/child_process_launcher.h

@@ -56,21 +56,23 @@ static_assert(static_cast<int>(LAUNCH_RESULT_START) >
 #endif
 
 struct ChildProcessLauncherPriority {
-  ChildProcessLauncherPriority(bool foreground,
+  ChildProcessLauncherPriority(bool visible,
                                bool has_media_stream,
                                unsigned int frame_depth,
                                bool intersects_viewport,
-                               bool boost_for_pending_views
+                               bool boost_for_pending_views,
+                               bool should_boost_for_pending_views
 #if defined(OS_ANDROID)
                                ,
                                ChildProcessImportance importance
 #endif
                                )
-      : foreground(foreground),
+      : visible(visible),
         has_media_stream(has_media_stream),
         frame_depth(frame_depth),
         intersects_viewport(intersects_viewport),
-        boost_for_pending_views(boost_for_pending_views)
+        boost_for_pending_views(boost_for_pending_views),
+        should_boost_for_pending_views(should_boost_for_pending_views)
 #if defined(OS_ANDROID)
         ,
         importance(importance)
@@ -78,22 +80,59 @@ struct ChildProcessLauncherPriority {
   {
   }
 
-  bool foreground;
+  // Returns true if the child process is backgrounded.
+  bool is_background() const {
+    return !visible && !has_media_stream &&
+           !(should_boost_for_pending_views && boost_for_pending_views);
+  }
+
+  bool operator==(const ChildProcessLauncherPriority& other) const;
+  bool operator!=(const ChildProcessLauncherPriority& other) const {
+    return !(*this == other);
+  }
+
+  // Prefer |is_background()| to inspecting these fields individually (to ensure
+  // all logic uses the same notion of "backgrounded").
+
+  // |visible| is true if the process is responsible for one or more widget(s)
+  // in foreground tabs. The notion of "visible" is determined by the embedder
+  // but is ideally a widget in a non-minimized, non-background, non-occluded
+  // tab (i.e. with pixels visible on the screen).
+  bool visible;
+
+  // |has_media_stream| is true when the process is responsible for "hearable"
+  // content.
   bool has_media_stream;
+
+  // |frame_depth| is the depth of the shallowest frame this process is
+  // responsible for which has |visible| visibility. It only makes sense to
+  // compare this property for two ChildProcessLauncherPriority instances with
+  // matching |visible| properties.
   unsigned int frame_depth;
+
+  // |intersects_viewport| is true if this process is responsible for a frame
+  // which intersects a viewport which has |visible| visibility. It only makes
+  // sense to compare this property for two ChildProcessLauncherPriority
+  // instances with matching |visible| properties.
   bool intersects_viewport;
+
+  // |boost_for_pending_views| is true if this process is responsible for a
+  // pending view (this is used to boost priority of a process responsible for
+  // foreground content which hasn't yet been added as a visible widget -- i.e.
+  // during navigation).
   bool boost_for_pending_views;
+
+  // True iff |boost_for_pending_views| should be considered in
+  // |is_background()|. This needs to be a separate parameter as opposed to
+  // having the experiment set |boost_for_pending_views == false| when
+  // |!should_boost_for_pending_views| as that would result in different
+  // |is_background()| logic than before and defeat the purpose of the
+  // experiment. TODO(gab): Remove this field when the
+  // BoostRendererPriorityForPendingViews desktop experiment is over.
+  bool should_boost_for_pending_views;
 #if defined(OS_ANDROID)
   ChildProcessImportance importance;
 #endif
-
-  // Returns true if the child process is backgrounded.
-  bool is_background() const { return !foreground && !has_media_stream; }
-
-  bool operator==(const ChildProcessLauncherPriority& other) const;
-  bool operator!=(const ChildProcessLauncherPriority& other) const {
-    return !(*this == other);
-  }
 };
 
 // Launches a process asynchronously and notifies the client of the process

content/browser/child_process_launcher_helper_android.cc

@@ -221,7 +221,7 @@ void ChildProcessLauncherHelper::SetProcessPriorityOnLauncherThread(
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
   return Java_ChildProcessLauncherHelperImpl_setPriority(
-      env, java_peer_, process.Handle(), priority.foreground,
+      env, java_peer_, process.Handle(), priority.visible,
       priority.has_media_stream, priority.frame_depth,
       priority.intersects_viewport, priority.boost_for_pending_views,
       static_cast<jint>(priority.importance));

content/browser/renderer_host/render_process_host_impl.cc

@@ -34,6 +34,7 @@
 #include "base/memory/shared_memory.h"
 #include "base/memory/shared_memory_handle.h"
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_base.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/persistent_histogram_allocator.h"
@@ -1141,6 +1142,66 @@ class UnmatchedServiceWorkerProcessTracker
   SiteProcessIDPairSet site_process_set_;
 };
 
+bool ShouldBoostPriorityForPendingViews() {
+#if defined(OS_ANDROID)
+  // On Android, renderer processes with pending views get an extra boost.
+  return true;
+#else
+  // On desktop platforms, new renderer processes have been considered
+  // foreground regardless of visibility since r385608. This is because it was
+  // previously discovered that backgrounding processes that weren't responsible
+  // for visible content resulted in running foreground navigations at
+  // background priority until the main frame was committed (and became a
+  // visible widget)... Thus new processes responsible for hidden content are
+  // now foreground until explicitly made visible and hidden again. Full details
+  // @ https://crbug.com/560446.
+  //
+  // The experiment below will attempt to use the new "pending views" signal to
+  // go back to pre r385608 behavior while still boosting priority of processes
+  // with pending views (until widgets are created and visibility kicks in).
+  // This will however not keep the nice side-effect of r385608 which was to
+  // boost new page loads happening in background tabs (example use case of a
+  // foreground action in a background tab : ctrl+click link of interest and
+  // actively wait for spinner to stop before switching tabs). If we decide that
+  // the latter use case is relevant, we should support it cross-platform.
+  //
+  // Thus, while this is a "boosting" experiment, it's really a "consider the
+  // initial foreground state a boost so we can unboost and background sooner".
+  //
+  // Returning true here triggers the experiment because it will cause the
+  // initial |ChildProcessLauncherPriority::boost_for_pending_views| value to be
+  // |true| which will in turn result in a call to UpdateProcessPriority() when
+  // RemovePendingView() is invoked (without the experiment the initial state
+  // has no pending view and although UpdateProcessPriority() is invoked from
+  // RemovePendingView(), it no-ops per "no change" compared to the initial
+  // ChildProcessLauncherPriority).
+  //
+  // Furthermore, it has been discovered that there is a priority inversion in
+  // foreground navigation without this experiment, the reason being that
+  // ChildProcessLauncherPriority::operator==() notices a difference in
+  // UpdateProcessPriority() when only |boost_for_pending_views| has changed. On
+  // desktop it would start in the |false| state and AddPendingView() would
+  // toggle it. This would result in UpdateProcessPriority() but desktop not
+  // handling the presence of |boost_for_pending_views|, would look at |!visible
+  // && !has_media_stream| and decide that it's time to background (bringing
+  // back the OP of https://crbug.com/560446 but worse because actual background
+  // tabs are running as foreground per r385608 -- so during a big session
+  // restore it's possible to end up with all processes foreground except for
+  // the foreground tab's process...). Full details @
+  // https://crbug.com/560446#c74.
+  //
+  // Hence this experiment is definitely the desired behavior, running it as an
+  // experiment on Canary merely to see what was impacted by existing bugs.
+  // TODO(gab): End the experiment and turn ShouldBoostPriorityForPendingViews()
+  // into a constant when results are in.
+  static bool should_boost_for_pending_views =
+      base::StartsWith(base::FieldTrialList::FindFullName(
+                           "BoostRendererPriorityForPendingViews"),
+                       "Enabled", base::CompareCase::SENSITIVE);
+  return should_boost_for_pending_views;
+#endif
+}
+
 void CopyFeatureSwitch(const base::CommandLine& src,
                        base::CommandLine* dest,
                        const char* switch_name) {
@@ -1395,7 +1456,24 @@ RenderProcessHostImpl::RenderProcessHostImpl(
                 false /* has_media_stream */,
                 frame_depth_,
                 false /* intersects_viewport */,
-                blink::kLaunchingProcessIsBoostedForPendingView
+                ShouldBoostPriorityForPendingViews(),
+#if defined(OS_ANDROID)
+                // Only use |boost_for_pending_views| to infer is_background()
+                // on non-Android platforms for now to avoid changing the
+                // old behavior while the experiment is under way.
+                // Without this, the following tests were failing on Android
+                // (they assume that toggling WidgetHidden() is sufficient to
+                // toggle backgrounding):
+                //   NavigationControllerBrowserTest.
+                //     NoDialogsFromSwappedOutFrames
+                //   SitePerProcessBrowserTest.
+                //     CommitTimeoutForHungRenderer
+                //     HiddenOOPIFWillNotGenerateCompositorFrames
+                // TODO(gab): Clean this up as soon as the experiment is over.
+                false
+#else
+                ShouldBoostPriorityForPendingViews()
+#endif
 #if defined(OS_ANDROID)
                 ,
                 ChildProcessImportance::NORMAL
@@ -3321,14 +3399,16 @@ void RenderProcessHostImpl::Cleanup() {
 }
 
 void RenderProcessHostImpl::AddPendingView() {
-  pending_views_++;
-  UpdateProcessPriority();
+  const bool had_pending_views = pending_views_++;
+  if (!had_pending_views)
+    UpdateProcessPriority();
 }
 
 void RenderProcessHostImpl::RemovePendingView() {
   DCHECK(pending_views_);
-  pending_views_--;
-  UpdateProcessPriority();
+  --pending_views_;
+  if (!pending_views_)
+    UpdateProcessPriority();
 }
 
 void RenderProcessHostImpl::AddWidget(RenderWidgetHost* widget) {
@@ -4090,14 +4170,13 @@ void RenderProcessHostImpl::UpdateProcessPriorityInputs() {
   }
 
   bool inputs_changed = new_visible_widgets_count != visible_clients_;
-#if defined(OS_ANDROID)
-  // OS_ANDROID in order to maintain the workaround on desktop to avoid
-  // backgrounding a new process. See the comment in OnProcessLaunched and
-  // https://crbug.com/560446. Only android uses frame_depth for now, so
-  // not a huge change.
-  inputs_changed = inputs_changed || frame_depth_ != new_frame_depth ||
-                   intersects_viewport_ != new_intersects_viewport;
-#endif
+  // Hide this update behind the ShouldBoostPriorityForPendingViews() experiment
+  // at the moment to avoid causing an undesired early UpdateProcessPriority().
+  // See the comment in OnProcessLaunched() and https://crbug.com/560446.
+  if (ShouldBoostPriorityForPendingViews()) {
+    inputs_changed = inputs_changed || frame_depth_ != new_frame_depth ||
+                     intersects_viewport_ != new_intersects_viewport;
+  }
   visible_clients_ = new_visible_widgets_count;
   frame_depth_ = new_frame_depth;
   intersects_viewport_ = new_intersects_viewport;
@@ -4113,21 +4192,26 @@ void RenderProcessHostImpl::UpdateProcessPriorityInputs() {
 void RenderProcessHostImpl::UpdateProcessPriority() {
   if (!run_renderer_in_process() && (!child_process_launcher_.get() ||
                                      child_process_launcher_->IsStarting())) {
-    priority_.foreground = !blink::kLaunchingProcessIsBackgrounded;
-    priority_.boost_for_pending_views =
-        blink::kLaunchingProcessIsBoostedForPendingView;
+    // This path can be hit early (no-op) or on ProcessDied(). Reset |priority_|
+    // to defaults in case this RenderProcessHostImpl is re-used.
+    priority_.visible = !blink::kLaunchingProcessIsBackgrounded;
+    priority_.boost_for_pending_views = ShouldBoostPriorityForPendingViews();
     return;
   }
 
   const ChildProcessLauncherPriority priority(
-      // We consider a process in foreground if it hosts no visible widgets --
-      // the callers must call this function whenever we transition in/out of
-      // those states.
       visible_clients_ > 0 || base::CommandLine::ForCurrentProcess()->HasSwitch(
                                   switches::kDisableRendererBackgrounding),
       media_stream_count_ > 0, frame_depth_, intersects_viewport_,
-      // boost_for_pending_views
-      !!pending_views_
+      !!pending_views_ /* boost_for_pending_views */,
+#if defined(OS_ANDROID)
+      // Same hack as in RenderProcessHostImpl::RenderProcessHostImpl.
+      // TODO(gab): Clean this up after ShouldBoostPriorityForPendingViews()
+      // experiment.
+      false
+#else
+      ShouldBoostPriorityForPendingViews()
+#endif
 #if defined(OS_ANDROID)
       ,
       GetEffectiveImportance()
@@ -4166,11 +4250,9 @@ void RenderProcessHostImpl::UpdateProcessPriority() {
     child_process_launcher_->SetProcessPriority(priority_);
   }
 
-  // Notify the child process of background state. Note
-  // |priority_.boost_for_pending_views| state is not sent to renderer simply
-  // due to lack of need.
+  // Notify the child process of background state.
   if (should_background_changed) {
-    GetRendererInterface()->SetProcessBackgrounded(priority.is_background());
+    GetRendererInterface()->SetProcessBackgrounded(priority_.is_background());
   }
 }
 
@@ -4184,8 +4266,9 @@ void RenderProcessHostImpl::OnProcessLaunched() {
 
   if (child_process_launcher_) {
     DCHECK(child_process_launcher_->GetProcess().IsValid());
-    DCHECK_EQ(blink::kLaunchingProcessIsBackgrounded,
-              priority_.is_background());
+    // TODO(https://crbug.com/875933): This should be based on
+    // |priority_.is_background()|, see similar check below.
+    DCHECK_EQ(blink::kLaunchingProcessIsBackgrounded, !priority_.visible);
 
     // Unpause the channel now that the process is launched. We don't flush it
     // yet to ensure that any initialization messages sent here (e.g., things
@@ -4199,32 +4282,28 @@ void RenderProcessHostImpl::OnProcessLaunched() {
     }
 
 // Not all platforms launch processes in the same backgrounded state. Make
-// sure |priority_.foreground| reflects this platform's initial process
+// sure |priority_.visible| reflects this platform's initial process
 // state.
 #if defined(OS_MACOSX)
-    priority_.foreground =
+    priority_.visible =
         !child_process_launcher_->GetProcess().IsProcessBackgrounded(
             MachBroker::GetInstance());
 #elif defined(OS_ANDROID)
     // Android child process priority works differently and cannot be queried
     // directly from base::Process.
-    DCHECK_EQ(blink::kLaunchingProcessIsBackgrounded,
-              priority_.is_background());
+    // TODO(https://crbug.com/875933): Fix initial priority on Android to
+    // reflect |priority_.is_background()|.
+    DCHECK_EQ(blink::kLaunchingProcessIsBackgrounded, !priority_.visible);
 #else
-    priority_.foreground =
+    priority_.visible =
         !child_process_launcher_->GetProcess().IsProcessBackgrounded();
 #endif  // defined(OS_MACOSX)
 
-    // Disable updating process priority on startup on desktop platforms for now
-    // as it incorrectly results in backgrounding foreground navigations until
-    // their first commit is made. A better long term solution would be to be
-    // aware of the tab's visibility at this point. https://crbug.com/560446.
-    // This is still needed on Android which uses
-    // |priority_.boost_for_pending_views| and requires RenderProcessHostImpl to
-    // propagate priority changes immediately to ChildProcessLauncher.
-#if defined(OS_ANDROID)
-    UpdateProcessPriority();
-#endif
+    // Only update the priority on startup if boosting is enabled (to avoid
+    // reintroducing https://crbug.com/560446#c13 while pending views only
+    // experimentally result in a boost).
+    if (priority_.boost_for_pending_views)
+      UpdateProcessPriority();
 
     // Share histograms between the renderer and this process.
     CreateSharedRendererHistogramAllocator();

content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncherHelperImpl.java

@@ -136,9 +136,8 @@ public final class ChildProcessLauncherHelperImpl {
 
                     sLauncherByPid.put(pid, ChildProcessLauncherHelperImpl.this);
                     if (mRanking != null) {
-                        mRanking.addConnection(connection, false /* foreground */,
-                                1 /* frameDepth */, false /* intersectsViewport */,
-                                ChildProcessImportance.MODERATE);
+                        mRanking.addConnection(connection, false /* visible */, 1 /* frameDepth */,
+                                false /* intersectsViewport */, ChildProcessImportance.MODERATE);
                     }
 
                     // If the connection fails and pid == 0, the Java-side cleanup was already
@@ -172,7 +171,7 @@ public final class ChildProcessLauncherHelperImpl {
     // This is the current computed importance from all the inputs from setPriority.
     // The initial value is MODERATE since a newly created connection has moderate bindings.
     private @ChildProcessImportance int mEffectiveImportance = ChildProcessImportance.MODERATE;
-    private boolean mForeground;
+    private boolean mVisible;
 
     @CalledByNative
     private static FileDescriptorInfo makeFdInfo(
@@ -435,7 +434,7 @@ public final class ChildProcessLauncherHelperImpl {
     }
 
     @CalledByNative
-    private void setPriority(int pid, boolean foreground, boolean hasMediaStream, long frameDepth,
+    private void setPriority(int pid, boolean visible, boolean hasMediaStream, long frameDepth,
             boolean intersectsViewport, boolean boostForPendingViews,
             @ChildProcessImportance int importance) {
         assert LauncherThread.runningOnLauncherThread();
@@ -447,7 +446,7 @@ public final class ChildProcessLauncherHelperImpl {
 
         ChildProcessConnection connection = mLauncher.getConnection();
         if (ChildProcessCreationParamsImpl.getIgnoreVisibilityForImportance()) {
-            foreground = false;
+            visible = false;
             boostForPendingViews = false;
         }
 
@@ -456,10 +455,10 @@ public final class ChildProcessLauncherHelperImpl {
 
         @ChildProcessImportance
         int newEffectiveImportance;
-        if ((foreground && frameDepth == 0) || importance == ChildProcessImportance.IMPORTANT
+        if ((visible && frameDepth == 0) || importance == ChildProcessImportance.IMPORTANT
                 || (hasMediaStream && !mediaRendererHasModerate)) {
             newEffectiveImportance = ChildProcessImportance.IMPORTANT;
-        } else if ((foreground && frameDepth > 0 && intersectsViewport) || boostForPendingViews
+        } else if ((visible && frameDepth > 0 && intersectsViewport) || boostForPendingViews
                 || importance == ChildProcessImportance.MODERATE
                 || (hasMediaStream && mediaRendererHasModerate)) {
             newEffectiveImportance = ChildProcessImportance.MODERATE;
@@ -468,13 +467,13 @@ public final class ChildProcessLauncherHelperImpl {
         }
 
         // Add first and remove second.
-        if (foreground && !mForeground) {
+        if (visible && !mVisible) {
             BindingManager manager = getBindingManager();
             if (mUseBindingManager && manager != null) {
                 manager.increaseRecency(connection);
             }
         }
-        mForeground = foreground;
+        mVisible = visible;
 
         if (mEffectiveImportance != newEffectiveImportance) {
             switch (newEffectiveImportance) {
@@ -497,7 +496,7 @@ public final class ChildProcessLauncherHelperImpl {
 
         if (mRanking != null) {
             mRanking.updateConnection(
-                    connection, foreground, frameDepth, intersectsViewport, importance);
+                    connection, visible, frameDepth, intersectsViewport, importance);
         }
 
         if (mEffectiveImportance != newEffectiveImportance) {

content/public/android/java/src/org/chromium/content/browser/ChildProcessRanking.java

@@ -19,17 +19,17 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         public final ChildProcessConnection connection;
 
         // Info for ranking a connection.
-        public boolean foreground;
+        public boolean visible;
         public long frameDepth;
         public boolean intersectsViewport;
         @ChildProcessImportance
         public int importance;
 
-        public ConnectionWithRank(ChildProcessConnection connection, boolean foreground,
+        public ConnectionWithRank(ChildProcessConnection connection, boolean visible,
                 long frameDepth, boolean intersectsViewport,
                 @ChildProcessImportance int importance) {
             this.connection = connection;
-            this.foreground = foreground;
+            this.visible = visible;
             this.frameDepth = frameDepth;
             this.intersectsViewport = intersectsViewport;
             this.importance = importance;
@@ -52,16 +52,16 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
             assert o2 != null;
 
             // Ranking order:
-            // * foreground or ChildProcessImportance.IMPORTANT
+            // * visible or ChildProcessImportance.IMPORTANT
             // * ChildProcessImportance.MODERATE
             // * intersectsViewport
             // * frameDepth (lower value is higher rank)
             // Note boostForPendingViews is not used for ranking.
 
             boolean o1IsForeground =
-                    o1.foreground || o1.importance == ChildProcessImportance.IMPORTANT;
+                    o1.visible || o1.importance == ChildProcessImportance.IMPORTANT;
             boolean o2IsForeground =
-                    o2.foreground || o2.importance == ChildProcessImportance.IMPORTANT;
+                    o2.visible || o2.importance == ChildProcessImportance.IMPORTANT;
 
             if (o1IsForeground && !o2IsForeground) {
                 return -1;
@@ -133,13 +133,13 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         return new ReverseRankIterator();
     }
 
-    public void addConnection(ChildProcessConnection connection, boolean foreground,
-            long frameDepth, boolean intersectsViewport, @ChildProcessImportance int importance) {
+    public void addConnection(ChildProcessConnection connection, boolean visible, long frameDepth,
+            boolean intersectsViewport, @ChildProcessImportance int importance) {
         assert connection != null;
         assert indexOf(connection) == -1;
         assert mSize < mRankings.length;
         mRankings[mSize] = new ConnectionWithRank(
-                connection, foreground, frameDepth, intersectsViewport, importance);
+                connection, visible, frameDepth, intersectsViewport, importance);
         mSize++;
         sort();
     }
@@ -156,14 +156,14 @@ public class ChildProcessRanking implements Iterable<ChildProcessConnection> {
         mSize--;
     }
 
-    public void updateConnection(ChildProcessConnection connection, boolean foreground,
+    public void updateConnection(ChildProcessConnection connection, boolean visible,
             long frameDepth, boolean intersectsViewport, @ChildProcessImportance int importance) {
         assert connection != null;
         assert mSize > 0;
         int i = indexOf(connection);
         assert i != -1;
 
-        mRankings[i].foreground = foreground;
+        mRankings[i].visible = visible;
         mRankings[i].frameDepth = frameDepth;
         mRankings[i].intersectsViewport = intersectsViewport;
         mRankings[i].importance = importance;

third_party/blink/public/common/page/launching_process_state.h

@@ -15,10 +15,8 @@ namespace blink {
 #if defined(OS_ANDROID)
 // This matches Android's ChildProcessConnection state before OnProcessLaunched.
 constexpr bool kLaunchingProcessIsBackgrounded = true;
-constexpr bool kLaunchingProcessIsBoostedForPendingView = true;
 #else
 constexpr bool kLaunchingProcessIsBackgrounded = false;
-constexpr bool kLaunchingProcessIsBoostedForPendingView = false;
 #endif
 
 }  // namespace blink