Support for process-wide incidents in the safe browsing incident reporting service.

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 6c8ce1b9f562fb94fc295ca70e1b3993c8c8d856..50a8b0ea92af3f60de62eb14a8fc1ee0a9d3a2e7 100644
--- a/chrome/browser/safe_browsing/incident_reporting_service.cc
+++ b/chrome/browser/safe_browsing/incident_reporting_service.cc
@@ -13,8 +13,10 @@
 #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"
@@ -70,6 +72,9 @@ struct PersistentIncidentState {
 // The amount of time the service will wait to collate incidents.
 const int64 kDefaultUploadDelayMs = 1000 * 60;  // one minute
 
+// The amount of time between running delayed analysis callbacks.
+const int64 kDefaultCallbackMs = 1000 * 20;

[robertshield, 2014/08/05 21:26:28]

how about kDefaultCallbackIntervalMs ?

[grt (UTC plus 2), 2014/08/06 02:37:31]

Done.

+
 // Returns the number of incidents contained in |incident|. The result is
 // expected to be 1. Used in DCHECKs.
 size_t CountIncidents(const ClientIncidentReport_IncidentData& incident) {
@@ -157,6 +162,19 @@ void MarkIncidentsAsReported(const std::vector<PersistentIncidentState>& states,
   }
 }
 
+// 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<ClientIncidentReport_IncidentData> 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 {
@@ -220,11 +238,14 @@ IncidentReportingService::IncidentReportingService(
               ->GetTaskRunnerWithShutdownBehavior(
                   base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
       environment_collection_pending_(),
-      collection_timeout_pending_(),
-      upload_timer_(FROM_HERE,
-                    base::TimeDelta::FromMilliseconds(kDefaultUploadDelayMs),
-                    this,
-                    &IncidentReportingService::OnCollectionTimeout),
+      collation_timeout_pending_(),
+      collation_timer_(FROM_HERE,
+                       base::TimeDelta::FromMilliseconds(kDefaultUploadDelayMs),
+                       this,
+                       &IncidentReportingService::OnCollationTimeout),
+      delayed_analysis_callbacks_(
+          base::TimeDelta::FromMilliseconds(kDefaultCallbackMs),
+          content::BrowserThread::GetBlockingPool()),
       receiver_weak_ptr_factory_(this),
       weak_ptr_factory_(this) {
   notification_registrar_.Add(this,
@@ -270,6 +291,26 @@ IncidentReportingService::CreatePreferenceValidationDelegate(Profile* profile) {
       new PreferenceValidationDelegate(GetAddIncidentCallback(profile)));
 }
 
+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 main thread via AddIncidentOnOriginThread.

[robertshield, 2014/08/05 21:26:28]

s/main/current ?

[grt (UTC plus 2), 2014/08/06 02:37:31]

Done.

+  delayed_analysis_callbacks_.RegisterCallback(
+      base::Bind(callback,
+                 base::Bind(&AddIncidentOnOriginThread,
+                            GetAddIncidentCallback(NULL),
+                            base::ThreadTaskRunnerHandle::Get())));
+
+  // 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::SetCollectEnvironmentHook(
     CollectEnvironmentDataFn collect_environment_data_hook,
     const scoped_refptr<base::TaskRunner>& task_runner) {
@@ -294,17 +335,32 @@ void IncidentReportingService::OnProfileAdded(Profile* profile) {
   ProfileContext* context = GetOrCreateProfileContext(profile);
   context->added = true;
 
+  const bool safe_browsing_enabled =
+      profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled);
+
+  // Start processing delayed analysis callbacks if this new profile
+  // participates in safe browsing.

[robertshield, 2014/08/05 21:26:28]

Re-mention here how Start() is idempotent for repeated calls?

[grt (UTC plus 2), 2014/08/06 02:37:31]

Done.

+  if (safe_browsing_enabled)
+    delayed_analysis_callbacks_.Start();
+
+  // Start a new report if this profile participates in safe browsing and there
+  // are process-wide incidents.
+  if (safe_browsing_enabled && GetProfileContext(NULL))
+    BeginReportProcessing();
+
+  // TODO(grt): register for pref change notifications to start delayed analysis
+  // and/or report processing if sb is currently disabled but subsequently
+  // enabled.
+
   // Nothing else to do if a report is not being assembled.
   if (!report_)
     return;
 
-  // Drop all incidents received prior to creation if the 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) {
+  // Drop all incidents associated with this profile that were received prior to
+  // its addition if the profile is not participating in safe browsing.
+  if (!context->incidents.empty() && !safe_browsing_enabled) {
+    for (size_t i = 0; i < context->incidents.size(); ++i)
       LogIncidentDataType(DROPPED, *context->incidents[i]);
-    }
     context->incidents.clear();
   }
 
@@ -362,35 +418,49 @@ void IncidentReportingService::OnProfileDestroyed(Profile* profile) {
     uploads_[i]->profiles_to_state.erase(profile);
 }
 
+Profile* IncidentReportingService::FindEligibleProfile() const {
+  Profile* candidate = NULL;
+  for (ProfileContextCollection::const_iterator scan = profiles_.begin();
+       scan != profiles_.end();
+       ++scan) {
+    // Skip over profiles that have yet to be added to the profile manager.
+    // This will also skip over the NULL-profile context used to hold
+    // process-wide incidents.
+    if (!scan->second->added)
+      continue;
+    PrefService* prefs = scan->first->GetPrefs();
+    if (prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled)) {

[mattm, 2014/08/06 02:10:17]

Need to check prefs::kSafeBrowsingEnabled too (extended reporting flag could be
set while safe browsing is actually disabled)

[grt (UTC plus 2), 2014/08/06 02:37:31]

Done.

+      candidate = scan->first;
+      break;
+    }
+    if (!candidate && prefs->GetBoolean(prefs::kSafeBrowsingEnabled))
+      candidate = scan->first;
+  }
+  return candidate;
+}
+
 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);
   DCHECK_EQ(1U, CountIncidents(*incident_data));
 
   ProfileContext* context = GetProfileContext(profile);
   // It is forbidden to call this function with a destroyed profile.
   DCHECK(context);
+  // 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);
 
-  // Drop the incident immediately if profile creation has completed and the
-  // profile is not participating in safe browsing. Preference evaluation is
-  // deferred until OnProfileAdded() if profile creation has not yet
-  // completed.
+  // 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 &&
       !profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
     LogIncidentDataType(DROPPED, *incident_data);
     return;
   }
 
