[BrowserSwitcher] Refresh sitelists every 30 minutes

Bug: 934992
Change-Id: I1a16b600e4ccbe5d690d0e8a433da97c427e6351
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1504263
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#638197}

chrome/browser/browser_switcher/browser_switcher_service.cc

@@ -30,6 +30,9 @@ namespace {
 // the sitelist fetch.
 const base::TimeDelta kFetchSitelistDelay = base::TimeDelta::FromSeconds(60);
 
+// How long to wait after a fetch to re-fetch the sitelist to keep it fresh.
+const base::TimeDelta kRefreshSitelistDelay = base::TimeDelta::FromMinutes(30);
+
 // How many times to re-try fetching the XML file for the sitelist.
 const int kFetchNumRetries = 1;
 
@@ -80,8 +83,7 @@ RulesetSource::~RulesetSource() = default;
 
 XmlDownloader::XmlDownloader(Profile* profile,
                              std::vector<RulesetSource> sources,
-                             base::OnceCallback<void()> all_done_callback,
-                             base::TimeDelta delay)
+                             base::OnceCallback<void()> all_done_callback)
     : sources_(std::move(sources)),
       all_done_callback_(std::move(all_done_callback)),
       weak_ptr_factory_(this) {
@@ -90,10 +92,7 @@ XmlDownloader::XmlDownloader(Profile* profile,
   other_url_factory_ =
       content::BrowserContext::GetDefaultStoragePartition(profile)
           ->GetURLLoaderFactoryForBrowserProcess();
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&XmlDownloader::FetchXml, weak_ptr_factory_.GetWeakPtr()),
-      delay);
+  FetchXml();
 }
 
 XmlDownloader::~XmlDownloader() = default;
@@ -153,10 +152,11 @@ BrowserSwitcherService::BrowserSwitcherService(Profile* profile)
       driver_(new AlternativeBrowserDriverImpl(&prefs_)),
       sitelist_(new BrowserSwitcherSitelistImpl(&prefs_)),
       weak_ptr_factory_(this) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&BrowserSwitcherService::StartDownload,
-                                weak_ptr_factory_.GetWeakPtr(),
-                                base::Unretained(profile)));
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&BrowserSwitcherService::StartDownload,
+                     weak_ptr_factory_.GetWeakPtr(), base::Unretained(profile)),
+      fetch_delay_);
 }
 
 BrowserSwitcherService::~BrowserSwitcherService() = default;
@@ -167,9 +167,14 @@ void BrowserSwitcherService::StartDownload(Profile* profile) {
     sitelist_downloader_ = std::make_unique<XmlDownloader>(
         profile, std::move(sources),
         base::BindOnce(&BrowserSwitcherService::OnAllRulesetsParsed,
-                       base::Unretained(this)),
-        fetch_delay_);
+                       base::Unretained(this)));
   }
+  // Refresh in 30 minutes, so the sitelists are never too stale.
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&BrowserSwitcherService::StartDownload,
+                     weak_ptr_factory_.GetWeakPtr(), base::Unretained(profile)),
+      refresh_delay_);
 }
 
 void BrowserSwitcherService::Shutdown() {
@@ -225,10 +230,15 @@ void BrowserSwitcherService::OnExternalSitelistParsed(ParsedXml xml) {
 }
 
 base::TimeDelta BrowserSwitcherService::fetch_delay_ = kFetchSitelistDelay;
+base::TimeDelta BrowserSwitcherService::refresh_delay_ = kRefreshSitelistDelay;
 
 // static
 void BrowserSwitcherService::SetFetchDelayForTesting(base::TimeDelta delay) {
   fetch_delay_ = delay;
 }
 
+void BrowserSwitcherService::SetRefreshDelayForTesting(base::TimeDelta delay) {
+  refresh_delay_ = delay;
+}
+
 }  // namespace browser_switcher

chrome/browser/browser_switcher/browser_switcher_service.h

@@ -51,8 +51,7 @@ class XmlDownloader {
   // |all_done_callback| once all the rulesets have been processed.
   XmlDownloader(Profile* profile,
                 std::vector<RulesetSource> sources,
-                base::OnceCallback<void()> all_done_callback,
-                base::TimeDelta delay);
+                base::OnceCallback<void()> all_done_callback);
   virtual ~XmlDownloader();
 
  private:
