Clean up expired verdicts when PasswordProtectionService is being destructed...

Clean up expired verdicts when PasswordProtectionService is being destructed (typically, that's during browser shutdown).

BUG=719002

Review-Url: https://codereview.chromium.org/2870193002
Cr-Commit-Position: refs/heads/master@{#471000}

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -44,6 +44,7 @@ ChromePasswordProtectionService::ChromePasswordProtectionService(
 
 ChromePasswordProtectionService::~ChromePasswordProtectionService() {
   if (content_settings()) {
+    CleanUpExpiredVerdicts();
     UMA_HISTOGRAM_COUNTS_1000(
         "PasswordProtection.NumberOfCachedVerdictBeforeShutdown",
         GetStoredVerdictCount());

components/safe_browsing/password_protection/password_protection_service.cc

@@ -201,6 +201,62 @@ void PasswordProtectionService::CacheVerdict(
       std::string(), std::move(verdict_dictionary));
 }
 
+void PasswordProtectionService::CleanUpExpiredVerdicts() {
+  DCHECK(content_settings_);
+  if (GetStoredVerdictCount() <= 0)
+    return;
+
+  ContentSettingsForOneType password_protection_settings;
+  content_settings_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+
+  for (const ContentSettingPatternSource& source :
+       password_protection_settings) {
+    GURL primary_pattern_url = GURL(source.primary_pattern.ToString());
+    // Find all verdicts associated with this origin.
+    std::unique_ptr<base::DictionaryValue> verdict_dictionary =
+        base::DictionaryValue::From(content_settings_->GetWebsiteSetting(
+            primary_pattern_url, GURL(),
+            CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(), nullptr));
+    std::vector<std::string> expired_keys;
+    for (base::DictionaryValue::Iterator it(*verdict_dictionary.get());
+         !it.IsAtEnd(); it.Advance()) {
+      base::DictionaryValue* verdict_entry = nullptr;
+      CHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion(
+          it.key(), &verdict_entry));
+      int verdict_received_time;
+      LoginReputationClientResponse verdict;
+      CHECK(ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict));
+
+      if (IsCacheExpired(verdict_received_time, verdict.cache_duration_sec())) {
+        // Since DictionaryValue::Iterator cannot be used to modify the
+        // dictionary, we record the keys of expired verdicts in |expired_keys|
+        // and remove them in the next for-loop.
+        expired_keys.push_back(it.key());
+      }
+    }
+
+    for (const std::string& key : expired_keys) {
+      verdict_dictionary->RemoveWithoutPathExpansion(key, nullptr);
+      stored_verdict_count_--;
+    }
+
+    if (verdict_dictionary->size() == 0u) {
+      content_settings_->ClearSettingsForOneTypeWithPredicate(
+          CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, base::Time(),
+          base::Bind(&OriginMatchPrimaryPattern, primary_pattern_url));
+    } else if (expired_keys.size() > 0u) {
+      // Set the website setting of this origin with the updated
+      // |verdict_diectionary|.
+      content_settings_->SetWebsiteSettingDefaultScope(
+          primary_pattern_url, GURL(),
+          CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+          std::move(verdict_dictionary));
+    }
+  }
+}
+
 void PasswordProtectionService::StartRequest(
     const GURL& main_frame_url,
     const GURL& password_form_action,

components/safe_browsing/password_protection/password_protection_service.h

@@ -71,6 +71,9 @@ class PasswordProtectionService : public history::HistoryServiceObserver {
                     LoginReputationClientResponse* verdict,
                     const base::Time& receive_time);
 
+  // Removes all the expired verdicts from cache.
+  void CleanUpExpiredVerdicts();
+
   // Creates an instance of PasswordProtectionRequest and call Start() on that
   // instance. This function also insert this request object in |requests_| for
   // record keeping.
@@ -150,7 +153,9 @@ class PasswordProtectionService : public history::HistoryServiceObserver {
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
                            TestPathVariantsMatchCacheExpression);
   FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
-                           TestCleanUpCachedVerdicts);
+                           TestRemoveCachedVerdictOnURLsDeleted);
+  FRIEND_TEST_ALL_PREFIXES(PasswordProtectionServiceTest,
+                           TestCleanUpExpiredVerdict);
 
   // Overridden from history::HistoryServiceObserver.
   void OnURLsDeleted(history::HistoryService* history_service,

components/safe_browsing/password_protection/password_protection_service_unittest.cc

@@ -367,7 +367,7 @@ TEST_F(PasswordProtectionServiceTest, TestGetCachedVerdicts) {
                 GURL("http://test.com/def/ghi/index.html"), &actual_verdict));
 }
 
