Fix crash in CleanUpExpiredVerdicts

In https://codereview.chromium.org/2895323002, I slightly changed the 
way how verdict is cached, which makes previous cached entries no
longer valid. Therefore, CleanUpExpiredVerdicts should also remove
any invalid verdict entries together with the expired ones.

TBR=vakh@chromium.org

Bug: 742713
Change-Id: Id9e162d485e436e4992cb83c5a4378eea3df9771
Reviewed-on: https://chromium-review.googlesource.com/572086
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Luke Z <lpz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486877}

components/safe_browsing/password_protection/password_protection_service.cc

@@ -180,11 +180,14 @@ PasswordProtectionService::GetCachedVerdict(
       continue;
     }
     base::DictionaryValue* verdict_entry = nullptr;
-    CHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion(
-        it.key() /* cache_expression */, &verdict_entry));
+    verdict_dictionary->GetDictionaryWithoutPathExpansion(
+        it.key() /* cache_expression */, &verdict_entry);
     int verdict_received_time;
     LoginReputationClientResponse verdict;
-    CHECK(ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict));
+    // Ignore any entry that we cannot parse. These invalid entries will be
+    // cleaned up during shutdown.
+    if (!ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict))
+      continue;
     // Since password protection content settings are keyed by origin, we only
     // need to compare the path part of the cache_expression and the given url.
     std::string cache_expression_path =
@@ -609,13 +612,13 @@ bool PasswordProtectionService::RemoveExpiredVerdicts(
       continue;
 
     base::DictionaryValue* verdict_entry = nullptr;
-    CHECK(verdict_dictionary->GetDictionaryWithoutPathExpansion(
-        it.key(), &verdict_entry));
+    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())) {
+    if (!ParseVerdictEntry(verdict_entry, &verdict_received_time, &verdict) ||
+        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.

components/safe_browsing/password_protection/password_protection_service_unittest.cc

@@ -222,6 +222,27 @@ class PasswordProtectionServiceTest : public testing::Test {
                                                verdict_received_time);
   }
 
+  void CacheInvalidVerdict() {
+    GURL invalid_hostname("http://invalid.com");
+    std::unique_ptr<base::DictionaryValue> verdict_dictionary =
+        base::DictionaryValue::From(content_setting_map_->GetWebsiteSetting(
+            invalid_hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+            std::string(), nullptr));
+
+    if (!verdict_dictionary.get())
+      verdict_dictionary = base::MakeUnique<base::DictionaryValue>();
+
+    std::unique_ptr<base::DictionaryValue> invalid_verdict_entry =
+        base::MakeUnique<base::DictionaryValue>();
+    invalid_verdict_entry->SetString("invalid", "invalid_string");
+
+    verdict_dictionary->SetWithoutPathExpansion(
+        "invalid_cache_expression", std::move(invalid_verdict_entry));
+    content_setting_map_->SetWebsiteSettingDefaultScope(
+        invalid_hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+        std::string(), std::move(verdict_dictionary));
+  }
+
   size_t GetStoredVerdictCount(LoginReputationClientRequest::TriggerType type) {
     return password_protection_service_->GetStoredVerdictCount(type);
   }
@@ -815,6 +836,23 @@ TEST_F(PasswordProtectionServiceTest, TestCleanUpExpiredVerdict) {
                 &actual_verdict));
 }
 
+TEST_F(PasswordProtectionServiceTest,
+       TestCleanUpExpiredVerdictWithInvalidEntry) {
+  CacheInvalidVerdict();
+  ContentSettingsForOneType password_protection_settings;
+  content_setting_map_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+  ASSERT_FALSE(password_protection_settings.empty());
+
+  password_protection_service_->CleanUpExpiredVerdicts();
+
+  content_setting_map_->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
+      &password_protection_settings);
+  EXPECT_TRUE(password_protection_settings.empty());
+}
+
 TEST_F(PasswordProtectionServiceTest, VerifyPasswordOnFocusRequestProto) {
   // Set up valid response.
   net::TestURLFetcher fetcher(0, GURL("http://bar.com"), nullptr);