Safe browsing incident reporting service improvements.

chrome/browser/safe_browsing/incident_reporting_service.cc

Index: chrome/browser/safe_browsing/incident_reporting_service.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting_service.cc b/chrome/browser/safe_browsing/incident_reporting_service.cc
index f0f48142e4d74c96acdc345cdd5ee922bd9465cd..6fca2a17260344696659a148958410c53b320e5a 100644
--- a/chrome/browser/safe_browsing/incident_reporting_service.cc
+++ b/chrome/browser/safe_browsing/incident_reporting_service.cc
@@ -7,16 +7,23 @@
 #include <math.h>
 
 #include "base/metrics/histogram.h"
+#include "base/prefs/pref_service.h"
 #include "base/process/process_info.h"
+#include "base/stl_util.h"
 #include "base/threading/sequenced_worker_pool.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/environment_data_collection.h"
 #include "chrome/browser/safe_browsing/incident_report_uploader_impl.h"
 #include "chrome/browser/safe_browsing/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 {
@@ -30,18 +37,48 @@ enum IncidentType {
   NUM_INCIDENT_TYPES
 };
 
+enum IncidentDisposition {
+  DROPPED,
+  ACCEPTED,
+};
+
 const int64 kDefaultUploadDelayMs = 1000 * 60;  // one minute
 
 void LogIncidentDataType(
+    IncidentDisposition disposition,
     const ClientIncidentReport_IncidentData& incident_data) {
-  if (incident_data.has_tracked_preference()) {
-    UMA_HISTOGRAM_ENUMERATION(
-        "SBIRS.Incident", TRACKED_PREFERENCE, NUM_INCIDENT_TYPES);
+  static const char kAcceptedMetric[] = "SBIRS.Incident";
+  static const char kDroppedMetric[] = "SBIRS.DroppedIncident";
+
+  IncidentType type = TRACKED_PREFERENCE;
+
+  // Add a switch statement once other types are supported.
+  DCHECK(incident_data.has_tracked_preference());
+
+  if (disposition == ACCEPTED) {
+    UMA_HISTOGRAM_ENUMERATION(kAcceptedMetric, type, NUM_INCIDENT_TYPES);
+  } else {
+    DCHECK_EQ(disposition, DROPPED);
+    UMA_HISTOGRAM_ENUMERATION(kDroppedMetric, type, NUM_INCIDENT_TYPES);
   }
 }
 
 }  // namespace
 
