Create verdict_cache_manager_factory to manage verdict_cache_manager.

Previously, verdict_cache_manager is managed by services_delegate.
This is not a typical way of managing objects keyed by a profile.

In this CL, make verdict_cache_manager a child class of
keyed_service. Also introduce verdict_cache_manager_factory as a keyed
service factory to manage it.

Bug: 1058179,1057937
Change-Id: Ia43d05c3f05f597770e906b1c6e20e8626fc7021
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2085632Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746858}

chrome/browser/safe_browsing/BUILD.gn

@@ -99,6 +99,7 @@ jumbo_static_library("safe_browsing") {
     ]
     deps += [
       ":url_lookup_service_factory",
+      ":verdict_cache_manager_factory",
       "//chrome/browser/engagement:mojo_bindings",
       "//chrome/common/safe_browsing:proto",
       "//components/safe_browsing/content",
@@ -279,6 +280,7 @@ source_set("url_lookup_service_factory") {
   ]
 
   deps = [
+    ":verdict_cache_manager_factory",
     "//components/keyed_service/content",
     "//components/safe_browsing/core/realtime:url_lookup_service",
     "//components/signin/public/identity_manager",
@@ -286,6 +288,22 @@ source_set("url_lookup_service_factory") {
   ]
 }
 
+source_set("verdict_cache_manager_factory") {
+  sources = [
+    "verdict_cache_manager_factory.cc",
+    "verdict_cache_manager_factory.h",
+  ]
+
+  deps = [
+    "//components/content_settings/core/browser",
+    "//components/history/core/browser",
+    "//components/keyed_service/content",
+    "//components/prefs",
+    "//components/safe_browsing/core:verdict_cache_manager",
+    "//content/public/browser",
+  ]
+}
+
 static_library("advanced_protection") {
   sources = [
     "advanced_protection_status_manager.cc",

chrome/browser/safe_browsing/chrome_password_protection_service.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
+#include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
@@ -234,7 +235,7 @@ ChromePasswordProtectionService::ChromePasswordProtectionService(
       profile_(profile),
       navigation_observer_manager_(sb_service->navigation_observer_manager()),
       pref_change_registrar_(new PrefChangeRegistrar),
-      cache_manager_(sb_service->GetVerdictCacheManager(profile)) {
+      cache_manager_(VerdictCacheManagerFactory::GetForProfile(profile)) {
   pref_change_registrar_->Init(profile_->GetPrefs());
 
 #if defined(SYNC_PASSWORD_REUSE_WARNING_ENABLED)

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -45,7 +45,6 @@
 #include "components/safe_browsing/core/ping_manager.h"
 #include "components/safe_browsing/core/realtime/policy_engine.h"
 #include "components/safe_browsing/core/triggers/trigger_manager.h"
-#include "components/safe_browsing/core/verdict_cache_manager.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "services/network/public/cpp/cross_thread_pending_shared_url_loader_factory.h"
@@ -241,13 +240,6 @@ V4ProtocolConfig SafeBrowsingService::GetV4ProtocolConfig() const {
       cmdline->HasSwitch(::switches::kDisableBackgroundNetworking));
 }
 
-VerdictCacheManager* SafeBrowsingService::GetVerdictCacheManager(
-    Profile* profile) const {
-  if (profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled))
-    return services_delegate_->GetVerdictCacheManager(profile);
-  return nullptr;
-}
-
 BinaryUploadService* SafeBrowsingService::GetBinaryUploadService(
     Profile* profile) const {
   return services_delegate_->GetBinaryUploadService(profile);
@@ -367,14 +359,12 @@ void SafeBrowsingService::OnOffTheRecordProfileCreated(
 
 void SafeBrowsingService::OnProfileWillBeDestroyed(Profile* profile) {
   observed_profiles_.Remove(profile);
-  services_delegate_->RemoveVerdictCacheManager(profile);
   services_delegate_->RemovePasswordProtectionService(profile);
   services_delegate_->RemoveTelemetryService(profile);
   services_delegate_->RemoveBinaryUploadService(profile);
 }
 
 void SafeBrowsingService::CreateServicesForProfile(Profile* profile) {
-  services_delegate_->CreateVerdictCacheManager(profile);
   services_delegate_->CreatePasswordProtectionService(profile);
   services_delegate_->CreateTelemetryService(profile);
   services_delegate_->CreateBinaryUploadService(profile);

chrome/browser/safe_browsing/services_delegate.cc

@@ -18,7 +18,6 @@
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/safe_browsing/buildflags.h"
 #include "components/safe_browsing/core/db/v4_local_database_manager.h"
-#include "components/safe_browsing/core/verdict_cache_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h"
@@ -62,37 +61,7 @@ PasswordProtectionService* ServicesDelegate::GetPasswordProtectionService(
                                                       : nullptr;
 }
 
-void ServicesDelegate::CreateVerdictCacheManager(Profile* profile) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(profile);
-  auto it = cache_manager_map_.find(profile);
-  DCHECK(it == cache_manager_map_.end());
-  auto cache_manager = std::make_unique<VerdictCacheManager>(
-      HistoryServiceFactory::GetForProfile(profile,
-                                           ServiceAccessType::EXPLICIT_ACCESS),
-      HostContentSettingsMapFactory::GetForProfile(profile));
-  cache_manager_map_[profile] = std::move(cache_manager);
-}
-
-void ServicesDelegate::RemoveVerdictCacheManager(Profile* profile) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(profile);
-  auto it = cache_manager_map_.find(profile);
-  if (it != cache_manager_map_.end())
-    cache_manager_map_.erase(it);
-}
-
-VerdictCacheManager* ServicesDelegate::GetVerdictCacheManager(
-    Profile* profile) const {
-  DCHECK(profile);
-  auto it = cache_manager_map_.find(profile);
-  return it != cache_manager_map_.end() ? it->second.get() : nullptr;
-}
-
 void ServicesDelegate::ShutdownServices() {
-  // Delete the VerdictCacheManager instances
-  cache_manager_map_.clear();
-
   // Delete the ChromePasswordProtectionService instances.
   password_protection_service_map_.clear();
 }

chrome/browser/safe_browsing/services_delegate.h

@@ -41,7 +41,6 @@ struct ResourceRequestInfo;
 class SafeBrowsingService;
 class SafeBrowsingDatabaseManager;
 struct V4ProtocolConfig;
-class VerdictCacheManager;
 
 // Abstraction to help organize code for mobile vs full safe browsing modes.
 // This helper class should be owned by a SafeBrowsingService, and it handles
@@ -130,10 +129,6 @@ class ServicesDelegate {
   virtual void CreateTelemetryService(Profile* profile) {}
   virtual void RemoveTelemetryService(Profile* profile) {}
 
-  virtual void CreateVerdictCacheManager(Profile* profile);
-  virtual void RemoveVerdictCacheManager(Profile* profile);
-  virtual VerdictCacheManager* GetVerdictCacheManager(Profile* profile) const;
-
   virtual void CreateBinaryUploadService(Profile* profile) = 0;
   virtual void RemoveBinaryUploadService(Profile* profile) = 0;
   virtual BinaryUploadService* GetBinaryUploadService(
@@ -154,10 +149,6 @@ class ServicesDelegate {
   base::flat_map<Profile*, std::unique_ptr<ChromePasswordProtectionService>>
       password_protection_service_map_;
 
-  // Tracks existing Profiles, and their corresponding VerdictCacheManager
-  // instances. Accessed on UI thread.
-  base::flat_map<Profile*, std::unique_ptr<VerdictCacheManager>>
-      cache_manager_map_;
 };
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/url_lookup_service_factory.cc

@@ -7,6 +7,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
@@ -36,6 +37,7 @@ RealTimeUrlLookupServiceFactory::RealTimeUrlLookupServiceFactory()
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(IdentityManagerFactory::GetInstance());
   DependsOn(ProfileSyncServiceFactory::GetInstance());
+  DependsOn(VerdictCacheManagerFactory::GetInstance());
 }
 
 KeyedService* RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
@@ -47,17 +49,10 @@ KeyedService* RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   auto url_loader_factory =
       std::make_unique<network::CrossThreadPendingSharedURLLoaderFactory>(
           g_browser_process->safe_browsing_service()->GetURLLoaderFactory());
-  // When |url_lookup_service| constructs, |cache_manager| is already
-  // constructed. Because |cache_manager| is constructed by |services_delegate|
-  // when the profile is created and |url_lookup_service| is constructed
-  // when the navigation starts.
-  VerdictCacheManager* cache_manager =
-      g_browser_process->safe_browsing_service()->GetVerdictCacheManager(
-          profile);
-  DCHECK(cache_manager);
   return new RealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
-      cache_manager, IdentityManagerFactory::GetForProfile(profile),
+      VerdictCacheManagerFactory::GetForProfile(profile),
+      IdentityManagerFactory::GetForProfile(profile),
       ProfileSyncServiceFactory::GetForProfile(profile), profile->GetPrefs(),
       profile->IsOffTheRecord());
 }

chrome/browser/safe_browsing/verdict_cache_manager_factory.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
+
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/safe_browsing/core/verdict_cache_manager.h"
+#include "content/public/browser/browser_context.h"
+
+namespace safe_browsing {
+
+// static
+VerdictCacheManager* VerdictCacheManagerFactory::GetForProfile(
+    Profile* profile) {
+  return static_cast<VerdictCacheManager*>(
+      GetInstance()->GetServiceForBrowserContext(profile, /* create= */ true));
+}
+
+// static
+VerdictCacheManagerFactory* VerdictCacheManagerFactory::GetInstance() {
+  return base::Singleton<VerdictCacheManagerFactory>::get();
+}
+
+VerdictCacheManagerFactory::VerdictCacheManagerFactory()
+    : BrowserContextKeyedServiceFactory(
+          "VerdictCacheManager",
+          BrowserContextDependencyManager::GetInstance()) {
+  DependsOn(HistoryServiceFactory::GetInstance());
+  DependsOn(HostContentSettingsMapFactory::GetInstance());
+}
+
+KeyedService* VerdictCacheManagerFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  Profile* profile = Profile::FromBrowserContext(context);
+  return new VerdictCacheManager(
+      HistoryServiceFactory::GetForProfile(profile,
+                                           ServiceAccessType::EXPLICIT_ACCESS),
+      HostContentSettingsMapFactory::GetForProfile(profile));
+}
+
+content::BrowserContext* VerdictCacheManagerFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextOwnInstanceInIncognito(context);
+}
+
+}  // namespace safe_browsing

chrome/browser/safe_browsing/verdict_cache_manager_factory.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_VERDICT_CACHE_MANAGER_FACTORY_H_
+#define CHROME_BROWSER_SAFE_BROWSING_VERDICT_CACHE_MANAGER_FACTORY_H_
+
+#include "base/memory/singleton.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+
+class KeyedService;
+class Profile;
+
+namespace content {
+class BrowserContext;
+}
+
+namespace safe_browsing {
+
+class VerdictCacheManager;
+
+// Singleton that owns VerdictCacheManager objects, one for each active
+// Profile. It listens to profile destroy events and destroy its associated
+// service. It returns a separate instance if the profile is in the Incognito
+// mode.
+class VerdictCacheManagerFactory : public BrowserContextKeyedServiceFactory {
+ public:
+  // Creates the service if it doesn't exist already for the given |profile|.
+  // If the service already exists, return its pointer.
+  static VerdictCacheManager* GetForProfile(Profile* profile);
+
+  // Get the singleton instance.
+  static VerdictCacheManagerFactory* GetInstance();
+
+ private:
+  friend struct base::DefaultSingletonTraits<VerdictCacheManagerFactory>;
+
+  VerdictCacheManagerFactory();
+  ~VerdictCacheManagerFactory() override = default;
+
+  // BrowserContextKeyedServiceFactory:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* context) const override;
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+
+  DISALLOW_COPY_AND_ASSIGN(VerdictCacheManagerFactory);
+};
+
+}  // namespace safe_browsing
+#endif  // CHROME_BROWSER_SAFE_BROWSING_VERDICT_CACHE_MANAGER_FACTORY_H_

chrome/browser/safe_browsing/verdict_cache_manager_factory_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
+
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/browser_task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+
+// Check that VerdictCacheManagerFactory returns different object
+// for off-the-record profile and regular profile.
+TEST(VerdictCacheManagerFactoryTest, OffTheRecordUseDifferentService) {
+  content::BrowserTaskEnvironment task_environment;
+
+  TestingProfile::Builder builder;
+  std::unique_ptr<TestingProfile> testing_profile = builder.Build();
+
+  // There should be a not null object for off-the-record profile.
+  EXPECT_NE(nullptr, VerdictCacheManagerFactory::GetForProfile(
+                         testing_profile->GetOffTheRecordProfile()));
+
+  EXPECT_NE(VerdictCacheManagerFactory::GetForProfile(testing_profile.get()),
+            VerdictCacheManagerFactory::GetForProfile(
+                testing_profile->GetOffTheRecordProfile()));
+}
+
+}  // namespace safe_browsing

chrome/test/BUILD.gn

@@ -5036,6 +5036,7 @@ test("unit_tests") {
       "../browser/safe_browsing/local_two_phase_testserver.h",
       "../browser/safe_browsing/safe_browsing_navigation_observer_unittest.cc",
       "../browser/safe_browsing/signature_evaluator_mac_unittest.cc",
+      "../browser/safe_browsing/verdict_cache_manager_factory_unittest.cc",
       "../common/safe_browsing/binary_feature_extractor_mac_unittest.cc",
       "../common/safe_browsing/binary_feature_extractor_unittest.cc",
       "../common/safe_browsing/binary_feature_extractor_win_unittest.cc",

components/safe_browsing/core/BUILD.gn

@@ -76,6 +76,7 @@ source_set("verdict_cache_manager") {
     "//base",
     "//components/content_settings/core/browser",
     "//components/history/core/browser",
+    "//components/keyed_service/core:core",
     "//components/password_manager/core/browser:browser",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:v4_protocol_manager_util",

components/safe_browsing/core/verdict_cache_manager.cc

@@ -366,12 +366,14 @@ VerdictCacheManager::VerdictCacheManager(
     history_service_observer_.Add(history_service);
 }
 
-VerdictCacheManager::~VerdictCacheManager() {
+void VerdictCacheManager::Shutdown() {
   CleanUpExpiredVerdicts();
   history_service_observer_.RemoveAll();
   weak_factory_.InvalidateWeakPtrs();
 }
 
+VerdictCacheManager::~VerdictCacheManager() {}
+
 void VerdictCacheManager::CachePhishGuardVerdict(
     LoginReputationClientRequest::TriggerType trigger_type,
     ReusedPasswordAccountType password_type,

components/safe_browsing/core/verdict_cache_manager.h

@@ -14,6 +14,7 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_service_observer.h"
+#include "components/keyed_service/core/keyed_service.h"
 #include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/proto/realtimeapi.pb.h"
 #include "url/gurl.h"
@@ -26,7 +27,8 @@ using ReusedPasswordAccountType =
     LoginReputationClientRequest::PasswordReuseEvent::ReusedPasswordAccountType;
 
 // Structure: http://screen/YaNfDRYrcnk.png.
-class VerdictCacheManager : public history::HistoryServiceObserver {
+class VerdictCacheManager : public history::HistoryServiceObserver,
+                            public KeyedService {
  public:
   explicit VerdictCacheManager(
       history::HistoryService* history_service,
@@ -42,6 +44,10 @@ class VerdictCacheManager : public history::HistoryServiceObserver {
     return weak_factory_.GetWeakPtr();
   }
 
+  // KeyedService:
+  // Called before the actual deletion of the object.
+  void Shutdown() override;
+
   // Stores |verdict| in |content_settings_| based on its |trigger_type|, |url|,
   // reused |password_type|, |verdict| and |receive_time|.
   void CachePhishGuardVerdict(