Use ProfileManagerObserver::OnProfileAdded in SafeBrowsingService

Instead of using NOTIFICATION_PROFILE_CREATED, listen for
OnProfileAdded. This will automatically filter out incognito profiles,
which are not "added" to the ProfileManager. Also, there's no need to
listen for profile destruction as ProfileManager and its profiles are
only destroyed after SafeBrowsingService is shut down.

Bug: 268984
Change-Id: I73f8163d6488968fec019f878951172d348e9e81
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1836433Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703113}

chrome/browser/extensions/api/safe_browsing_private/safe_browsing_private_api_unittest.cc

@@ -101,7 +101,7 @@ void SafeBrowsingPrivateApiUnitTest::SetUp() {
   TestingBrowserProcess::GetGlobal()->SetSafeBrowsingService(
       safe_browsing_service);
   g_browser_process->safe_browsing_service()->Initialize();
-  safe_browsing_service->AddPrefService(profile()->GetPrefs());
+  safe_browsing_service->OnProfileAdded(profile());
 }
 
 void SafeBrowsingPrivateApiUnitTest::TearDown() {

chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc

@@ -301,7 +301,7 @@ class SafeBrowsingBlockingPageTestBase
     // get notified of it, so include that notification now.
     Profile* profile =
         Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-    safe_browsing_service->AddPrefService(profile->GetPrefs());
+    safe_browsing_service->OnProfileAdded(profile);
     content::BrowserThread::RunAllPendingTasksOnThreadForTesting(
         content::BrowserThread::IO);
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -1105,10 +1105,10 @@ class SafeBrowsingBlockingQuietPageTest
     auto* safe_browsing_service =
         sb_service_factory.CreateSafeBrowsingService();
     // A profile was created already but SafeBrowsingService wasn't around to
-    // get notified of it, so include that notification now.
-    safe_browsing_service->AddPrefService(
-        Profile::FromBrowserContext(web_contents()->GetBrowserContext())
-            ->GetPrefs());
+    // get notified of it (and it wasn't associated with a ProfileManager), so
+    // include that profile now.
+    safe_browsing_service->OnProfileAdded(
+        Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
     TestingBrowserProcess::GetGlobal()->SetSafeBrowsingService(
         safe_browsing_service);
     g_browser_process->safe_browsing_service()->Initialize();

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -128,6 +128,8 @@ void SafeBrowsingService::Initialize() {
   CreateTriggerManager();
 
   // Track profile creation and destruction.
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->AddObserver(this);
   profiles_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
                           content::NotificationService::AllSources());
   profiles_registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
@@ -143,6 +145,8 @@ void SafeBrowsingService::ShutDown() {
   shutdown_ = true;
 
   // Remove Profile creation/destruction observers.
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->RemoveObserver(this);
   profiles_registrar_.RemoveAll();
 
   // Delete the PrefChangeRegistrars, whose dtors also unregister |this| as an
@@ -341,8 +345,6 @@ void SafeBrowsingService::Observe(int type,
       services_delegate_->CreateVerdictCacheManager(profile);
       services_delegate_->CreatePasswordProtectionService(profile);
       services_delegate_->CreateTelemetryService(profile);
-      if (!profile->IsOffTheRecord())
-        AddPrefService(profile->GetPrefs());
       services_delegate_->CreateBinaryUploadService(profile);
       break;
     }
@@ -352,8 +354,6 @@ void SafeBrowsingService::Observe(int type,
       services_delegate_->RemoveVerdictCacheManager(profile);
       services_delegate_->RemovePasswordProtectionService(profile);
       services_delegate_->RemoveTelemetryService();
-      if (!profile->IsOffTheRecord())
-        RemovePrefService(profile->GetPrefs());
       services_delegate_->RemoveBinaryUploadService(profile);
       break;
     }
@@ -362,7 +362,9 @@ void SafeBrowsingService::Observe(int type,
   }
 }
 
-void SafeBrowsingService::AddPrefService(PrefService* pref_service) {
+void SafeBrowsingService::OnProfileAdded(Profile* profile) {
+  // Start following the safe browsing preference on |pref_service|.
+  PrefService* pref_service = profile->GetPrefs();
   DCHECK(prefs_map_.find(pref_service) == prefs_map_.end());
   std::unique_ptr<PrefChangeRegistrar> registrar =
       std::make_unique<PrefChangeRegistrar>();
@@ -388,15 +390,6 @@ void SafeBrowsingService::AddPrefService(PrefService* pref_service) {
   RecordExtendedReportingMetrics(*pref_service);
 }
 
-void SafeBrowsingService::RemovePrefService(PrefService* pref_service) {
-  if (prefs_map_.find(pref_service) != prefs_map_.end()) {
-    prefs_map_.erase(pref_service);
-    RefreshState();
-  } else {
-    NOTREACHED();
-  }
-}
-
 std::unique_ptr<SafeBrowsingService::StateSubscription>
 SafeBrowsingService::RegisterStateCallback(
     const base::Callback<void(void)>& callback) {

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -20,6 +20,7 @@
 #include "base/observer_list.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "chrome/browser/net/proxy_config_monitor.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/safe_browsing/services_delegate.h"
 #include "components/safe_browsing/buildflags.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
@@ -79,7 +80,8 @@ class TriggerManager;
 // alive until SafeBrowsingService is destroyed, however, they are disabled
 // permanently when Shutdown method is called.
 class SafeBrowsingService : public SafeBrowsingServiceInterface,
-                            public content::NotificationObserver {
+                            public content::NotificationObserver,
+                            public ProfileManagerObserver {
  public:
   static base::FilePath GetCookieFilePathForTesting();
 
@@ -243,11 +245,8 @@ class SafeBrowsingService : public SafeBrowsingServiceInterface,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
-  // Starts following the safe browsing preference on |pref_service|.
-  void AddPrefService(PrefService* pref_service);
-
-  // Stop following the safe browsing preference on |pref_service|.
-  void RemovePrefService(PrefService* pref_service);
+  // ProfileManagerObserver overrides:
+  void OnProfileAdded(Profile* profile) override;
 
   // Checks if any profile is currently using the safe browsing service, and
   // starts or stops the service accordingly.

chrome/browser/safe_browsing/ui_manager_unittest.cc

@@ -96,9 +96,8 @@ class SafeBrowsingUIManagerTest : public ChromeRenderViewHostTestHarness {
     g_browser_process->safe_browsing_service()->Initialize();
     // A profile was created already but SafeBrowsingService wasn't around to
     // get notified of it, so include that notification now.
-    safe_browsing_service->AddPrefService(
-        Profile::FromBrowserContext(web_contents()->GetBrowserContext())
-            ->GetPrefs());
+    safe_browsing_service->OnProfileAdded(
+        Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
     content::BrowserThread::RunAllPendingTasksOnThreadForTesting(
         content::BrowserThread::IO);
   }