-  // Start assembling a new report if this is the first incident ever or the
-  // first since the 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 do so.
   if (!incident_data->has_incident_time_msec())
     incident_data->set_incident_time_msec(base::Time::Now().ToJavaTime());
@@ -398,6 +468,10 @@ void IncidentReportingService::AddIncident(
   // Take ownership of the incident.
   context->incidents.push_back(incident_data.release());
 
+  // Remember when the first incident for this report arrived.
+  if (first_incident_time_.is_null())
+    first_incident_time_ = base::Time::Now();
+  // Log the time between the previous incident and this one.
   if (!last_incident_time_.is_null()) {
     UMA_HISTOGRAM_TIMES("SBIRS.InterIncidentTime",
                         base::TimeTicks::Now() - last_incident_time_);
@@ -406,14 +480,30 @@ void IncidentReportingService::AddIncident(
 
   // Persist the incident data.
 
-  // Restart the delay timer to send the report upon expiration.
-  collection_timeout_pending_ = true;
-  upload_timer_.Reset();
+  // Start assembling a new report if this is the first incident ever or the
+  // first since the last upload.
+  BeginReportProcessing();
+}
+
+void IncidentReportingService::BeginReportProcessing() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Createa a new report if needed.

[mattm, 2014/08/06 02:10:17]

Createa

[grt (UTC plus 2), 2014/08/06 02:37:31]

Done.

+  if (!report_)
+    report_.reset(new ClientIncidentReport());
 
+  // Ensure that collection tasks are running (calls are idempotent).
+  BeginIncidentCollation();
   BeginEnvironmentCollection();
   BeginDownloadCollection();
 }
 
+void IncidentReportingService::BeginIncidentCollation() {
+  // Restart the delay timer to send the report upon expiration.
+  collation_timeout_pending_ = true;
+  collation_timer_.Reset();
+}
+
 void IncidentReportingService::BeginEnvironmentCollection() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(report_);
@@ -462,7 +552,6 @@ void IncidentReportingService::OnEnvironmentDataCollected(
       first_incident_time_ - base::CurrentProcessInfo::CreationTime();
   environment_data->mutable_process()->set_uptime_msec(uptime.InMilliseconds());
 #endif
-  first_incident_time_ = base::Time();
 
   report_->set_allocated_environment(environment_data.release());
 
@@ -474,34 +563,35 @@ void IncidentReportingService::OnEnvironmentDataCollected(
 }
 
 bool IncidentReportingService::WaitingToCollateIncidents() {
-  return collection_timeout_pending_;
+  return collation_timeout_pending_;
 }
 
 void IncidentReportingService::CancelIncidentCollection() {
-  collection_timeout_pending_ = false;
+  collation_timeout_pending_ = false;
   last_incident_time_ = base::TimeTicks();
   report_.reset();
 }
 
-void IncidentReportingService::OnCollectionTimeout() {
+void IncidentReportingService::OnCollationTimeout() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Exit early if collection was cancelled.
-  if (!collection_timeout_pending_)
+  if (!collation_timeout_pending_)
     return;
 
-  // Wait another round if incidents have come in from a profile that has yet to
-  // complete creation.
+  // Wait another round if profile-bound 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->added && !scan->second->incidents.empty()) {
-      upload_timer_.Reset();
+    if (scan->first && !scan->second->added &&
+        !scan->second->incidents.empty()) {
+      collation_timer_.Reset();
       return;
     }
   }
 
-  collection_timeout_pending_ = false;
+  collation_timeout_pending_ = false;
 
   UploadIfCollectionComplete();
 }
@@ -581,6 +671,7 @@ void IncidentReportingService::UploadIfCollectionComplete() {
 
   // Take ownership of the report and clear things for future reports.
   scoped_ptr<ClientIncidentReport> report(report_.Pass());
+  first_incident_time_ = base::Time();
   last_incident_time_ = base::TimeTicks();
 
   // Drop the report if no executable download was found.
@@ -601,8 +692,28 @@ void IncidentReportingService::UploadIfCollectionComplete() {
         prefs::kMetricsReportingEnabled));
   }
 
