Remove NotificationService notifications from IncidentReportingService

NOTIFICATION_PROFILE_DESTROYED was only used for normal (on the record)
profiles. But normal profiles aren't destroyed until browser shutdown,
and are destroyed after SafeBrowsingService is destroyed, so there is
no need to listen to this notification.

NOTIFICATION_PROFILE_ADDED can be replaced with
ProfileManagerObserver::OnProfileAdded.

Also, delegate between the two constructors (prod and test), so
code needn't be duplicated between them.

Bug: 268984
Change-Id: Iab614421b7c51136ccae79a45d00f8ee709a73b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1832883
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703511}

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc

@@ -24,9 +24,10 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/branding_buildflags.h"
 #include "build/build_config.h"
-#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/metrics/chrome_metrics_service_accessor.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/safe_browsing/incident_reporting/environment_data_collection.h"
 #include "chrome/browser/safe_browsing/incident_reporting/extension_data_collection.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident.h"
@@ -42,7 +43,6 @@
 #include "components/safe_browsing/proto/csd.pb.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item_utils.h"
-#include "content/public/browser/notification_service.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h"
 
@@ -156,11 +156,10 @@ struct IncidentReportingService::ProfileContext {
   // The incidents data of which should be cleared.
   std::vector<std::unique_ptr<Incident>> incidents_to_clear;
 
-  // State storage for this profile; null until PROFILE_ADDED notification is
-  // received.
+  // State storage for this profile; null until OnProfileAdded is called.
   std::unique_ptr<StateStore> state_store;
 
-  // False until PROFILE_ADDED notification is received.
+  // False until OnProfileAdded is called.
   bool added;
 
  private:
@@ -321,27 +320,11 @@ bool IncidentReportingService::IsEnabledForProfile(Profile* profile) {
 
 IncidentReportingService::IncidentReportingService(
     SafeBrowsingService* safe_browsing_service)
-    : url_loader_factory_(safe_browsing_service
-                              ? safe_browsing_service->GetURLLoaderFactory()
-                              : nullptr),
-      collect_environment_data_fn_(&CollectEnvironmentData),
-      environment_collection_task_runner_(GetBackgroundTaskRunner()),
-      environment_collection_pending_(),
-      collation_timeout_pending_(),
-      collation_timer_(FROM_HERE,
-                       base::TimeDelta::FromMilliseconds(kDefaultUploadDelayMs),
-                       this,
-                       &IncidentReportingService::OnCollationTimeout),
-      delayed_analysis_callbacks_(
+    : IncidentReportingService(
+          safe_browsing_service,
           base::TimeDelta::FromMilliseconds(kDefaultCallbackIntervalMs),
           GetBackgroundTaskRunner()) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_ADDED,
-                              content::NotificationService::AllSources());
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_DESTROYED,
-                              content::NotificationService::AllSources());
   DownloadProtectionService* download_protection_service =
       (safe_browsing_service
            ? safe_browsing_service->download_protection_service()
@@ -364,6 +347,9 @@ IncidentReportingService::~IncidentReportingService() {
   CancelEnvironmentCollection();
   CancelDownloadCollection();
   CancelAllReportUploads();
+
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->RemoveObserver(this);
 }
 
 std::unique_ptr<IncidentReceiver>
@@ -406,21 +392,18 @@ IncidentReportingService::IncidentReportingService(
     SafeBrowsingService* safe_browsing_service,
     base::TimeDelta delayed_task_interval,
     const scoped_refptr<base::TaskRunner>& delayed_task_runner)
-    : collect_environment_data_fn_(&CollectEnvironmentData),
+    : url_loader_factory_(safe_browsing_service
+                              ? safe_browsing_service->GetURLLoaderFactory()
+                              : nullptr),
+      collect_environment_data_fn_(&CollectEnvironmentData),
       environment_collection_task_runner_(GetBackgroundTaskRunner()),
-      environment_collection_pending_(),
-      collation_timeout_pending_(),
       collation_timer_(FROM_HERE,
                        base::TimeDelta::FromMilliseconds(kDefaultUploadDelayMs),
                        this,
                        &IncidentReportingService::OnCollationTimeout),
       delayed_analysis_callbacks_(delayed_task_interval, delayed_task_runner) {
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_ADDED,
-                              content::NotificationService::AllSources());
-  notification_registrar_.Add(this,
-                              chrome::NOTIFICATION_PROFILE_DESTROYED,
-                              content::NotificationService::AllSources());
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->AddObserver(this);
 }
 
 void IncidentReportingService::SetCollectEnvironmentHook(
@@ -441,6 +424,13 @@ void IncidentReportingService::DoExtensionCollection(
 }
 
 void IncidentReportingService::OnProfileAdded(Profile* profile) {
+  // Handle the addition of a new profile to the ProfileManager. Create a new
+  // context for |profile| if one does not exist, drop any received incidents
+  // for the profile if the profile is not participating in safe browsing
+  // extended reporting, and initiate a new search for the most recent download
+  // if a report is being assembled and the most recent has not been found.
+  // Note that |profile| is assumed to outlive |this|.
+
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Track the addition of all profiles even when no report is being assembled
@@ -449,7 +439,7 @@ void IncidentReportingService::OnProfileAdded(Profile* profile) {
   ProfileContext* context = GetOrCreateProfileContext(profile);
   DCHECK(!context->added);
   context->added = true;
-  context->state_store.reset(new StateStore(profile));
+  context->state_store = std::make_unique<StateStore>(profile);
   bool enabled_for_profile = IsEnabledForProfile(profile);
 
   // Drop all incidents associated with this profile that were received prior to
@@ -528,28 +518,6 @@ IncidentReportingService::GetProfileContext(Profile* profile) {
   return it != profiles_.end() ? it->second.get() : nullptr;
 }
 
-void IncidentReportingService::OnProfileDestroyed(Profile* profile) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  auto it = profiles_.find(profile);
-  if (it == profiles_.end())
-    return;
-
-  // Take ownership of the context.
-  std::unique_ptr<ProfileContext> context = std::move(it->second);
-  it->second = nullptr;
-
-  // TODO(grt): Persist incidents for upload on future profile load.
-
-  // Remove the association with this profile context from all pending uploads.
-  for (const auto& upload : uploads_)
-    upload->profiles_to_state.erase(context.get());
-
-  // Forget about this profile. Incidents not yet sent for upload are lost.
-  // No new incidents will be accepted for it.
-  profiles_.erase(it);
-}
-
 Profile* IncidentReportingService::FindEligibleProfile() const {
   for (const auto& scan : profiles_) {
     // Skip over profiles that have yet to be added to the profile manager.
@@ -1010,26 +978,4 @@ void IncidentReportingService::OnClientDownloadRequest(
   }
 }
 
-void IncidentReportingService::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  switch (type) {
-    case chrome::NOTIFICATION_PROFILE_ADDED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (!profile->IsOffTheRecord())
-        OnProfileAdded(profile);
-      break;
-    }
-    case chrome::NOTIFICATION_PROFILE_DESTROYED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (!profile->IsOffTheRecord())
-        OnProfileDestroyed(profile);
-      break;
-    }
-    default:
-      break;
-  }
-}
-
 }  // namespace safe_browsing

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h

