Insulate browser tests from environmental memory pressure.

This CL also makes sure that the default base::MemoryPressureMonitor
won't be created when running browser tests.  This helps insulate the
tests from their environment and ensure that the tests behave
deterministically (rather than flakily depending on things outside the
test control, like the overall memory pressure on the bot which runs the
test).  This is accomplished by avoiding creating the monitor if
--browser-test switch is present.

Before this CL, the --browser-test switch would be injected by
ContentBrowserTest::SetUp.  After this CL, the switch is injected by
BrowserTestBase::SetUp - this means that insulation from memory pressure
covers not only content_browsertests, but also browser_tests (and other
tests that subclass BrowserTestBase rather than ContentBrowserTest).

This CL also makes tweaks in memory-pressure-monitoring code to make
sure that things work properly after the changes described above.
In particular, the CL also makes the following changes:

- Move DummyMemoryPressureMonitor from
  //chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
  into //base (renaming the class into FakeMemoryPressureMonitor), so
  that it can be reused by TabManagerTest.OomPressureListener browser
  test.  Note that the FakeMemoryPressureMonitor has to be constructed
  and present early during browser startup (so that TabManager::Start
  detects presence of the monitor).

- Stop incorrect downcasting that used to be made by
  base::chromeos::MemoryPressureMonitor::Get

There are also some opportunistic changes that are not really required
for correctness:

- Rewrite TabManager::OnMemoryPressure to follow most recent
  style guidelines that help enforce that missing a switch/case
  branch will lead to a compile error (thanks to a missing "default"
  branch).

Bug: 852905
Change-Id: I6eb651a4f7a7bc6d1a2138ba31dd248c7e33f2a4
Reviewed-on: https://chromium-review.googlesource.com/1194879
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590331}

base/memory/fake_memory_pressure_monitor.cc

+// Copyright 2018 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.
+
+#include "base/memory/fake_memory_pressure_monitor.h"
+
+namespace base {
+namespace test {
+
+FakeMemoryPressureMonitor::FakeMemoryPressureMonitor()
+    : MemoryPressureMonitor(),
+      memory_pressure_level_(MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_NONE) {}
+
+FakeMemoryPressureMonitor::~FakeMemoryPressureMonitor() {}
+
+void FakeMemoryPressureMonitor::SetAndNotifyMemoryPressure(
+    MemoryPressureLevel level) {
+  memory_pressure_level_ = level;
+  base::MemoryPressureListener::SimulatePressureNotification(level);
+}
+
+base::MemoryPressureMonitor::MemoryPressureLevel
+FakeMemoryPressureMonitor::GetCurrentPressureLevel() {
+  return memory_pressure_level_;
+}
+
+void FakeMemoryPressureMonitor::SetDispatchCallback(
+    const DispatchCallback& callback) {
+  LOG(ERROR) << "FakeMemoryPressureMonitor::SetDispatchCallback";
+}
+
+}  // namespace test
+}  // namespace base

base/memory/fake_memory_pressure_monitor.h

+// Copyright 2018 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.
+
+#ifndef BASE_MEMORY_FAKE_MEMORY_PRESSURE_MONITOR_H_
+#define BASE_MEMORY_FAKE_MEMORY_PRESSURE_MONITOR_H_
+
+#include "base/macros.h"
+#include "base/memory/memory_pressure_monitor.h"
+
+namespace base {
+namespace test {
+
+class FakeMemoryPressureMonitor : public base::MemoryPressureMonitor {
+ public:
+  FakeMemoryPressureMonitor();
+  ~FakeMemoryPressureMonitor() override;
+
+  void SetAndNotifyMemoryPressure(MemoryPressureLevel level);
+
+  // base::MemoryPressureMonitor overrides:
+  MemoryPressureLevel GetCurrentPressureLevel() override;
+  void SetDispatchCallback(const DispatchCallback& callback) override;
+
+ private:
+  MemoryPressureLevel memory_pressure_level_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeMemoryPressureMonitor);
+};
+
+}  // namespace test
+}  // namespace base
+
+#endif  // BASE_MEMORY_FAKE_MEMORY_PRESSURE_MONITOR_H_

base/memory/memory_pressure_monitor_chromeos.cc

