Link PasswordProtectionService to Profile and SB Service

components/safe_browsing/password_protection/password_protection_service.cc

Index: components/safe_browsing/password_protection/password_protection_service.cc
diff --git a/components/safe_browsing/password_protection/password_protection_service.cc b/components/safe_browsing/password_protection/password_protection_service.cc
index 47bdeb41aa2e5b7d97b01815cddc9293eb134896..5f977c1e36ed275aeda26704e61228c4213d92b6 100644
--- a/components/safe_browsing/password_protection/password_protection_service.cc
+++ b/components/safe_browsing/password_protection/password_protection_service.cc
@@ -11,6 +11,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/history/core/browser/history_service.h"
 #include "components/safe_browsing/password_protection/password_protection_request.h"
 #include "components/safe_browsing_db/database_manager.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
@@ -19,6 +21,7 @@
 #include "net/base/escape.h"
 
 using content::BrowserThread;
+using history::HistoryService;
 
 namespace safe_browsing {
 
@@ -62,15 +65,23 @@ GURL GetHostNameWithHTTPScheme(const GURL& url) {
 
 PasswordProtectionService::PasswordProtectionService(
     const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager,
-    scoped_refptr<net::URLRequestContextGetter> request_context_getter)
-    : request_context_getter_(request_context_getter),
+    scoped_refptr<net::URLRequestContextGetter> request_context_getter,
+    HistoryService* history_service,
+    HostContentSettingsMap* host_content_settings_map)
+    : stored_verdict_count_(-1),
       database_manager_(database_manager),
+      request_context_getter_(request_context_getter),
+      history_service_observer_(this),
+      content_settings_(host_content_settings_map),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (history_service)
+    history_service_observer_.Add(history_service);
 }
 
 PasswordProtectionService::~PasswordProtectionService() {
   CancelPendingRequests();
+  history_service_observer_.RemoveAll();
   weak_factory_.InvalidateWeakPtrs();
 }
 