@@ -18,6 +18,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_service.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_util.h"
 #include "chrome/browser/safe_browsing/incident_reporting/delayed_analysis_callback.h"
@@ -25,8 +26,6 @@
 #include "chrome/browser/safe_browsing/incident_reporting/download_metadata_manager.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_report_uploader.h"
 #include "chrome/browser/safe_browsing/incident_reporting/last_download_finder.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 
 class Profile;
 
@@ -36,8 +35,6 @@ class TaskRunner;
 
 namespace content {
 class DownloadManager;
-class NotificationDetails;
-class NotificationSource;
 }
 
 namespace network {
@@ -77,7 +74,7 @@ class SafeBrowsingService;
 // the initial incident. Finally, already-reported incidents are pruned and any
 // remaining are uploaded in an incident report.
 // Lives on the UI thread.
-class IncidentReportingService : public content::NotificationObserver {
+class IncidentReportingService : public ProfileManagerObserver {
  public:
   explicit IncidentReportingService(SafeBrowsingService* safe_browsing_service);
 
@@ -107,6 +104,9 @@ class IncidentReportingService : public content::NotificationObserver {
   // storage.
   void AddDownloadManager(content::DownloadManager* download_manager);
 
+  // ProfileManagerObserver:
+  void OnProfileAdded(Profile* profile) override;
+
  protected:
   // A pointer to a function that populates a protobuf with environment data.
   typedef void (*CollectEnvironmentDataFn)(
@@ -131,14 +131,6 @@ class IncidentReportingService : public content::NotificationObserver {
   virtual void DoExtensionCollection(
       ClientIncidentReport_ExtensionData* extension_data);
 
-  // Handles the addition of a new profile to the ProfileManager. Creates a new
-  // context for |profile| if one does not exist, drops any received incidents
-  // for the profile if the profile is not participating in safe browsing
-  // extended reporting, and initiates a new search for the most recent download
-  // if a report is being assembled and the most recent has not been found.
-  // Overridden by unit tests to inject incidents prior to creation.
-  virtual void OnProfileAdded(Profile* profile);
-
   // Initiates a search for the most recent binary download. Overriden by unit
   // tests to provide a fake finder.
   virtual std::unique_ptr<LastDownloadFinder> CreateDownloadFinder(
@@ -163,10 +155,6 @@ class IncidentReportingService : public content::NotificationObserver {
   // Returns the context for |profile|, or NULL if it is unknown.
   ProfileContext* GetProfileContext(Profile* profile);
 
-  // Handles the destruction of a profile. Incidents reported for the profile
-  // but not yet uploaded are dropped.
-  void OnProfileDestroyed(Profile* profile);
-
   // Returns an initialized profile for which incident reporting is enabled.
   Profile* FindEligibleProfile() const;
 
@@ -264,11 +252,6 @@ class IncidentReportingService : public content::NotificationObserver {
   void OnClientDownloadRequest(download::DownloadItem* download,
                                const ClientDownloadRequest* request);
 
-  // content::NotificationObserver methods.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
   // Accessor for an URLLoaderFactory with which reports will be sent.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
@@ -283,20 +266,17 @@ class IncidentReportingService : public content::NotificationObserver {
   // collection task at shutdown if it has not yet started.
   scoped_refptr<base::TaskRunner> environment_collection_task_runner_;
 
-  // Registrar for observing profile lifecycle notifications.
-  content::NotificationRegistrar notification_registrar_;
-
   // A subscription for ClientDownloadRequests, used to persist them for later
   // use.
   ClientDownloadRequestSubscription client_download_request_subscription_;
 
   // True when the asynchronous environment collection task has been fired off
   // but has not yet completed.
-  bool environment_collection_pending_;
+  bool environment_collection_pending_ = false;
 
   // True when an incident has been received and the service is waiting for the
   // collation_timer_ to fire.
-  bool collation_timeout_pending_;
+  bool collation_timeout_pending_ = false;
 
   // A timer upon the firing of which the service will report received
   // incidents.

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc

@@ -156,7 +156,7 @@ class IncidentReportingServiceTest : public testing::Test {
   };
 
   // A type for specifying the action to be taken by the test fixture during
-  // profile initialization (before NOTIFICATION_PROFILE_ADDED is sent).
+  // profile initialization (before OnProfileAdded is called).
   enum OnProfileAdditionAction {
     ON_PROFILE_ADDITION_NO_ACTION,
     ON_PROFILE_ADDITION_ADD_INCIDENT,  // Add an incident to the service.
@@ -466,7 +466,7 @@ class IncidentReportingServiceTest : public testing::Test {
     ProfileProperties() : on_addition_action(ON_PROFILE_ADDITION_NO_ACTION) {}
 
     // The action taken by the test fixture during profile initialization
-    // (before NOTIFICATION_PROFILE_ADDED is sent).
+    // (before OnProfileAdded is called).
     OnProfileAdditionAction on_addition_action;
   };