@@ -20,6 +20,9 @@ namespace chromeos {
 
 namespace {
 
+// Type-safe version of |g_monitor| from base/memory/memory_pressure_monitor.cc.
+MemoryPressureMonitor* g_monitor = nullptr;
+
 // The time between memory pressure checks. While under critical pressure, this
 // is also the timer to repeat cleanup attempts.
 const int kMemoryPressureIntervalMs = 1000;
@@ -117,6 +120,9 @@ MemoryPressureMonitor::MemoryPressureMonitor(
       dispatch_callback_(
           base::Bind(&MemoryPressureListener::NotifyMemoryPressure)),
       weak_ptr_factory_(this) {
+  DCHECK(!g_monitor);
+  g_monitor = this;
+
   StartObserving();
   LOG_IF(ERROR,
          base::SysInfo::IsRunningOnChromeOS() && !low_mem_file_.is_valid())
@@ -124,6 +130,9 @@ MemoryPressureMonitor::MemoryPressureMonitor(
 }
 
 MemoryPressureMonitor::~MemoryPressureMonitor() {
+  DCHECK(g_monitor);
+  g_monitor = nullptr;
+
   StopObserving();
 }
 
@@ -140,8 +149,7 @@ MemoryPressureMonitor::GetCurrentPressureLevel() {
 
 // static
 MemoryPressureMonitor* MemoryPressureMonitor::Get() {
-  return static_cast<MemoryPressureMonitor*>(
-      base::MemoryPressureMonitor::Get());
+  return g_monitor;
 }
 
 void MemoryPressureMonitor::StartObserving() {

base/test/BUILD.gn

@@ -26,6 +26,8 @@ static_library("test_config") {
 static_library("test_support") {
   testonly = true
   sources = [
+    "../memory/fake_memory_pressure_monitor.cc",
+    "../memory/fake_memory_pressure_monitor.h",
     "../task/sequence_manager/test/fake_task.cc",
     "../task/sequence_manager/test/fake_task.h",
     "../task/sequence_manager/test/lazy_thread_controller_for_test.cc",

chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc

@@ -11,7 +11,7 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/memory/memory_pressure_monitor.h"
+#include "base/memory/fake_memory_pressure_monitor.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
@@ -112,31 +112,6 @@ class DummyTaskManager : public task_manager::TestTaskManager {
   DISALLOW_COPY_AND_ASSIGN(DummyTaskManager);
 };
 
-class DummyMemoryPressureMonitor : public base::MemoryPressureMonitor {
- public:
-  DummyMemoryPressureMonitor()
-      : MemoryPressureMonitor(),
-        memory_pressure_level_(
-            MemoryPressureLevel::MEMORY_PRESSURE_LEVEL_NONE) {}
-  ~DummyMemoryPressureMonitor() override {}
-
-  void SetAndNotifyMemoryPressure(MemoryPressureLevel level) {
-    memory_pressure_level_ = level;
-    base::MemoryPressureListener::SimulatePressureNotification(level);
-  }
-
-  // base::CriticalMemoryPressureMonitor:
-  MemoryPressureLevel GetCurrentPressureLevel() override {
-    return memory_pressure_level_;
-  }
-  void SetDispatchCallback(const DispatchCallback& callback) override {}
-
- private:
-  MemoryPressureLevel memory_pressure_level_;
-
-  DISALLOW_COPY_AND_ASSIGN(DummyMemoryPressureMonitor);
-};
-
 }  // namespace
 
 class ResourceReporterTest : public testing::Test {
@@ -165,12 +140,12 @@ class ResourceReporterTest : public testing::Test {
     return ResourceReporter::GetInstance();
   }
 
-  DummyMemoryPressureMonitor* monitor() { return &monitor_; }
+  base::test::FakeMemoryPressureMonitor* monitor() { return &monitor_; }
 
  private:
   content::TestBrowserThreadBundle thread_bundle_;
 
-  DummyMemoryPressureMonitor monitor_;
+  base::test::FakeMemoryPressureMonitor monitor_;
 
   DummyTaskManager task_manager_;
 

chrome/browser/resource_coordinator/tab_manager.cc

@@ -533,15 +533,13 @@ void TabManager::OnMemoryPressure(
   // memory pressure signal after it becomes more reliable.
   switch (memory_pressure_level) {
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE:
-      break;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE:
-      break;
+      return;
     case base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL:
       LogMemoryAndDiscardTab(LifecycleUnitDiscardReason::URGENT);
-      break;
-    default:
-      NOTREACHED();
+      return;
   }
+  NOTREACHED();
   // TODO(skuhne): If more memory pressure levels are introduced, consider
   // calling PurgeBrowserMemory() before CRITICAL is reached.
 }

chrome/browser/resource_coordinator/tab_manager_browsertest.cc

@@ -5,6 +5,7 @@
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
+#include "base/memory/fake_memory_pressure_monitor.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -292,6 +293,7 @@ class TabManagerTest : public InProcessBrowserTest {
         GetWebContentsAt(index));
   }
 
+  base::test::FakeMemoryPressureMonitor fake_memory_pressure_monitor_;
   base::SimpleTestTickClock test_clock_;
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_;
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -499,13 +501,13 @@ IN_PROC_BROWSER_TEST_F(TabManagerTest, OomPressureListener) {
   EXPECT_FALSE(IsTabDiscarded(GetWebContentsAt(1)));
 
   // Nothing should happen with a moderate memory pressure event.
-  base::MemoryPressureListener::NotifyMemoryPressure(
+  fake_memory_pressure_monitor_.SetAndNotifyMemoryPressure(
       base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE);
   EXPECT_FALSE(IsTabDiscarded(GetWebContentsAt(0)));
   EXPECT_FALSE(IsTabDiscarded(GetWebContentsAt(1)));
 
   // A critical memory pressure event should discard a tab.
-  base::MemoryPressureListener::NotifyMemoryPressure(
+  fake_memory_pressure_monitor_.SetAndNotifyMemoryPressure(
       base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
   // Coming here, an asynchronous operation will collect system stats. Once in,
   // a tab should get discarded. As such we need to give it 10s time to discard.

content/browser/browser_main_loop.cc

@@ -437,6 +437,31 @@ void SetFileUrlPathAliasForIpcFuzzer() {
 const base::Feature kBrowserResponsivenessCalculator{
     "BrowserResponsivenessCalculator", base::FEATURE_DISABLED_BY_DEFAULT};
 
+std::unique_ptr<base::MemoryPressureMonitor> CreateMemoryPressureMonitor(
+    const base::CommandLine& command_line) {
+  // Behavior of browser tests should not depend on things outside of their
+  // control (like the amount of memory on the system running the tests).
+  if (command_line.HasSwitch(switches::kBrowserTest))
+    return nullptr;
+
+// TODO(chrisha): Simplify this code once MemoryPressureMonitor is made a
+// concrete class.
+#if defined(OS_CHROMEOS)
+  if (chromeos::switches::MemoryPressureHandlingEnabled()) {
+    return std::make_unique<base::chromeos::MemoryPressureMonitor>(
+        chromeos::switches::GetMemoryPressureThresholds());
+  }
+  return nullptr;
+#elif defined(OS_MACOSX)
+  return std::make_unique<base::mac::MemoryPressureMonitor>();
+#elif defined(OS_WIN)
+  return CreateWinMemoryPressureMonitor(command_line);
+#else
+  // No memory monitor on other platforms...
+  return nullptr;
+#endif
+}
+
 }  // namespace
 
 #if defined(USE_X11)
@@ -1498,21 +1523,7 @@ bool BrowserMainLoop::UsingInProcessGpu() const {
 }
 
 void BrowserMainLoop::InitializeMemoryManagementComponent() {
-  // TODO(chrisha): Abstract away this construction mess to a helper function,
-  // once MemoryPressureMonitor is made a concrete class.
-#if defined(OS_CHROMEOS)
-  if (chromeos::switches::MemoryPressureHandlingEnabled()) {
-    memory_pressure_monitor_ =
-        std::make_unique<base::chromeos::MemoryPressureMonitor>(
-            chromeos::switches::GetMemoryPressureThresholds());
-  }
-#elif defined(OS_MACOSX)
-  memory_pressure_monitor_ =
-      std::make_unique<base::mac::MemoryPressureMonitor>();
-#elif defined(OS_WIN)
-  memory_pressure_monitor_ =
-      CreateWinMemoryPressureMonitor(parsed_command_line_);
-#endif
+  memory_pressure_monitor_ = CreateMemoryPressureMonitor(parsed_command_line_);
 
   if (base::FeatureList::IsEnabled(features::kMemoryCoordinator))
     MemoryCoordinatorImpl::GetInstance()->Start();

content/public/common/content_switches.cc

@@ -45,6 +45,11 @@ const char kBrowserStartupDialog[]          = "browser-startup-dialog";
 // Path to the exe to run for the renderer and plugin subprocesses.
 const char kBrowserSubprocessPath[]         = "browser-subprocess-path";
 
+// Tells whether the code is running browser tests (this changes the startup URL
+// used by the content shell and also disables features that can make tests
+// flaky [like monitoring of memory pressure]).
+const char kBrowserTest[] = "browser-test";
+
 // Sets the tile size used by composited layers.
 const char kDefaultTileWidth[]              = "default-tile-width";
 const char kDefaultTileHeight[]             = "default-tile-height";

content/public/common/content_switches.h

@@ -24,6 +24,7 @@ CONTENT_EXPORT extern const char kBlinkSettings[];
 CONTENT_EXPORT extern const char kBrowserCrashTest[];
 CONTENT_EXPORT extern const char kBrowserStartupDialog[];
 CONTENT_EXPORT extern const char kBrowserSubprocessPath[];
+CONTENT_EXPORT extern const char kBrowserTest[];
 CONTENT_EXPORT extern const char kDefaultTileWidth[];
 CONTENT_EXPORT extern const char kDefaultTileHeight[];
 CONTENT_EXPORT extern const char kDisable2dCanvasAntialiasing[];

content/public/test/browser_test_base.cc

@@ -170,6 +170,12 @@ void BrowserTestBase::SetUp() {
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
 
+  // Features that depend on external factors (e.g. memory pressure monitor) can
+  // disable themselves based on the switch below (to ensure that browser tests
+  // behave deterministically / do not flakily change behavior based on external
+  // factors).
+  command_line->AppendSwitch(switches::kBrowserTest);
+
   // Override the child process connection timeout since tests can exceed that
   // when sharded.
   command_line->AppendSwitchASCII(

content/public/test/content_browser_test.cc

@@ -59,8 +59,6 @@ ContentBrowserTest::~ContentBrowserTest() {
 
 void ContentBrowserTest::SetUp() {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  command_line->AppendSwitch(switches::kContentBrowserTest);
-
   SetUpCommandLine(command_line);
 
 #if defined(OS_ANDROID)

content/shell/app/shell_content_main.cc

@@ -6,15 +6,14 @@
 
 #include "build/build_config.h"
 #include "content/public/app/content_main.h"
+#include "content/public/common/content_switches.h"
 #include "content/shell/app/shell_main_delegate.h"
-#include "content/shell/common/shell_switches.h"
 
 #if defined(OS_MACOSX)
 int ContentMain(int argc,
                 const char** argv) {
   bool is_browsertest = false;
-  std::string browser_test_flag(std::string("--") +
-                                switches::kContentBrowserTest);
+  std::string browser_test_flag(std::string("--") + switches::kBrowserTest);
   for (int i = 0; i < argc; ++i) {
     if (browser_test_flag == argv[i]) {
       is_browsertest = true;

content/shell/browser/shell_browser_main_parts.cc

@@ -64,7 +64,7 @@ namespace {
 
 GURL GetStartupURL() {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
-  if (command_line->HasSwitch(switches::kContentBrowserTest))
+  if (command_line->HasSwitch(switches::kBrowserTest))
     return GURL();
   const base::CommandLine::StringVector& args = command_line->GetArgs();
 

content/shell/browser/shell_content_browser_client.cc

@@ -297,8 +297,8 @@ void ShellContentBrowserClient::AppendExtraCommandLineSwitches(
   // content_test_launcher.cc and instead uses shell_main.cc. So give a signal
   // to shell_main.cc that it's a browser test.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kContentBrowserTest)) {
-    command_line->AppendSwitch(switches::kContentBrowserTest);
+          switches::kBrowserTest)) {
+    command_line->AppendSwitch(switches::kBrowserTest);
   }
 #endif
 }

content/shell/common/shell_switches.cc

@@ -10,9 +10,6 @@
 
 namespace switches {
 
-// Tells Content Shell that it's running as a content_browsertest.
-const char kContentBrowserTest[] = "browser-test";
-
 // Makes Content Shell use the given path for its data directory.
 const char kContentShellDataPath[] = "data-path";
 

content/shell/common/shell_switches.h

@@ -12,7 +12,6 @@
 
 namespace switches {
 
-extern const char kContentBrowserTest[];
 extern const char kContentShellDataPath[];
 extern const char kCrashDumpsDir[];
 extern const char kExposeInternalsForTesting[];