@@ -102,17 +113,15 @@ void PasswordProtectionService::CheckCsdWhitelistOnIOThread(
 
 LoginReputationClientResponse::VerdictType
 PasswordProtectionService::GetCachedVerdict(
-    const HostContentSettingsMap* settings,
     const GURL& url,
     LoginReputationClientResponse* out_response) {
-  DCHECK(settings);
-  if (!url.is_valid()) {
+  if (!url.is_valid() || !content_settings_) {

[Nathan Parker, 2017/03/30 21:38:12]

when would !content_settings be true? Should that be a dcheck?

[Jialiu Lin, 2017/03/30 23:23:07]

Yes.... object lifetime scares me... so I tend to use if condition more often
than DCHECK. And I'll try to get rid of this habit.

Changed to DCHECK. 

[Nathan Parker, 2017/03/31 18:41:15]

I totally agree! Better safe than sorry...  But then we don't want to hide a
bug.  I sorta want to have it both ways:

if (!ptr) {
  DCHECK(false);
  return;
}

But then that's saying we're not confident in our testing, and it adds
complexity (you have to handle that early return, and its side effects, etc).

     return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
   }
 
   GURL hostname = GetHostNameWithHTTPScheme(url);
   std::unique_ptr<base::DictionaryValue> verdict_dictionary =
-      base::DictionaryValue::From(settings->GetWebsiteSetting(
+      base::DictionaryValue::From(content_settings_->GetWebsiteSetting(
           hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
           std::string(), nullptr));
   // Return early if there is no verdict cached for this origin.
@@ -170,13 +179,13 @@ PasswordProtectionService::GetCachedVerdict(
 void PasswordProtectionService::CacheVerdict(
     const GURL& url,
     LoginReputationClientResponse* verdict,
-    const base::Time& receive_time,
-    HostContentSettingsMap* settings) {
+    const base::Time& receive_time) {
   DCHECK(verdict);
+  DCHECK(content_settings_);
 
   GURL hostname = GetHostNameWithHTTPScheme(url);
   std::unique_ptr<base::DictionaryValue> verdict_dictionary =
-      base::DictionaryValue::From(settings->GetWebsiteSetting(
+      base::DictionaryValue::From(content_settings_->GetWebsiteSetting(
           hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
           std::string(), nullptr));
 
@@ -189,25 +198,25 @@ void PasswordProtectionService::CacheVerdict(
   // Increases stored verdict count if we haven't seen this cache expression
   // before.
   if (!verdict_dictionary->HasKey(verdict->cache_expression()))
-    stored_verdict_counts_[settings] = GetStoredVerdictCount() + 1U;
+    stored_verdict_count_ = GetStoredVerdictCount() + 1;
   // If same cache_expression is already in this verdict_dictionary, we simply
   // override it.
   verdict_dictionary->SetWithoutPathExpansion(verdict->cache_expression(),
                                               std::move(verdict_entry));
-  settings->SetWebsiteSettingDefaultScope(
+  content_settings_->SetWebsiteSettingDefaultScope(
       hostname, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
       std::string(), std::move(verdict_dictionary));
 }
 
 void PasswordProtectionService::StartRequest(
     const GURL& main_frame_url,
-    LoginReputationClientRequest::TriggerType type,
-    bool is_extended_reporting,
-    bool is_incognito) {
+    LoginReputationClientRequest::TriggerType type) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!IsPingingEnabled())
+    return;
   std::unique_ptr<PasswordProtectionRequest> request =
       base::MakeUnique<PasswordProtectionRequest>(
-          main_frame_url, type, is_extended_reporting, is_incognito,
+          main_frame_url, type, IsExtendedReporting(), IsIncognito(),
           GetWeakPtr(), GetRequestTimeoutInMS());
   DCHECK(request);
   request->Start();
@@ -222,10 +231,9 @@ void PasswordProtectionService::RequestFinished(
   DCHECK(request);
   // TODO(jialiul): We don't cache verdict for incognito mode for now.
   // Later we may consider temporarily caching verdict.
-  if (!request->is_incognito() && response) {
-    CacheVerdict(request->main_frame_url(), response.get(), base::Time::Now(),
-                 GetSettingMapForActiveProfile());
-  }
+  if (!request->is_incognito() && response)
+    CacheVerdict(request->main_frame_url(), response.get(), base::Time::Now());
+
   // Finished processing this request. Remove it from pending list.
   for (auto it = requests_.begin(); it != requests_.end(); it++) {
     if (it->get() == request) {
@@ -254,31 +262,30 @@ GURL PasswordProtectionService::GetPasswordProtectionRequestUrl() {
   return url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 }
 
-size_t PasswordProtectionService::GetStoredVerdictCount() {
-  HostContentSettingsMap* content_setting_map = GetSettingMapForActiveProfile();
-  DCHECK(content_setting_map);
-  auto it = stored_verdict_counts_.find(content_setting_map);
+int PasswordProtectionService::GetStoredVerdictCount() {
+  DCHECK(content_settings_);
   // If we have already computed this, return its value.
-  if (it != stored_verdict_counts_.end())
-    return it->second;
+  if (stored_verdict_count_ >= 0)
+    return stored_verdict_count_;
 
   ContentSettingsForOneType password_protection_settings;
-  content_setting_map->GetSettingsForOneType(
+  content_settings_->GetSettingsForOneType(
       CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(),
       &password_protection_settings);
-  stored_verdict_counts_[content_setting_map] = 0U;
+  stored_verdict_count_ = 0;
   if (password_protection_settings.empty())
-    return 0U;
+    return 0;
 
   for (const ContentSettingPatternSource& source :
        password_protection_settings) {
     std::unique_ptr<base::DictionaryValue> verdict_dictionary =
-        base::DictionaryValue::From(content_setting_map->GetWebsiteSetting(
+        base::DictionaryValue::From(content_settings_->GetWebsiteSetting(
             GURL(source.primary_pattern.ToString()), GURL(),
             CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION, std::string(), nullptr));
-    stored_verdict_counts_[content_setting_map] += verdict_dictionary->size();
+    if (verdict_dictionary.get() && verdict_dictionary->empty())

[Nathan Parker, 2017/03/30 21:38:12]

Do you mean !empty()?  If it's empty, the ->size() would be 0.

[Jialiu Lin, 2017/03/30 23:23:06]

Oops. it should be !empty().

+      stored_verdict_count_ += static_cast<int>(verdict_dictionary->size());
   }
-  return stored_verdict_counts_[content_setting_map];
+  return stored_verdict_count_;

[Nathan Parker, 2017/03/30 21:38:12]

It might be interesting to log this value to UMA, like on startup or on profile
registration. Not necessary in this CL.

[Jialiu Lin, 2017/03/30 23:23:08]

This number is sent with every ping. I think that's probably enough. 

 }
 
 int PasswordProtectionService::GetRequestTimeoutInMS() {
@@ -292,38 +299,33 @@ void PasswordProtectionService::OnMatchCsdWhiteListResult(
       match_whitelist);
 }
 
-HostContentSettingsMap*
-PasswordProtectionService::GetSettingMapForActiveProfile() {
-  // TODO(jialiul): Make this a pure virtual function when we have a derived
-  // class ready in chrome/browser/safe_browsing directory.
-  return nullptr;
-}
-
 void PasswordProtectionService::OnURLsDeleted(
     history::HistoryService* history_service,
     bool all_history,
     bool expired,
     const history::URLRows& deleted_rows,
     const std::set<GURL>& favicon_urls) {
-  HostContentSettingsMap* setting_map = GetSettingMapForActiveProfile();
-  if (!setting_map)
-    return;
-
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&PasswordProtectionService::RemoveContentSettingsOnURLsDeleted,
-                 GetWeakPtr(), all_history, deleted_rows, setting_map));
+                 GetWeakPtr(), all_history, deleted_rows));
+}
+
+void PasswordProtectionService::HistoryServiceBeingDeleted(
+    history::HistoryService* history_service) {
+  history_service_observer_.RemoveAll();
 }
 
 void PasswordProtectionService::RemoveContentSettingsOnURLsDeleted(
     bool all_history,
-    const history::URLRows& deleted_rows,
-    HostContentSettingsMap* setting_map) {
+    const history::URLRows& deleted_rows) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(content_settings_);
+
   if (all_history) {
-    setting_map->ClearSettingsForOneType(
+    content_settings_->ClearSettingsForOneType(
         CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION);
-    stored_verdict_counts_[setting_map] = 0U;
+    stored_verdict_count_ = 0;
     return;
   }
 
@@ -334,13 +336,17 @@ void PasswordProtectionService::RemoveContentSettingsOnURLsDeleted(
   for (const history::URLRow& row : deleted_rows) {
     GURL url_key = GetHostNameWithHTTPScheme(row.url());
     std::unique_ptr<base::DictionaryValue> verdict_dictionary =
-        base::DictionaryValue::From(setting_map->GetWebsiteSetting(
+        base::DictionaryValue::From(content_settings_->GetWebsiteSetting(
             url_key, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
             std::string(), nullptr));
-    size_t verdict_count = verdict_dictionary->size();
-    stored_verdict_counts_[setting_map] =
-        GetStoredVerdictCount() - verdict_count;
-    setting_map->ClearSettingsForOneTypeWithPredicate(
+
+    // Move on if we have no cached verdict for this deleted history row.
+    if (!verdict_dictionary.get() || verdict_dictionary->empty())
+      continue;
+
+    int verdict_count = static_cast<int>(verdict_dictionary->size());
+    stored_verdict_count_ = GetStoredVerdictCount() - verdict_count;
+    content_settings_->ClearSettingsForOneTypeWithPredicate(
         CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
         base::Bind(&OriginMatchPrimaryPattern, url_key));
   }