@@ -93,6 +92,7 @@ class BrowserSwitcherService : public KeyedService {
   void SetSitelistForTesting(std::unique_ptr<BrowserSwitcherSitelist> sitelist);
 
   static void SetFetchDelayForTesting(base::TimeDelta delay);
+  static void SetRefreshDelayForTesting(base::TimeDelta delay);
 
  protected:
   // Return a platform-specific list of URLs to download 1 minute after startup,
@@ -103,6 +103,7 @@ class BrowserSwitcherService : public KeyedService {
 
   // Delay for the IEEM/external XML fetch tasks, launched from the constructor.
   static base::TimeDelta fetch_delay_;
+  static base::TimeDelta refresh_delay_;
 
  private:
   void StartDownload(Profile* profile);

chrome/browser/browser_switcher/browser_switcher_service_browsertest.cc

@@ -46,11 +46,9 @@ const char kSitelistXml[] =
     "<rules version=\"1\"><docMode><domain docMode=\"9\">"
     "docs.google.com</domain></docMode></rules>";
 
-#if defined(OS_WIN)
-const char kOtherSItelistXml[] =
+const char kOtherSitelistXml[] =
     "<rules version=\"1\"><docMode><domain docMode=\"9\">"
     "yahoo.com</domain></docMode></rules>";
-#endif
 
 bool ReturnValidXml(content::URLLoaderInterceptor::RequestParams* params) {
   std::string headers = "HTTP/1.1 200 OK\nContent-Type: text/html\n\n";
@@ -94,6 +92,8 @@ class BrowserSwitcherServiceTest : public InProcessBrowserTest {
         .WillRepeatedly(testing::Return(true));
     policy::BrowserPolicyConnector::SetPolicyProviderForTesting(&provider_);
     BrowserSwitcherService::SetFetchDelayForTesting(base::TimeDelta());
+    BrowserSwitcherService::SetRefreshDelayForTesting(
+        TestTimeouts::action_timeout() * 3 / 2);
 #if defined(OS_WIN)
     ASSERT_TRUE(fake_appdata_dir_.CreateUniqueTempDir());
     base::PathService::Override(base::DIR_LOCAL_APP_DATA,
@@ -170,8 +170,21 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
                        ExternalFetchAndParseAfterStartup) {
   SetExternalUrl(kAValidUrl);
 
-  content::URLLoaderInterceptor interceptor(
-      base::BindRepeating(ReturnValidXml));
+  int counter = 0;
+  content::URLLoaderInterceptor interceptor(base::BindRepeating(
+      [](int* counter, content::URLLoaderInterceptor::RequestParams* params) {
+        if (params->url_request.url.spec() != kAValidUrl)
+          return false;
+        // Return a different sitelist on refresh.
+        const char* sitelist_xml =
+            (*counter == 0) ? kSitelistXml : kOtherSitelistXml;
+        std::string headers = "HTTP/1.1 200 OK\nContent-Type: text/html\n\n";
+        content::URLLoaderInterceptor::WriteResponse(
+            headers, std::string(sitelist_xml), params->client.get());
+        (*counter)++;
+        return true;
+      },
+      &counter));
 
   // Execute everything and make sure the rules are applied correctly.
   auto* service =
@@ -180,13 +193,72 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
   base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
       base::BindOnce(
-          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+          [](BrowserSwitcherService* service) {
             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),
+      TestTimeouts::action_timeout());
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+            EXPECT_FALSE(ShouldSwitch(service, GURL("http://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()),
+      TestTimeouts::action_timeout() * 2);
+  run_loop.Run();
+}
+
+IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest,
+                       ExternalFirstFetchFailsButSecondWorks) {
+  SetExternalUrl(kAValidUrl);
+
+  int counter = 0;
+  content::URLLoaderInterceptor interceptor(base::BindRepeating(
+      [](int* counter, content::URLLoaderInterceptor::RequestParams* params) {
+        if (params->url_request.url.spec() != kAValidUrl)
+          return false;
+        // First request fails, but second succeeds.
+        if (*counter == 0)
+          FailToDownload(params);
+        else
+          ReturnValidXml(params);
+        (*counter)++;
+        return true;
+      },
+      &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) {
+            EXPECT_FALSE(ShouldSwitch(service, GURL("http://google.com/")));
+            EXPECT_FALSE(
+                ShouldSwitch(service, GURL("http://docs.google.com/")));
+            EXPECT_FALSE(ShouldSwitch(service, GURL("http://yahoo.com/")));
+          },
+          service),
       TestTimeouts::action_timeout());
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](BrowserSwitcherService* service, base::OnceClosure quit) {
+            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()),
+      TestTimeouts::action_timeout() * 2);
   run_loop.Run();
 }
 
@@ -458,8 +530,8 @@ IN_PROC_BROWSER_TEST_F(BrowserSwitcherServiceTest, WritesSitelistsToCacheFile) {
 
   base::FilePath external_sitelist_path =
       dir.GetPath().AppendASCII("external_sitelist.xml");
-  base::WriteFile(external_sitelist_path, kOtherSItelistXml,
-                  strlen(kOtherSItelistXml));
+  base::WriteFile(external_sitelist_path, kOtherSitelistXml,
+                  strlen(kOtherSitelistXml));
 
   policy::PolicyMap policies;
   EnableBrowserSwitcher(&policies);