[BrowserSwitcher] Deflake BrowserSwitcherServiceTest

Previously, the tests waited for action_timeout(), to wait for LBS to
load the rules and write some files. As expected, this is fairly flaky,
and it slows down the tests.

Now, `BrowserSwitcherService' exposes a new event for testing, which
fires once LBS has loaded the rules. This should reduce flakes by
listening for the event instead of using an unreliable timeout.

Also rewrote some tests in a synchronous style, which improves
readability.

Bug: 1044619, 1070400
Change-Id: If1a2093cfe3bface73a46896ff2bc95b80ab2f2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2152885
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760531}

chrome/browser/browser_switcher/browser_switcher_service.cc

@@ -239,6 +239,11 @@ void BrowserSwitcherService::Init() {
   StartDownload(fetch_delay());
 }
 
+void BrowserSwitcherService::OnAllRulesetsLoadedForTesting(
+    base::OnceCallback<void()> cb) {
+  all_rulesets_loaded_callback_for_testing_ = std::move(cb);
+}
+
 void BrowserSwitcherService::StartDownload(base::TimeDelta delay) {
   // This destroys the previous XmlDownloader, which cancels any scheduled
   // refresh operations.
@@ -319,6 +324,8 @@ void BrowserSwitcherService::LoadRulesFromPrefs() {
 
 void BrowserSwitcherService::OnAllRulesetsParsed() {
   callback_list_.Notify(this);
+  if (all_rulesets_loaded_callback_for_testing_)
+    std::move(all_rulesets_loaded_callback_for_testing_).Run();
 }
 
 std::unique_ptr<BrowserSwitcherService::CallbackSubscription>

chrome/browser/browser_switcher/browser_switcher_service.h

@@ -124,6 +124,8 @@ class BrowserSwitcherService : public KeyedService {
   explicit BrowserSwitcherService(Profile* profile);
   ~BrowserSwitcherService() override;
 
+  virtual void Init();
+
   // KeyedService:
   void Shutdown() override;
 
@@ -149,7 +151,10 @@ class BrowserSwitcherService : public KeyedService {
   // happens.
   virtual void LoadRulesFromPrefs();
 
-  void Init();
+  // Called after all XML rulesets finished downloading, and the rules are
+  // applied. The XML is downloaded asynchronously, so browser tests use this
+  // event to check that they applied correctly.
+  void OnAllRulesetsLoadedForTesting(base::OnceCallback<void()> callback);
 
  protected:
   virtual void OnAllRulesetsParsed();
@@ -194,6 +199,8 @@ class BrowserSwitcherService : public KeyedService {
   // CallbackList for OnAllRulesetsParsed() listeners.
   base::CallbackList<AllRulesetsParsedCallbackSignature> callback_list_;
 
+  base::OnceCallback<void()> all_rulesets_loaded_callback_for_testing_;
+
   // Per-profile helpers.
   std::unique_ptr<AlternativeBrowserDriver> driver_;
   std::unique_ptr<BrowserSwitcherSitelist> sitelist_;

chrome/browser/browser_switcher/browser_switcher_service_browsertest.cc

@@ -30,8 +30,6 @@
 #include "url/gurl.h"
 
 #if defined(OS_WIN)
-#include "base/files/scoped_temp_dir.h"
-#include "base/path_service.h"
 #include "chrome/browser/browser_switcher/browser_switcher_policy_migrator.h"
 #include "chrome/browser/browser_switcher/browser_switcher_service_win.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -104,14 +102,15 @@ class BrowserSwitcherServiceTest : public InProcessBrowserTest {
         .WillRepeatedly(testing::Return(true));
     policy::BrowserPolicyConnector::SetPolicyProviderForTesting(&provider_);
     BrowserSwitcherService::SetFetchDelayForTesting(base::TimeDelta());
-    BrowserSwitcherService::SetRefreshDelayForTesting(action_timeout() * 3 / 2);
-#if defined(OS_WIN)
-    ASSERT_TRUE(fake_appdata_dir_.CreateUniqueTempDir());
-#endif
+    BrowserSwitcherService::SetRefreshDelayForTesting(base::TimeDelta());
   }
 
   void SetUpOnMainThread() override {
 #if defined(OS_WIN)
+    fake_appdata_dir_ =
+        browser()->profile()->GetPath().AppendASCII("FakeAppData");
+    ASSERT_TRUE(DirectoryExists(fake_appdata_dir_) ||
+                CreateDirectory(fake_appdata_dir_));
     BrowserSwitcherServiceFactory::GetInstance()->SetTestingFactory(
         browser()->profile(),
         base::BindRepeating(
@@ -143,20 +142,48 @@ class BrowserSwitcherServiceTest : public InProcessBrowserTest {
     base::RunLoop().RunUntilIdle();
   }
 
+  void WaitForRefresh() {
+    base::RunLoop run_loop;
+    GetService()->OnAllRulesetsLoadedForTesting(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  void WaitForActionTimeout() {
+    base::RunLoop run_loop;
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, run_loop.QuitClosure(), TestTimeouts::action_timeout());
+    run_loop.Run();
+  }
+
+  BrowserSwitcherService* GetService() {
+    return BrowserSwitcherServiceFactory::GetForBrowserContext(
+        browser()->profile());
+  }
+
   policy::MockConfigurationPolicyProvider& policy_provider() {
     return provider_;
   }
 
-  base::TimeDelta action_timeout() {
-    // Makes the tests a little less slow.
-    return TestTimeouts::action_timeout() / 2;
+#if defined(OS_WIN)
+  BrowserSwitcherServiceWin* GetServiceWin() {
+    return static_cast<BrowserSwitcherServiceWin*>(GetService());
   }
 
-#if defined(OS_WIN)
-  const base::FilePath& appdata_dir() const {
-    return fake_appdata_dir_.GetPath();
+  void WaitForCacheFile() {
+    base::RunLoop run_loop;
+    GetServiceWin()->OnCacheFileUpdatedForTesting(run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  void WaitForSitelistCacheFile() {
+    base::RunLoop run_loop;
+    GetServiceWin()->OnSitelistCacheFileUpdatedForTesting(
+        run_loop.QuitClosure());
+    run_loop.Run();
   }
 
+  const base::FilePath& appdata_dir() const { return fake_appdata_dir_; }
+
   const base::FilePath cache_dir() const {
     return appdata_dir().AppendASCII("Google").AppendASCII("BrowserSwitcher");
   }
@@ -174,7 +201,7 @@ class BrowserSwitcherServiceTest : public InProcessBrowserTest {
   policy::MockConfigurationPolicyProvider provider_;
 
 #if defined(OS_WIN)
-  base::ScopedTempDir fake_appdata_dir_;
+  base::FilePath fake_appdata_dir_;
 #endif
 };
 
@@ -193,18 +220,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, ExternalSitelistInvalidUrl) {
       &fetch_happened));
 
   // Execute everything and make sure we didn't get to the fetch step.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](bool* happened, base::OnceClosure quit) {
-            EXPECT_FALSE(*happened);
-            std::move(quit).Run();
-          },
-          &fetch_happened, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForActionTimeout();
+  EXPECT_FALSE(fetch_happened);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -228,32 +246,17 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       &counter));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service) {
+  auto* service = GetService();
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://yahoo.com/")));
-          },
-          service),
-      action_timeout());
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://yahoo.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout() * 2);
-  run_loop.Run();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -276,31 +279,16 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       &counter));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service) {
+  auto* service = GetService();
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://yahoo.com/")));
-          },
-          service),
-      action_timeout());
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout() * 2);
-  run_loop.Run();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -311,43 +299,16 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
   content::URLLoaderInterceptor interceptor(
       base::BindRepeating(ReturnValidXml));
 
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service,
-             BrowserSwitcherServiceTest* test) {
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            // This will cause the sitelist to be downloaded.
-            test->SetExternalUrl(kAValidUrl);
-          },
-          service, this),
-      action_timeout());
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service,
-             BrowserSwitcherServiceTest* test) {
+  auto* service = GetService();
+
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
+
+  SetExternalUrl(kAValidUrl);
+  WaitForRefresh();
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
-            // This will cause the sitelist to be cleared again.
-            test->SetExternalUrl(kAnInvalidUrl);
-          },
-          service, this),
-      action_timeout() * 2);
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout() * 3);
-  run_loop.Run();
+
+  SetExternalUrl(kAnInvalidUrl);
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, ExternalFileUrl) {
@@ -361,20 +322,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, ExternalFileUrl) {
   SetExternalUrl(net::FilePathToFileURL(sitelist_path).spec());
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -385,21 +337,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(FailToDownload));
 
   // Execute everything and make sure no rules are applied.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -417,18 +359,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       &fetch_happened));
 
   // Execute everything and make sure we didn't get to the fetch step.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](bool* happened, base::OnceClosure quit) {
-            EXPECT_FALSE(*happened);
-            std::move(quit).Run();
-          },
-          &fetch_happened, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForActionTimeout();
+  EXPECT_FALSE(fetch_happened);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -448,22 +381,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(ReturnValidXml));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+  WaitForRefresh();
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://google.com/")));
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://yahoo.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -474,21 +396,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(&ReturnValidXml));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://yahoo.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      TestTimeouts::action_timeout());
-  run_loop.Run();
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -499,11 +411,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(&ReturnValidXml));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
+  auto* service = GetService();
   // No timeout here, since we're checking that the rules get applied *before*
   // downloading.
-  base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://yahoo.com/")));
@@ -526,18 +436,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemSitelistInvalidUrl) {
       &fetch_happened));
 
   // Execute everything and make sure we didn't get to the fetch step.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](bool* happened, base::OnceClosure quit) {
-            EXPECT_FALSE(*happened);
-            std::move(quit).Run();
-          },
-          &fetch_happened, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForActionTimeout();
+  EXPECT_FALSE(fetch_happened);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -549,25 +450,13 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(&ReturnValidXml));
 
   // Execute everything and make sure the rules are applied correctly.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
 }
 
-// Disabled test due to flaky failures on Win 7: crbug.com/1044619
-IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
-                       DISABLED_IeemIgnoresFailedDownload) {
+IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemIgnoresFailedDownload) {
   SetUseIeSitelist(true);
   BrowserSwitcherServiceWin::SetIeemSitelistUrlForTesting(kAValidUrl);
 
@@ -575,21 +464,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(FailToDownload));
 
   // Execute everything and make sure no rules are applied.
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+  auto* service = GetService();
+
+  WaitForRefresh();
   EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemIgnoresNonManagedPref) {
@@ -606,18 +485,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemIgnoresNonManagedPref) {
       &fetch_happened));
 
   // Execute everything and make sure we didn't get to the fetch step.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](bool* happened, base::OnceClosure quit) {
-            EXPECT_FALSE(*happened);
-            std::move(quit).Run();
-          },
-          &fetch_happened, run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForActionTimeout();
+  EXPECT_FALSE(fetch_happened);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemListensForPrefChanges) {
@@ -628,43 +498,15 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, IeemListensForPrefChanges) {
   content::URLLoaderInterceptor interceptor(
       base::BindRepeating(ReturnValidXml));
 
-  auto* service =
-      BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service,
-             BrowserSwitcherServiceTest* test) {
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            // This will cause the sitelist to be downloaded.
-            test->SetUseIeSitelist(true);
-          },
-          service, this),
-      action_timeout());
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service,
-             BrowserSwitcherServiceTest* test) {
+  auto* service = GetService();
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
+
+  SetUseIeSitelist(true);
+  WaitForRefresh();
   EXPECT_TRUE(ShouldSwitch(service, GURL("http://docs.google.com/")));
-            // This will cause the sitelist to be cleared again.
-            test->SetUseIeSitelist(false);
-          },
-          service, this),
-      action_timeout() * 2);
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
-            EXPECT_FALSE(
-                ShouldSwitch(service, GURL("http://docs.google.com/")));
-            std::move(quit).Run();
-          },
-          service, run_loop.QuitClosure()),
-      action_timeout() * 3);
-  run_loop.Run();
+
+  SetUseIeSitelist(false);
+  EXPECT_FALSE(ShouldSwitch(service, GURL("http://docs.google.com/")));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesPrefsToCacheFile) {
@@ -694,17 +536,8 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesPrefsToCacheFile) {
   base::RunLoop().RunUntilIdle();
 
   // Execute everything and check "cache.dat" file contents.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_file_path,
-             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
-            base::ScopedAllowBlockingForTesting allow_blocking;
-            base::File file(cache_file_path,
-                            base::File::FLAG_OPEN | base::File::FLAG_READ);
-            ASSERT_TRUE(file.IsValid());
+  GetService();
+  WaitForCacheFile();
 
   const char expected_output[] =
       "1\n"
@@ -717,20 +550,13 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesPrefsToCacheFile) {
       "1\n"
       "foo.example.com\n";
 
-            std::unique_ptr<char[]> buffer(new char[file.GetLength() + 1]);
-            buffer.get()[file.GetLength()] = '\0';
-            file.Read(0, buffer.get(), file.GetLength());
-            EXPECT_EQ(std::string(expected_output), std::string(buffer.get()));
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  std::string output;
+  EXPECT_TRUE(base::ReadFileToString(cache_file_path(), &output));
+  EXPECT_EQ(expected_output, output);
 
   // Check that sitelistcache.dat doesn't exist.
-            EXPECT_FALSE(base::PathExists(sitelist_cache_file_path));
-
-            std::move(quit).Run();
-          },
-          cache_file_path(), sitelist_cache_file_path(),
-          run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  EXPECT_FALSE(base::PathExists(sitelist_cache_file_path()));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesSitelistsToCacheFile) {
@@ -769,22 +595,17 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesSitelistsToCacheFile) {
 
   // Execute everything and check "cache.dat" file contents. It should
   // contain the *union* of both sitelists, not just one of them.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_file_path,
-             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
-            base::ScopedAllowBlockingForTesting allow_blocking;
-            base::File file(cache_file_path,
-                            base::File::FLAG_OPEN | base::File::FLAG_READ);
-            ASSERT_TRUE(file.IsValid());
+  GetService();
+  // LBS will write to cache.dat twice: once before downloading the XML files,
+  // and then once after. We're interested in the second state, so wait for 2
+  // writes to cache.dat.
+  WaitForCacheFile();
+  WaitForCacheFile();
 
   base::FilePath expected_chrome_path;
   base::FilePath::CharType chrome_path[MAX_PATH];
 #if defined(OS_WIN)
-            ::GetModuleFileName(NULL, chrome_path, ARRAYSIZE(chrome_path));
+  ::GetModuleFileName(nullptr, chrome_path, ARRAYSIZE(chrome_path));
   expected_chrome_path = base::FilePath(chrome_path);
 #endif
   std::string expected_output = base::StringPrintf(
@@ -800,20 +621,12 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesSitelistsToCacheFile) {
       "greylist.invalid.com\n",
       expected_chrome_path.MaybeAsASCII().c_str());
 
-            std::unique_ptr<char[]> buffer(new char[file.GetLength() + 1]);
-            buffer.get()[file.GetLength()] = '\0';
-            file.Read(0, buffer.get(), file.GetLength());
-            EXPECT_EQ(expected_output, std::string(buffer.get()));
+  std::string output;
+  EXPECT_TRUE(base::ReadFileToString(cache_file_path(), &output));
+  EXPECT_EQ(expected_output, output);
 
   // Check that sitelistcache.dat doesn't exist.
-            EXPECT_FALSE(base::PathExists(sitelist_cache_file_path));
-
-            std::move(quit).Run();
-          },
-          cache_file_path(), sitelist_cache_file_path(),
-          run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  EXPECT_FALSE(base::PathExists(sitelist_cache_file_path()));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -825,19 +638,15 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
       base::BindRepeating(&ReturnValidXml));
 
   // Execute everything and check "cache.dat" file contents.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_file_path, base::OnceClosure quit) {
+  GetService();
+  // LBS will write to cache.dat twice: once before downloading the XML files,
+  // and then once after. We're interested in the second state, so wait for 2
+  // writes to cache.dat.
+  WaitForCacheFile();
+  WaitForCacheFile();
+
   base::ScopedAllowBlockingForTesting allow_blocking;
-            ASSERT_TRUE(base::PathExists(base::FilePath(cache_file_path)));
-            std::move(quit).Run();
-          },
-          cache_file_path(), run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  ASSERT_TRUE(base::PathExists(cache_file_path()));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, CacheFileCorrectOnStartup) {
@@ -852,23 +661,18 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, CacheFileCorrectOnStartup) {
       base::BindRepeating(&ReturnValidXml));
 
   // Execute everything and check "cache.dat" file contents.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_file_path,
-             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
+  GetService();
+  WaitForCacheFile();
+
   base::ScopedAllowBlockingForTesting allow_blocking;
-            base::File file(cache_file_path,
+  base::File file(cache_file_path(),
                   base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(base::PathExists(cache_file_path()));
   ASSERT_TRUE(file.IsValid());
   base::FilePath expected_chrome_path;
   base::FilePath::CharType chrome_path[MAX_PATH];
-#if defined(OS_WIN)
-            ::GetModuleFileName(NULL, chrome_path, ARRAYSIZE(chrome_path));
+  ::GetModuleFileName(nullptr, chrome_path, ARRAYSIZE(chrome_path));
   expected_chrome_path = base::FilePath(chrome_path);
-#endif
   std::string expected_output = base::StringPrintf(
       "1\n"
       "\n"
@@ -880,17 +684,9 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, CacheFileCorrectOnStartup) {
       "0\n",
       expected_chrome_path.MaybeAsASCII().c_str());
 
-            std::unique_ptr<char[]> buffer(new char[file.GetLength() + 1]);
-            buffer.get()[file.GetLength()] = '\0';
-            file.Read(0, buffer.get(), file.GetLength());
-            EXPECT_EQ(expected_output, std::string(buffer.get()));
-
-            std::move(quit).Run();
-          },
-          cache_file_path(), sitelist_cache_file_path(),
-          run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  std::string output;
+  EXPECT_TRUE(base::ReadFileToString(cache_file_path(), &output));
+  EXPECT_EQ(expected_output, output);
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -907,43 +703,33 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
   ASSERT_TRUE(base::PathExists(sitelist_cache_file_path()));
 
   // Check that "sitelistcache.dat" got cleaned up on startup.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
-            EXPECT_FALSE(base::PathExists(sitelist_cache_file_path));
-            std::move(quit).Run();
-          },
-          sitelist_cache_file_path(), run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForSitelistCacheFile();
+
+  EXPECT_FALSE(base::PathExists(sitelist_cache_file_path()));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesNothingIfDisabled) {
-  base::ScopedAllowBlockingForTesting allow_blocking;
-
   // No policies configured.
 
   // Check that "cache.dat" and "sitelistcache.dat" don't exist when LBS is not
   // configured.
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_dir, base::FilePath cache_file_path,
-             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
-            EXPECT_FALSE(base::PathExists(cache_dir));
-            EXPECT_FALSE(base::PathExists(cache_file_path));
-            EXPECT_FALSE(base::PathExists(sitelist_cache_file_path));
-            std::move(quit).Run();
-          },
-          cache_dir(), cache_file_path(), sitelist_cache_file_path(),
-          run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  auto* service = GetServiceWin();
+  // Need to initialize both RunLoops at the same time to avoid deadlocks
+  // depending on which is written first (cache.dat or sitelistcache.dat).
+  base::RunLoop cache_run_loop;
+  base::RunLoop sitelist_cache_run_loop;
+  service->OnCacheFileUpdatedForTesting(cache_run_loop.QuitClosure());
+  service->OnSitelistCacheFileUpdatedForTesting(
+      sitelist_cache_run_loop.QuitClosure());
+  cache_run_loop.Run();
+  sitelist_cache_run_loop.Run();
+
+  base::ScopedAllowBlockingForTesting allow_blocking;
+
+  EXPECT_FALSE(base::PathExists(cache_dir()));
+  EXPECT_FALSE(base::PathExists(cache_file_path()));
+  EXPECT_FALSE(base::PathExists(sitelist_cache_file_path()));
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
@@ -973,22 +759,11 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
   ASSERT_TRUE(base::PathExists(cache_file_path()));
   ASSERT_TRUE(base::PathExists(sitelist_cache_file_path()));
 
-  BrowserSwitcherServiceFactory::GetForBrowserContext(browser()->profile());
-  base::RunLoop run_loop;
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(
-          [](base::FilePath cache_dir, base::FilePath cache_file_path,
-             base::FilePath sitelist_cache_file_path, base::OnceClosure quit) {
-            EXPECT_TRUE(base::PathExists(cache_dir));
-            EXPECT_TRUE(base::PathExists(cache_file_path));
-            EXPECT_TRUE(base::PathExists(sitelist_cache_file_path));
-            std::move(quit).Run();
-          },
-          cache_dir(), cache_file_path(), sitelist_cache_file_path(),
-          run_loop.QuitClosure()),
-      action_timeout());
-  run_loop.Run();
+  GetService();
+  WaitForActionTimeout();
+  EXPECT_TRUE(base::PathExists(cache_dir()));
+  EXPECT_TRUE(base::PathExists(cache_file_path()));
+  EXPECT_TRUE(base::PathExists(sitelist_cache_file_path()));
 }
 #endif
 

chrome/browser/browser_switcher/browser_switcher_service_win.cc

@@ -27,6 +27,8 @@
 #include "chrome/browser/browser_switcher/browser_switcher_sitelist.h"
 #include "chrome/browser/browser_switcher/ieem_sitelist_parser.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_registry.h"
 
 namespace browser_switcher {
@@ -136,9 +138,7 @@ BrowserSwitcherServiceWin::BrowserSwitcherServiceWin(
       cache_dir_for_testing_(std::move(cache_dir_for_testing)),
       sequenced_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
-           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {
-  UpdateAllCacheFiles();
-}
+           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {}
 
 BrowserSwitcherServiceWin::~BrowserSwitcherServiceWin() = default;
 
@@ -166,14 +166,16 @@ std::vector<RulesetSource> BrowserSwitcherServiceWin::GetRulesetSources() {
   return sources;
 }
 
+void BrowserSwitcherServiceWin::Init() {
+  BrowserSwitcherService::Init();
+  UpdateAllCacheFiles();
+}
+
 void BrowserSwitcherServiceWin::LoadRulesFromPrefs() {
   BrowserSwitcherService::LoadRulesFromPrefs();
   if (prefs().UseIeSitelist())
     sitelist()->SetIeemSitelist(
         ParsedXml(prefs().GetCachedIeemSitelist(), base::nullopt));
-  if (!prefs().IsEnabled())
-    return;
-  SavePrefsToFile();
 }
 
 base::FilePath BrowserSwitcherServiceWin::GetCacheDir() {
@@ -232,25 +234,51 @@ void BrowserSwitcherServiceWin::OnIeemSitelistParsed(ParsedXml xml) {
   }
 }
 
-void BrowserSwitcherServiceWin::SavePrefsToFile() {
-  DCHECK(prefs().IsEnabled());
+void BrowserSwitcherServiceWin::CacheFileUpdated() {
+  if (cache_file_updated_callback_for_testing_)
+    std::move(cache_file_updated_callback_for_testing_).Run();
+}
+
+void BrowserSwitcherServiceWin::SitelistCacheFileUpdated() {
+  if (sitelist_cache_file_updated_callback_for_testing_)
+    std::move(sitelist_cache_file_updated_callback_for_testing_).Run();
+}
+
+void BrowserSwitcherServiceWin::OnCacheFileUpdatedForTesting(
+    base::OnceClosure cb) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  cache_file_updated_callback_for_testing_ = std::move(cb);
+}
+
+void BrowserSwitcherServiceWin::OnSitelistCacheFileUpdatedForTesting(
+    base::OnceClosure cb) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  sitelist_cache_file_updated_callback_for_testing_ = std::move(cb);
+}
+
+void BrowserSwitcherServiceWin::DeletePrefsFile() {
   base::FilePath path = GetCacheDir();
   if (path.empty())
     return;
   path = path.AppendASCII("cache.dat");
-  sequenced_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&SaveDataToFile, SerializeCacheFile(prefs(), sitelist()),
-                     std::move(path)));
+  sequenced_task_runner_->PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)),
+      base::BindOnce(&BrowserSwitcherServiceWin::CacheFileUpdated,
+                     base::Unretained(this)));
 }
 
-void BrowserSwitcherServiceWin::DeletePrefsFile() {
+void BrowserSwitcherServiceWin::SavePrefsToFile() {
+  DCHECK(prefs().IsEnabled());
   base::FilePath path = GetCacheDir();
   if (path.empty())
     return;
   path = path.AppendASCII("cache.dat");
-  sequenced_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)));
+  sequenced_task_runner_->PostTaskAndReply(
+      FROM_HERE,
+      base::BindOnce(&SaveDataToFile, SerializeCacheFile(prefs(), sitelist()),
+                     std::move(path)),
+      base::BindOnce(&BrowserSwitcherServiceWin::CacheFileUpdated,
+                     base::Unretained(this)));
 }
 
 void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() {
@@ -258,8 +286,10 @@ void BrowserSwitcherServiceWin::DeleteSitelistCacheFile() {
   if (path.empty())
     return;
   path = path.AppendASCII("sitelistcache.dat");
-  sequenced_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)));
+  sequenced_task_runner_->PostTaskAndReply(
+      FROM_HERE, base::BindOnce(&RemoveFile, std::move(path)),
+      base::BindOnce(&BrowserSwitcherServiceWin::SitelistCacheFileUpdated,
+                     base::Unretained(this)));
 }
 
 void BrowserSwitcherServiceWin::UpdateAllCacheFiles() {

chrome/browser/browser_switcher/browser_switcher_service_win.h

@@ -22,6 +22,8 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
       base::FilePath cache_dir_for_testing = base::FilePath());
   ~BrowserSwitcherServiceWin() override;
 
+  void Init() override;
+
   static void SetIeemSitelistUrlForTesting(const std::string& url);
 
   // BrowserSwitcherService:
@@ -29,6 +31,9 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
 
   void LoadRulesFromPrefs() override;
 
+  void OnCacheFileUpdatedForTesting(base::OnceClosure cb);
+  void OnSitelistCacheFileUpdatedForTesting(base::OnceClosure cb);
+
  protected:
   // BrowserSwitcherService:
   void OnAllRulesetsParsed() override;
@@ -59,11 +64,16 @@ class BrowserSwitcherServiceWin : public BrowserSwitcherService {
   // extension, or from a previous Chrome version. Called during initialization.
   void DeleteSitelistCacheFile();
 
+  void CacheFileUpdated();
+  void SitelistCacheFileUpdated();
+
   // Updates or cleans up cache.dat and sitelistcache.dat, based on whether
   // BrowserSwitcher is enabled or disabled.
   void UpdateAllCacheFiles();
 
   base::FilePath cache_dir_for_testing_;
+  base::OnceClosure cache_file_updated_callback_for_testing_;
+  base::OnceClosure sitelist_cache_file_updated_callback_for_testing_;
 
   scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;