+struct IncidentReportingService::ProfileContext {
+  ProfileContext();
+  ~ProfileContext();
+
+  // The incidents collected for this profile pending creation and/or upload.
+  ScopedVector<ClientIncidentReport_IncidentData> incidents;
+
+  // False until PROFILE_CREATED notification is received.
+  bool created;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ProfileContext);
+};
+
 class IncidentReportingService::UploadContext {
  public:
   explicit UploadContext(scoped_ptr<ClientIncidentReport> report);
@@ -54,6 +91,12 @@ class IncidentReportingService::UploadContext {
   DISALLOW_COPY_AND_ASSIGN(UploadContext);
 };
 
+IncidentReportingService::ProfileContext::ProfileContext() : created() {
+}
+
+IncidentReportingService::ProfileContext::~ProfileContext() {
+}
+
 IncidentReportingService::UploadContext::UploadContext(
     scoped_ptr<ClientIncidentReport> report)
     : report(report.Pass()) {
@@ -73,7 +116,6 @@ IncidentReportingService::IncidentReportingService(
           content::BrowserThread::GetBlockingPool()
               ->GetTaskRunnerWithShutdownBehavior(
                   base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
-      enabled_(),
       environment_collection_pending_(),
       collection_timeout_pending_(),
       upload_timer_(FROM_HERE,
@@ -82,39 +124,46 @@ IncidentReportingService::IncidentReportingService(
                     &IncidentReportingService::OnCollectionTimeout),
       receiver_weak_ptr_factory_(this),
       weak_ptr_factory_(this) {
+  notification_registrar_.Add(this,
+                              chrome::NOTIFICATION_PROFILE_CREATED,
+                              content::NotificationService::AllSources());
+  notification_registrar_.Add(this,
+                              chrome::NOTIFICATION_PROFILE_DESTROYED,
+                              content::NotificationService::AllSources());
 }
 
 IncidentReportingService::~IncidentReportingService() {
-  if (enabled_)
-    SetEnabled(false);
-}
-
-void IncidentReportingService::SetEnabled(bool enabled) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  if (!enabled) {
-    CancelIncidentCollection();
+  CancelIncidentCollection();
 
-    // Cancel all internal asynchronous tasks.
-    weak_ptr_factory_.InvalidateWeakPtrs();
+  // Cancel all internal asynchronous tasks.
+  weak_ptr_factory_.InvalidateWeakPtrs();
 
-    CancelEnvironmentCollection();
-    CancelAllReportUploads();
-  }
+  CancelEnvironmentCollection();
+  CancelAllReportUploads();
 
-  enabled_ = enabled;
+  STLDeleteValues(&profiles_);
 }
 
-AddIncidentCallback IncidentReportingService::GetAddIncidentCallback() {
+AddIncidentCallback IncidentReportingService::GetAddIncidentCallback(
+    Profile* profile) {
+  // Force the context to be created so that incidents added before
+  // OnProfileCreated 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());
+                    receiver_weak_ptr_factory_.GetWeakPtr(),
+                    profile);
 }
 
 scoped_ptr<TrackedPreferenceValidationDelegate>
-IncidentReportingService::CreatePreferenceValidationDelegate() {
+IncidentReportingService::CreatePreferenceValidationDelegate(Profile* profile) {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (profile->IsOffTheRecord())
+    return scoped_ptr<TrackedPreferenceValidationDelegate>();
   return scoped_ptr<TrackedPreferenceValidationDelegate>(
-      new PreferenceValidationDelegate(GetAddIncidentCallback()));
+      new PreferenceValidationDelegate(GetAddIncidentCallback(profile)));
 }
 
 void IncidentReportingService::SetCollectEnvironmentHook(
@@ -145,22 +194,86 @@ scoped_ptr<IncidentReportUploader> IncidentReportingService::StartReportUpload(
 #endif
 }
 
+IncidentReportingService::ProfileContext*
+IncidentReportingService::GetOrCreateProfileContext(Profile* profile) {
+  ProfileContextCollection::iterator it =
+      profiles_.insert(ProfileContextCollection::value_type(profile, NULL))
+          .first;
+  if (!it->second)
+    it->second = new ProfileContext();
+  return it->second;
+}
+
+IncidentReportingService::ProfileContext*
+IncidentReportingService::GetProfileContext(Profile* profile) {
+  ProfileContextCollection::iterator it = profiles_.find(profile);
+  return it == profiles_.end() ? NULL : it->second;
+}
+
+void IncidentReportingService::OnProfileCreated(Profile* profile) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ProfileContext* context = GetOrCreateProfileContext(profile);
+  context->created = true;
+
+  // Drop all incidents if this profile is not participating in safe browsing.
+  if (!context->incidents.empty() &&
+      !profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    for (size_t i = 0; i < context->incidents.size(); ++i) {
+      LogIncidentDataType(DROPPED, *context->incidents[i]);
+    }
+    context->incidents.clear();
+  }
+}
+
+void IncidentReportingService::OnProfileDestroyed(Profile* profile) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  ProfileContextCollection::iterator it = profiles_.find(profile);
+  if (it == profiles_.end())
+    return;
+
+  // TODO(grt): Persist incidents for upload on future profile load.
+
+  // Forget about this profile. Incidents not yet sent for upload are lost.
+  // No new incidents will be accepted for it.
+  delete it->second;
+  profiles_.erase(it);
+}
+
 void IncidentReportingService::AddIncident(
+    Profile* profile,
     scoped_ptr<ClientIncidentReport_IncidentData> incident_data) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  // Incidents outside the context of a profile are not supported at the moment.
+  DCHECK(profile);
+
+  ProfileContext* context = GetProfileContext(profile);
+  // The profile is unknown if it has already been destroyed.

[mattm, 2014/06/18 21:07:18]

Probably not safe to allow this to be called after the profile has been
destroyed (what if one profile gets destroyed and another gets created at the
same address?)

[grt (UTC plus 2), 2014/06/18 22:47:58]

The only case where GetProfileContext would return NULL is if it were called
with a profile after NOTIFICATION_PROFILE_DESTROYED had been sent for the
Profile. This should never happen. Is the Chromium-y thing to do to let the
process crash when the NULL pointer is dereferenced so we see the impossible
appear in crash reports (and therefore remove lines 252-254)?

[mattm, 2014/06/18 23:05:38]

yeah, that sounds good. You could replace it with a DCHECK to make it explicit.

[grt (UTC plus 2), 2014/06/19 02:24:39]

Done.

+  if (!context)
+    return;
 
-  // TODO(grt): Don't ignore incidents that arrive before
-  // NOTIFICATION_PROFILE_CREATED; http://crbug.com/383365.
-  if (!enabled_)
+  // Drop the incident immediately if profile creation has completed and the
+  // profile is not participating in safe browsing.
+  if (context->created &&
+      !profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    LogIncidentDataType(DROPPED, *incident_data);
     return;
+  }
 
+  // Create a new report if this is the first incident ever or first since last
+  // upload.
   if (!report_) {
     report_.reset(new ClientIncidentReport());
     first_incident_time_ = base::Time::Now();
   }
+
+  // Provide time to the new incident if the caller didn't provide it.
   if (!incident_data->has_incident_time_msec())
     incident_data->set_incident_time_msec(base::Time::Now().ToJavaTime());
-  report_->mutable_incident()->AddAllocated(incident_data.release());
+
+  // Take ownership of the incident.
+  context->incidents.push_back(incident_data.release());
 
   if (!last_incident_time_.is_null()) {
     UMA_HISTOGRAM_TIMES("SBIRS.InterIncidentTime",
@@ -243,6 +356,17 @@ void IncidentReportingService::OnCollectionTimeout() {
   if (!collection_timeout_pending_)
     return;
 
+  // Wait another round if incidents have come in from a profile that has yet to
+  // complete creation.
+  for (ProfileContextCollection::iterator scan = profiles_.begin();
+       scan != profiles_.end();
+       ++scan) {
+    if (!scan->second->created && !scan->second->incidents.empty()) {
+      upload_timer_.Reset();
+      return;
+    }
+  }
+
   collection_timeout_pending_ = false;
 
   UploadIfCollectionComplete();
@@ -273,14 +397,55 @@ void IncidentReportingService::UploadIfCollectionComplete() {
   scoped_ptr<ClientIncidentReport> report(report_.Pass());
   last_incident_time_ = base::TimeTicks();
 
+  ClientIncidentReport_EnvironmentData_Process* process =
+      report->mutable_environment()->mutable_process();
+
+  // Not all platforms have a metrics reporting preference.
+  if (g_browser_process->local_state()->FindPreference(
+          prefs::kMetricsReportingEnabled)) {
+    process->set_metrics_consent(g_browser_process->local_state()->GetBoolean(
+        prefs::kMetricsReportingEnabled));
+  }
+  // Check for extended consent in any profile while collecting incidents.
+  process->set_extended_consent(false);
+  // Collect incidents across all profiles participating in safe browsing. Drop
+  // incidents if the profile stopped participating before collection completed.
+  for (ProfileContextCollection::iterator scan = profiles_.begin();
+       scan != profiles_.end();
+       ++scan) {
+    PrefService* prefs = scan->first->GetPrefs();
+    if (process &&
+        prefs->GetBoolean(prefs::kSafeBrowsingDownloadFeedbackEnabled)) {

[mattm, 2014/06/18 21:07:17]

kSafeBrowsingExtendedReportingEnabled now

[grt (UTC plus 2), 2014/06/18 22:47:58]

thanks for the pointer.

[grt (UTC plus 2), 2014/06/19 02:24:39]

Done.

+      process->set_extended_consent(true);

[mattm, 2014/06/18 21:07:18]

is it okay that this isn't sent on a per-profile/incident basis?

[grt (UTC plus 2), 2014/06/18 22:47:58]

it is, thanks for checking.

+      process = NULL;  // Don't check any more once one is found.
+    }
+    ProfileContext* context = scan->second;
+    if (context->incidents.empty())
+      continue;
+    if (prefs->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+      for (size_t i = 0; i < context->incidents.size(); ++i) {
+        ClientIncidentReport_IncidentData* incident = context->incidents[i];
+        LogIncidentDataType(ACCEPTED, *incident);
+        // Ownership of the incident is passed to the report.
+        report->mutable_incident()->AddAllocated(incident);
+      }
+      context->incidents.weak_clear();
+    } else {
+      for (size_t i = 0; i < context->incidents.size(); ++i) {
+        LogIncidentDataType(DROPPED, *context->incidents[i]);
+      }
+      context->incidents.clear();
+    }
+  }
+
   const int original_count = report->incident_size();
+  // Abandon the request if all incidents were dropped.
+  if (!original_count)
+    return;
+
   UMA_HISTOGRAM_COUNTS_100("SBIRS.IncidentCount", original_count);
-  for (int i = 0; i < original_count; ++i) {
-    LogIncidentDataType(report->incident(i));
-  }
 
   PruneReportedIncidents(report.get());
-  // TODO(grt): prune incidents from opted-out profiles; http://crbug.com/383039
 
   int final_count = report->incident_size();
   {
@@ -294,8 +459,6 @@ void IncidentReportingService::UploadIfCollectionComplete() {
     return;
 
   scoped_ptr<UploadContext> context(new UploadContext(report.Pass()));
-  scoped_refptr<base::RefCountedData<bool> > is_killswitch_on(
-      new base::RefCountedData<bool>(false));
   if (!database_manager_) {
     // No database manager during testing. Take ownership of the context and
     // continue processing.
@@ -378,4 +541,26 @@ void IncidentReportingService::OnReportUploadResult(
   // else retry?
 }
 
+void IncidentReportingService::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_PROFILE_CREATED: {
+      Profile* profile = content::Source<Profile>(source).ptr();
+      if (!profile->IsOffTheRecord())
+        OnProfileCreated(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