Take a Profile when adding an incident to the incident reporting service.

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc

 // Copyright 2014 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/incident_reporting/incident_reporting_service.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/process/process_info.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/prefs/tracked/tracked_preference_validation_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/database_manager.h"
 #include "chrome/browser/safe_browsing/incident_reporting/environment_data_collection.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident.h"
+#include "chrome/browser/safe_browsing/incident_reporting/incident_receiver.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_report_uploader_impl.h"
 #include "chrome/browser/safe_browsing/incident_reporting/preference_validation_delegate.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace safe_browsing {
@@ skipping 120 matching lines @@
   }
   if (types_to_remove.empty())
     return;
 
   DictionaryPrefUpdate pref_update(profile->GetPrefs(),
                                    prefs::kSafeBrowsingIncidentsSent);
   for (const auto& incident_type : types_to_remove)
     pref_update.Get()->RemoveWithoutPathExpansion(incident_type, NULL);
 }
 
-// Runs |callback| on the thread to which |thread_runner| belongs. The callback
-// is run immediately if this function is called on |thread_runner|'s thread.
-void AddIncidentOnOriginThread(
-    const AddIncidentCallback& callback,
-    scoped_refptr<base::SingleThreadTaskRunner> thread_runner,
-    scoped_ptr<Incident> incident) {
-  if (thread_runner->BelongsToCurrentThread())
-    callback.Run(incident.Pass());
-  else
-    thread_runner->PostTask(FROM_HERE,
-                            base::Bind(callback, base::Passed(&incident)));
-}
-
 }  // namespace
 
 struct IncidentReportingService::ProfileContext {
   ProfileContext();
   ~ProfileContext();
 
   // The incidents collected for this profile pending creation and/or upload.
   // Will contain null values for pruned incidents.
   ScopedVector<Incident> incidents;
 
@@ skipping 18 matching lines @@
   // The uploader in use. This is NULL until the CSD killswitch is checked.
   scoped_ptr<IncidentReportUploader> uploader;
 
   // A mapping of profiles to the data to be persisted upon successful upload.
   PersistentIncidentStateCollection profiles_to_state;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(UploadContext);
 };
 
+// An IncidentReceiver that is weakly-bound to the service and transparently
+// bounces process-wide incidents back to the main thread for handling.
+class IncidentReportingService::Receiver : public IncidentReceiver {
+ public:
+  explicit Receiver(const base::WeakPtr<IncidentReportingService>& service);
+  ~Receiver() override;
+
+  // IncidentReceiver methods:
+  void AddIncidentForProfile(Profile* profile,
+                             scoped_ptr<Incident> incident) override;
+  void AddIncidentForProcess(scoped_ptr<Incident> incident) override;
+
+ private:
+  static void AddIncidentOnUIThread(
+      const base::WeakPtr<IncidentReportingService>& service,
+      Profile* profile,
+      scoped_ptr<Incident> incident);
+
+  base::WeakPtr<IncidentReportingService> service_;
+  scoped_refptr<base::SingleThreadTaskRunner> thread_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(Receiver);
+};
+
+IncidentReportingService::Receiver::Receiver(
+    const base::WeakPtr<IncidentReportingService>& service)
+    : service_(service),
+      thread_runner_(base::ThreadTaskRunnerHandle::Get()) {
+}
+
+IncidentReportingService::Receiver::~Receiver() {
+}
+
+void IncidentReportingService::Receiver::AddIncidentForProfile(
+    Profile* profile,
+    scoped_ptr<Incident> incident) {
+  DCHECK(thread_runner_->BelongsToCurrentThread());

[robertshield, 2015/02/03 02:54:42]

For my own education: this DCHECK in practice checks that this is called on the
UI thread, so is there a reason not to check for being on the UI thread
explicitly?  It seems like not doing so (as you've done here) is nicer from a
not-as-coupled-to-specific-threading-model point of view, and yet the static
method is called AddIncident*OnUIThread* which is kind of specific. 

[grt (UTC plus 2), 2015/02/03 03:27:59]

I wish I didn't have to use BrowserThread anywhere in the impl since it leads to
tighter coupling and more weight in unit tests. I just noticed that the comment
for the class (line 208) says "main thread" rather than "UI thread," so I guess
the answer is that sometimes I'm inconsistent. I'll rename the function on line
262 so that it matches the comment and is therefore less jarring.

+  DCHECK(profile);
+  AddIncidentOnUIThread(service_, profile, incident.Pass());
+}
+
+void IncidentReportingService::Receiver::AddIncidentForProcess(
+    scoped_ptr<Incident> incident) {
+  if (thread_runner_->BelongsToCurrentThread()) {
+    AddIncidentOnUIThread(service_, nullptr, incident.Pass());
+  } else if (!thread_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&IncidentReportingService::Receiver::AddIncidentOnUIThread,
+                 service_, nullptr, base::Passed(&incident)))) {
+    LogIncidentDataType(DISCARDED, *incident);
+  }
+}
+
+// static
+void IncidentReportingService::Receiver::AddIncidentOnUIThread(
+    const base::WeakPtr<IncidentReportingService>& service,
+    Profile* profile,
+    scoped_ptr<Incident> incident) {
+  if (service)
+    service->AddIncident(profile, incident.Pass());
+  else
+    LogIncidentDataType(DISCARDED, *incident);
+}
+
 IncidentReportingService::ProfileContext::ProfileContext() : added() {
 }
 
 IncidentReportingService::ProfileContext::~ProfileContext() {
   for (Incident* incident : incidents) {
     if (incident)
       LogIncidentDataType(DISCARDED, *incident);
   }
 }
 
