Clear network state only when REMOVE_CACHE is set, not unconditionally

Previously, network state like HSTS data was cleared whenever
BrowsingDataRemover::Remove() was called. Some archaeology
(https://codereview.chromium.org/7717023/) reveals that the original
intention was to clear this state when REMOVE_CACHE was set, but due to
a curly brace mishap, we've been clearing it over-aggressively for
years.

When I changed this behavior to remove network state only when
REMOVE_CACHE is set, it revealed that a number of tests are relying on
state being asynchronously cleared. This is no longer the case, as for
example when only REMOVE_DOWNLOADS is set. This CL fixes that by calling
NotifyIfDone() at the end of RemoveImpl(), to catch cases where no state
is being wiped asynchronously. This is a little weird since download
removal does appear to be async -- but it matches the documentation of
BrowsingDataRemover::Observer, which says that the observer fires when
"keywords have been deleted, cache cleared and all other tasks
scheduled".

BUG=603682

Review-Url: https://codereview.chromium.org/1941073002
Cr-Commit-Position: refs/heads/master@{#391248}
diff --git a/chrome/browser/browsing_data/browsing_data_remover.cc b/chrome/browser/browsing_data/browsing_data_remover.cc
index 2d65f9a..e7d98cd 100644
--- a/chrome/browser/browsing_data/browsing_data_remover.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover.cc
@@ -881,6 +881,14 @@
 
     storage_partition_remove_mask |=
         content::StoragePartition::REMOVE_DATA_MASK_WEBRTC_IDENTITY;
+
+    // When clearing cache, wipe accumulated network related data
+    // (TransportSecurityState and HttpServerPropertiesManager data).
+    waiting_for_clear_networking_history_ = true;
+    profile_->ClearNetworkingHistorySince(
+        delete_begin_,
+        base::Bind(&BrowsingDataRemover::OnClearedNetworkingHistory,
+                   weak_ptr_factory_.GetWeakPtr()));
   }
 
   if (remove_mask & REMOVE_WEBRTC_IDENTITY) {
@@ -958,14 +966,6 @@
   if ((remove_mask & (REMOVE_CACHE | REMOVE_COOKIES)))
     prefs->SetString(omnibox::kZeroSuggestCachedResults, std::string());
 
-  // Always wipe accumulated network related data (TransportSecurityState and
-  // HttpServerPropertiesManager data).
-  waiting_for_clear_networking_history_ = true;
-  profile_->ClearNetworkingHistorySince(
-      delete_begin_,
-      base::Bind(&BrowsingDataRemover::OnClearedNetworkingHistory,
-                 weak_ptr_factory_.GetWeakPtr()));
-
   if (remove_mask & (REMOVE_COOKIES | REMOVE_HISTORY)) {
     domain_reliability::DomainReliabilityService* service =
       domain_reliability::DomainReliabilityServiceFactory::
@@ -1016,6 +1016,9 @@
     choice = ONLY_CACHE;
   }
 
+  // Notify in case all actions taken were synchronous.
+  NotifyIfDone();
+
   UMA_HISTOGRAM_ENUMERATION(
       "History.ClearBrowsingData.UserDeletedCookieOrCache",
       choice, MAX_CHOICE_VALUE);
diff --git a/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc b/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc
index 840d1e2..a70fcd8 100644
--- a/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc
+++ b/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc
@@ -25,7 +25,10 @@
 #include "content/public/common/content_paths.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
+#include "net/http/transport_security_state.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserThread;
@@ -97,6 +100,43 @@
   }
 };
 
+class BrowsingDataRemoverTransportSecurityStateBrowserTest
+    : public BrowsingDataRemoverBrowserTest {
+ public:
+  BrowsingDataRemoverTransportSecurityStateBrowserTest() {}
+
+  void SetUpOnMainThread() override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&BrowsingDataRemoverTransportSecurityStateBrowserTest::
+                       SetUpTransportSecurityState,
+                   this, base::RetainedRef(
+                             browser()->profile()->GetRequestContext())));
+  }
+
+  void CheckTransportSecurityState(
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      bool should_be_cleared) {
+    net::TransportSecurityState* state =
+        context_getter->GetURLRequestContext()->transport_security_state();
+    if (should_be_cleared)
+      EXPECT_FALSE(state->ShouldUpgradeToSSL("example.test"));
+    else
+      EXPECT_TRUE(state->ShouldUpgradeToSSL("example.test"));
+  }
+
+ protected:
+  void SetUpTransportSecurityState(
+      scoped_refptr<net::URLRequestContextGetter> context_getter) {
+    net::TransportSecurityState* state =
+        context_getter->GetURLRequestContext()->transport_security_state();
+    base::Time expiry = base::Time::Now() + base::TimeDelta::FromDays(1000);
+    EXPECT_FALSE(state->ShouldUpgradeToSSL("example.test"));
+    state->AddHSTS("example.test", expiry, false);
+    EXPECT_TRUE(state->ShouldUpgradeToSSL("example.test"));
+  }
+};
+
 // Test BrowsingDataRemover for downloads.
 IN_PROC_BROWSER_TEST_F(BrowsingDataRemoverBrowserTest, Download) {
   DownloadAnItem();
@@ -136,7 +176,33 @@
   RunScriptAndCheckResult("getRecords()", "text2");
 }
 
-// Profile::ClearNetworkingHistorySince should be exercised here too see whether
-// the call gets delegated through ProfileIO[Impl]Data properly, which is hard
-// to write unit-tests for. Currently this is done by both of the above tests.
-// Add standalone test if this changes.
+// Verify that TransportSecurityState data is cleared for REMOVE_CACHE.
+IN_PROC_BROWSER_TEST_F(BrowsingDataRemoverTransportSecurityStateBrowserTest,
+                       ClearTransportSecurityState) {
+  RemoveAndWait(BrowsingDataRemover::REMOVE_CACHE);
+  base::RunLoop run_loop;
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BrowsingDataRemoverTransportSecurityStateBrowserTest::
+                     CheckTransportSecurityState,
+                 this,
+                 base::RetainedRef(browser()->profile()->GetRequestContext()),
+                 true /* should be cleared */),
+      run_loop.QuitClosure());
+}
+
+// Verify that TransportSecurityState data is not cleared if REMOVE_CACHE is not
+// set.
+IN_PROC_BROWSER_TEST_F(BrowsingDataRemoverTransportSecurityStateBrowserTest,
+                       PreserveTransportSecurityState) {
+  RemoveAndWait(BrowsingDataRemover::REMOVE_SITE_DATA);
+  base::RunLoop run_loop;
+  BrowserThread::PostTaskAndReply(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&BrowsingDataRemoverTransportSecurityStateBrowserTest::
+                     CheckTransportSecurityState,
+                 this,
+                 base::RetainedRef(browser()->profile()->GetRequestContext()),
+                 false /* should not be cleared */),
+      run_loop.QuitClosure());
+}