-TEST_F(PasswordProtectionServiceTest, TestCleanUpCachedVerdicts) {
+TEST_F(PasswordProtectionServiceTest, TestRemoveCachedVerdictOnURLsDeleted) {
   ASSERT_EQ(0U, GetStoredVerdictCount());
   // Prepare 2 verdicts. One is for origin "http://foo.com", and the other is
   // for "http://bar.com".
@@ -403,6 +403,7 @@ TEST_F(PasswordProtectionServiceTest, TestCleanUpCachedVerdicts) {
       true /* all_history */, history::URLRows());
   EXPECT_EQ(0U, GetStoredVerdictCount());
 }
+
 TEST_F(PasswordProtectionServiceTest,
        TestNoRequestCreatedIfMainFrameURLIsNotValid) {
   ASSERT_EQ(0u, password_protection_service_->GetPendingRequestsCount());
@@ -538,4 +539,43 @@ TEST_F(PasswordProtectionServiceTest, TestTearDownWithPendingRequests) {
   EXPECT_THAT(histograms_.GetAllSamples(kRequestOutcomeHistogramName),
               testing::ElementsAre(base::Bucket(2 /* CANCELED */, 1)));
 }
+
+TEST_F(PasswordProtectionServiceTest, TestCleanUpExpiredVerdict) {
+  ASSERT_EQ(0U, GetStoredVerdictCount());
+  // Prepare 4 verdicts:
+  // (1) "foo.com/abc" valid
+  // (2) "foo.com/def" expired
+  // (3) "bar.com/abc" expired
+  // (4) "bar.com/def" expired
+  base::Time now = base::Time::Now();
+  CacheVerdict(GURL("https://foo.com/abc/index.jsp"),
+               LoginReputationClientResponse::LOW_REPUTATION, 10 * 60,
+               "foo.com/abc", now);
+  CacheVerdict(GURL("https://foo.com/def/index.jsp"),
+               LoginReputationClientResponse::LOW_REPUTATION, 0, "foo.com/def",
+               now);
+  CacheVerdict(GURL("https://bar.com/abc/index.jsp"),
+               LoginReputationClientResponse::PHISHING, 0, "bar.com/abc", now);
+  CacheVerdict(GURL("https://bar.com/def/index.jsp"),
+               LoginReputationClientResponse::PHISHING, 0, "bar.com/def", now);
+  ASSERT_EQ(4U, GetStoredVerdictCount());
+
+  password_protection_service_->CleanUpExpiredVerdicts();
+
+  ASSERT_EQ(1U, GetStoredVerdictCount());
+  LoginReputationClientResponse actual_verdict;
+  // Has cached verdict for foo.com/abc.
+  EXPECT_EQ(LoginReputationClientResponse::LOW_REPUTATION,
+            password_protection_service_->GetCachedVerdict(
+                GURL("https://foo.com/abc/test.jsp"), &actual_verdict));
+  // No cached verdict for foo.com/def.
+  EXPECT_EQ(LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED,
+            password_protection_service_->GetCachedVerdict(
+                GURL("https://foo.com/def/index.jsp"), &actual_verdict));
+  // Nothing in content setting for bar.com.
+  EXPECT_EQ(nullptr, content_setting_map_->GetWebsiteSetting(
+                         GURL("https://bar.com"), GURL(),
+                         CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+                         std::string(), nullptr));
+}
 }  // namespace safe_browsing