-  // Check for extended consent in any profile while collecting incidents.
-  process->set_extended_consent(false);
+  // Find a profile suitable for tracking process-wide incidents.
+  Profile* analyses_profile = FindEligibleProfile();

[mattm, 2014/08/06 02:10:17]

is "analyses" intentional here? (vs analysis)

[grt (UTC plus 2), 2014/08/06 02:37:31]

Yeah, it's the profile used for the plural analyses. Kinda confusing, huh. I've
tweaked the names and comments. What do you think?

+  process->set_extended_consent(
+      analyses_profile ? analyses_profile->GetPrefs()->GetBoolean(
+                             prefs::kSafeBrowsingExtendedReportingEnabled) :
+                       false);
+
+  // Associate process-wide incidents with the analyses profile.
+  ProfileContext* null_context = GetProfileContext(NULL);
+  if (null_context && analyses_profile) {
+    DCHECK(!null_context->incidents.empty());
+    ProfileContext* analyses_context = GetProfileContext(analyses_profile);
+    // Move the incidents to the target context.
+    analyses_context->incidents.insert(analyses_context->incidents.end(),
+                                       null_context->incidents.begin(),
+                                       null_context->incidents.end());
+    null_context->incidents.weak_clear();
+    // Delete the process-wide context.
+    delete null_context;
+    profiles_.erase(NULL);

[mattm, 2014/08/06 02:10:17]

Does this mean no more process-wide incidents can be added after this?

[grt (UTC plus 2), 2014/08/06 02:37:31]

Nice catch. Any added after this should go into a new report. The incidents in
the context should be cleared, but the context must live.

+  }
+
   // Collect incidents across all profiles participating in safe browsing. Drop
   // incidents if the profile stopped participating before collection completed.
   // Prune previously submitted incidents.
@@ -612,12 +723,11 @@ void IncidentReportingService::UploadIfCollectionComplete() {
   for (ProfileContextCollection::iterator scan = profiles_.begin();
        scan != profiles_.end();
        ++scan) {
+    // Bypass process-wide incidents that have not yet been associated with a
+    // profile.
+    if (!scan->first)
+      continue;
     PrefService* prefs = scan->first->GetPrefs();
-    if (process &&
-        prefs->GetBoolean(prefs::kSafeBrowsingExtendedReportingEnabled)) {
-      process->set_extended_consent(true);
-      process = NULL;  // Don't check any more once one is found.
-    }
     ProfileContext* context = scan->second;
     if (context->incidents.empty())
       continue;