@@ skipping 56 matching lines @@
   // Cancel all internal asynchronous tasks.
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   CancelEnvironmentCollection();
   CancelDownloadCollection();
   CancelAllReportUploads();
 
   STLDeleteValues(&profiles_);
 }
 
-AddIncidentCallback IncidentReportingService::GetAddIncidentCallback(
-    Profile* profile) {
-  // Force the context to be created so that incidents added before
-  // OnProfileAdded is called are held until the profile's preferences can be
-  // queried.
-  ignore_result(GetOrCreateProfileContext(profile));
-
-  return base::Bind(&IncidentReportingService::AddIncident,
-                    receiver_weak_ptr_factory_.GetWeakPtr(),
-                    profile);
+scoped_ptr<IncidentReceiver> IncidentReportingService::GetIncidentReceiver() {
+  return make_scoped_ptr(new Receiver(receiver_weak_ptr_factory_.GetWeakPtr()));
 }
 
 scoped_ptr<TrackedPreferenceValidationDelegate>
 IncidentReportingService::CreatePreferenceValidationDelegate(Profile* profile) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (profile->IsOffTheRecord())
     return scoped_ptr<TrackedPreferenceValidationDelegate>();
   return scoped_ptr<TrackedPreferenceValidationDelegate>(
-      new PreferenceValidationDelegate(GetAddIncidentCallback(profile)));
+      new PreferenceValidationDelegate(profile, GetIncidentReceiver()));
 }
 
 void IncidentReportingService::RegisterDelayedAnalysisCallback(
     const DelayedAnalysisCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // |callback| will be run on the blocking pool, so it will likely run the
-  // AddIncidentCallback there as well. Bounce the run of that callback back to
-  // the current thread via AddIncidentOnOriginThread.
+  // |callback| will be run on the blocking pool. The receiver will bounce back
+  // to the origin thread if needed.
   delayed_analysis_callbacks_.RegisterCallback(
-      base::Bind(callback,
-                 base::Bind(&AddIncidentOnOriginThread,
-                            GetAddIncidentCallback(NULL),
-                            base::ThreadTaskRunnerHandle::Get())));
+      base::Bind(callback, base::Passed(GetIncidentReceiver())));
 
   // Start running the callbacks if any profiles are participating in safe
   // browsing. If none are now, running will commence if/when a participaing
   // profile is added.
   if (FindEligibleProfile())
     delayed_analysis_callbacks_.Start();
 }
 
 void IncidentReportingService::AddDownloadManager(
     content::DownloadManager* download_manager) {
@@ skipping 171 matching lines @@
       }
     }
   }
   return candidate;
 }
 
 void IncidentReportingService::AddIncident(Profile* profile,
                                            scoped_ptr<Incident> incident) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  ProfileContext* context = GetProfileContext(profile);
-  // It is forbidden to call this function with a destroyed profile.
-  DCHECK(context);
+  // Ignore incidents from off-the-record profiles.
+  if (profile && profile->IsOffTheRecord())
+    return;
+
+  ProfileContext* context = GetOrCreateProfileContext(profile);
   // If this is a process-wide incident, the context must not indicate that the
   // profile (which is NULL) has been added to the profile manager.
   DCHECK(profile || !context->added);
 
   LogIncidentDataType(RECEIVED, *incident);
 
   // Drop the incident immediately if the profile has already been added to the
   // manager and is not participating in safe browsing. Preference evaluation is
   // deferred until OnProfileAdded() otherwise.
   if (context->added &&
@@ skipping 424 matching lines @@
       if (!profile->IsOffTheRecord())
         OnProfileDestroyed(profile);
       break;
     }
     default:
       break;
   }
 }
 
 }  // namespace safe_browsing