More fine-grained pruning in safe browsing incident reporting service.

chrome/browser/safe_browsing/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_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/stl_util.h"
+#include "base/strings/string_number_conversions.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/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/browser/safe_browsing/tracked_preference_incident_handlers.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 {
 
 namespace {
 
+// The type of an incident. Used for user metrics and for pruning of
+// previously-reported incidents.
 enum IncidentType {
   // Start with 1 rather than zero; otherwise there won't be enough buckets for
   // the histogram.
   TRACKED_PREFERENCE = 1,
+  // Values for new incident types go here.
   NUM_INCIDENT_TYPES
 };
 
+// The action taken for an incident; used for user metrics (see
+// LogIncidentDataType).
 enum IncidentDisposition {
   DROPPED,
   ACCEPTED,
 };
 
+// The state persisted for a specific instance of an incident to enable pruning
+// of previously-reported incidents.
+struct PersistentIncidentState {
+  // The type of the incident.
+  IncidentType type;
+
+  // The key for a specific instance of an incident.
+  std::string key;
+
+  // A hash digest representing a specific instance of an incident.
+  uint32_t digest;
+};
+
+// Handlers for a specific type of incident.
+struct IncidentTypeHandlers {
+  // The type of incident to which these handlers apply.
+  IncidentType type;
+
+  // A pointer to the has_*() method on IncidentData that tests for the presence
+  // of the incident payload for this type.
+  bool (ClientIncidentReport_IncidentData::*has_type_fn)(void) const;
+
+  // A function that returns a unique key representing an incident.
+  std::string (*get_key_fn)(const ClientIncidentReport_IncidentData&);
+
+  // A function that returns a hash digest of an incident's data.
+  uint32_t (*get_digest_fn)(const ClientIncidentReport_IncidentData&);
+};
+
+// The collection of handlers for all incident types.
+const IncidentTypeHandlers kIncidentTypeHandlers[] = {
+    {TRACKED_PREFERENCE,
+     &ClientIncidentReport_IncidentData::has_tracked_preference,
+     &GetTrackedPreferenceIncidentKey,
+     &GetTrackedPreferenceIncidentDigest},
+    // Handler definitions for new incident types go here.
+};
+
+// The amount of time the service will wait to collate incidents.
 const int64 kDefaultUploadDelayMs = 1000 * 60;  // one minute
 
+// 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) {
+  size_t result = 0;
+  for (size_t i = 0; i < arraysize(kIncidentTypeHandlers); ++i) {
+    if ((incident.*kIncidentTypeHandlers[i].has_type_fn)())

[Scott Hess - ex-Googler, 2014/07/24 21:22:20]

This is wow.  And I'm not sure there are alternatives.  Maybe

  (incident.(*kIncidentTypeHandlers[i].has_type_fn))()

I think that's still a little wiggly, because "has_type_fn" feels like a boolean
setting, but I'm not sure what else wouldn't feel like that.  Or perhaps

  (*kIncidentTypeHandlers[i].has_type_fn)(incident);

which is pretty clear about the function call.  Or perhaps leave it alone, it's
a thing you can do in C++, and it does what you need to do...

[grt (UTC plus 2), 2014/07/24 22:58:12]

i was making use of the .* operator here which takes an instance on the lhs and
a pointer to a member on the rhs. a valid alternative would be to wrap
kIncidentTypeHandlers[i].has_type_fn in parens:

  (incident.*(kIncidentTypeHandlers[i].has_type_fn))()

moot now since i've removed this.

+      ++result;
+  }
+  return result;
+}
+
+// Returns the type of incident contained in |incident_data|.
+IncidentType GetIncidentType(
+    const ClientIncidentReport_IncidentData& incident_data) {
+  for (size_t i = 0; i < arraysize(kIncidentTypeHandlers); ++i) {
+    if ((incident_data.*kIncidentTypeHandlers[i].has_type_fn)())
+      return kIncidentTypeHandlers[i].type;
+  }
+  NOTREACHED();
+  return NUM_INCIDENT_TYPES;
+}
+
+// Returns the handlers for a given type of incident.
+const IncidentTypeHandlers* GetIncidentTypeHandlers(IncidentType type) {
+  for (size_t i = 0; i < arraysize(kIncidentTypeHandlers); ++i) {
+    if (kIncidentTypeHandlers[i].type == type)
+      return &kIncidentTypeHandlers[i];
+  }
+  NOTREACHED();
+  return NULL;
+}
+
+// Logs the type of incident in |incident_data| to a user metrics histogram.
 void LogIncidentDataType(
     IncidentDisposition disposition,
     const ClientIncidentReport_IncidentData& incident_data) {
-  IncidentType type = TRACKED_PREFERENCE;
-
-  // Add a switch statement once other types are supported.
-  DCHECK(incident_data.has_tracked_preference());
-
+  IncidentType type = GetIncidentType(incident_data);
   if (disposition == ACCEPTED) {
     UMA_HISTOGRAM_ENUMERATION("SBIRS.Incident", type, NUM_INCIDENT_TYPES);
   } else {
     DCHECK_EQ(disposition, DROPPED);
     UMA_HISTOGRAM_ENUMERATION("SBIRS.DroppedIncident", type,
                               NUM_INCIDENT_TYPES);
   }
 }
 
+// Computes the persistent state for an incident.
+PersistentIncidentState ComputeIncidentState(
+    const ClientIncidentReport_IncidentData& incident) {
+  PersistentIncidentState state;
+
+  state.type = GetIncidentType(incident);
+  const IncidentTypeHandlers* handlers = GetIncidentTypeHandlers(state.type);
+  state.key = handlers->get_key_fn(incident);
+  state.digest = handlers->get_digest_fn(incident);
+
+  return state;
+}
+
+// Returns true if the incident described by |state| has already been reported
+// based on the bookkeeping in the |incidents_sent| preference dictionary.
+bool IncidentHasBeenReported(const base::DictionaryValue* incidents_sent,
+                             const PersistentIncidentState& state) {
+  const base::DictionaryValue* type_dict = NULL;
+  std::string digest_string;
+  return (incidents_sent &&
+          incidents_sent->GetDictionaryWithoutPathExpansion(
+              base::IntToString(state.type), &type_dict) &&
+          type_dict->GetStringWithoutPathExpansion(state.key, &digest_string) &&
+          digest_string == base::UintToString(state.digest));
+}
+
+// Marks the incidents described by |states| as having been reported
+// in |incidents_set|.
+void MarkIncidentsAsReported(const std::vector<PersistentIncidentState>& states,
+                             base::DictionaryValue* incidents_sent) {
+  for (size_t i = 0; i < states.size(); ++i) {
+    const PersistentIncidentState& data = states[i];
+    base::DictionaryValue* type_dict = NULL;
+    const std::string type_string(base::IntToString(data.type));
+    if (!incidents_sent->GetDictionaryWithoutPathExpansion(type_string,
+                                                           &type_dict)) {
+      type_dict = new base::DictionaryValue();
+      incidents_sent->SetWithoutPathExpansion(type_string, type_dict);
+    }
+    type_dict->SetStringWithoutPathExpansion(data.key,
+                                             base::UintToString(data.digest));
+  }
+}
+
 }  // namespace
 
 struct IncidentReportingService::ProfileContext {
   ProfileContext();
   ~ProfileContext();
 
   // The incidents collected for this profile pending creation and/or upload.
   ScopedVector<ClientIncidentReport_IncidentData> incidents;
 
   // False until PROFILE_ADDED notification is received.
   bool added;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ProfileContext);
 };
 
 class IncidentReportingService::UploadContext {
  public:
+  typedef std::map<Profile*, std::vector<PersistentIncidentState> >
+      PersistentIncidentStateCollection;
+
   explicit UploadContext(scoped_ptr<ClientIncidentReport> report);
   ~UploadContext();
 
   // The report being uploaded.
   scoped_ptr<ClientIncidentReport> report;
 
   // The uploader in use. This is NULL until the CSD killswitch is checked.
   scoped_ptr<IncidentReportUploader> uploader;
 
-  // The set of profiles from which incidents in |report| originated.
-  std::vector<Profile*> profiles;
+  // A mapping of profiles to the data to be persisted upon successful upload.
+  PersistentIncidentStateCollection profiles_to_state;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(UploadContext);
 };
 
 IncidentReportingService::ProfileContext::ProfileContext() : added() {
 }
 
 IncidentReportingService::ProfileContext::~ProfileContext() {
 }
@@ skipping 148 matching lines @@
   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);
 
-  // Remove the association with this profile from any pending uploads.
-  for (size_t i = 0; i < uploads_.size(); ++i) {
-    UploadContext* upload = uploads_[i];
-    std::vector<Profile*>::iterator it =
-        std::find(upload->profiles.begin(), upload->profiles.end(), profile);
-    if (it != upload->profiles.end()) {
-      *it = upload->profiles.back();
-      upload->profiles.resize(upload->profiles.size() - 1);
-      break;
-    }
-  }
+  // Remove the association with this profile from all pending uploads.
+  for (size_t i = 0; i < uploads_.size(); ++i)
+    uploads_[i]->profiles_to_state.erase(profile);
 }
 
 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);
 
   // 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.
   if (context->added &&
@@ skipping 216 matching lines @@
   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.
-  // Prune incidents if the profile has already submitted any incidents.
-  // Associate the participating profiles with the upload.
+  // Prune previously submitted incidents.
+  // Associate the profiles and their incident data with the upload.
   size_t prune_count = 0;
-  std::vector<Profile*> profiles;
+  UploadContext::PersistentIncidentStateCollection profiles_to_state;
   for (ProfileContextCollection::iterator scan = profiles_.begin();
        scan != profiles_.end();
        ++scan) {
     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;
     if (!prefs->GetBoolean(prefs::kSafeBrowsingEnabled)) {
       for (size_t i = 0; i < context->incidents.size(); ++i) {
         LogIncidentDataType(DROPPED, *context->incidents[i]);
       }
       context->incidents.clear();
-    } else if (prefs->GetBoolean(prefs::kSafeBrowsingIncidentReportSent)) {
-      // Prune all incidents.
-      // TODO(grt): Only prune previously submitted incidents;
-      // http://crbug.com/383043.
-      prune_count += context->incidents.size();
+      continue;
+    }
+    std::vector<PersistentIncidentState> states;
+    const base::DictionaryValue* incidents_sent =
+        prefs->GetDictionary(prefs::kSafeBrowsingIncidentsSent);
+    // Prep persistent data and prune any incidents already sent.
+    for (size_t i = 0; i < context->incidents.size(); ++i) {
+      ClientIncidentReport_IncidentData* incident = context->incidents[i];
+      const PersistentIncidentState state = ComputeIncidentState(*incident);
+      if (IncidentHasBeenReported(incidents_sent, state)) {
+        ++prune_count;
+        delete context->incidents[i];
+        context->incidents[i] = NULL;
+      } else {
+        states.push_back(state);
+      }
+    }
+    if (prefs->GetBoolean(prefs::kSafeBrowsingIncidentReportSent)) {
+      // Prune all incidents as if they had been reported, migrating to the new
+      // technique. TODO(grt): remove this branch after it has shipped.
+      for (size_t i = 0; i < context->incidents.size(); ++i) {
+        if (context->incidents[i])
+          ++prune_count;
+      }
       context->incidents.clear();
+      prefs->ClearPref(prefs::kSafeBrowsingIncidentReportSent);
+      DictionaryPrefUpdate pref_update(prefs,
+                                       prefs::kSafeBrowsingIncidentsSent);
+      MarkIncidentsAsReported(states, pref_update.Get());
     } else {
       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);
+        if (incident) {
+          LogIncidentDataType(ACCEPTED, *incident);
+          // Ownership of the incident is passed to the report.
+          report->mutable_incident()->AddAllocated(incident);
+        }
       }
       context->incidents.weak_clear();
-      profiles.push_back(scan->first);
+      std::vector<PersistentIncidentState>& profile_states =
+          profiles_to_state[scan->first];
+      profile_states.swap(states);
     }
   }
 
   const int count = report->incident_size();
   // Abandon the request if all incidents were dropped with none pruned.
   if (!count && !prune_count)
     return;
 
   UMA_HISTOGRAM_COUNTS_100("SBIRS.IncidentCount", count + prune_count);
 
   {
     double prune_pct = static_cast<double>(prune_count);
     prune_pct = prune_pct * 100.0 / (count + prune_count);
     prune_pct = round(prune_pct);
     UMA_HISTOGRAM_PERCENTAGE("SBIRS.PruneRatio", static_cast<int>(prune_pct));
   }
   // Abandon the report if all incidents were pruned.
   if (!count)
     return;
 
   scoped_ptr<UploadContext> context(new UploadContext(report.Pass()));
-  context->profiles.swap(profiles);
+  context->profiles_to_state.swap(profiles_to_state);
   if (!database_manager_) {
     // No database manager during testing. Take ownership of the context and
     // continue processing.
     UploadContext* temp_context = context.get();
     uploads_.push_back(context.release());
     IncidentReportingService::OnKillSwitchResult(temp_context, false);
   } else {
     if (content::BrowserThread::PostTaskAndReplyWithResult(
             content::BrowserThread::IO,
             FROM_HERE,
@@ skipping 34 matching lines @@
                            scoped_ptr<ClientIncidentResponse>());
     }
   } else {
     OnReportUploadResult(context,
                          IncidentReportUploader::UPLOAD_SUPPRESSED,
                          scoped_ptr<ClientIncidentResponse>());
   }
 }
 
 void IncidentReportingService::HandleResponse(const UploadContext& context) {
-  for (size_t i = 0; i < context.profiles.size(); ++i) {
-    context.profiles[i]->GetPrefs()->SetBoolean(
-        prefs::kSafeBrowsingIncidentReportSent, true);
+  for (UploadContext::PersistentIncidentStateCollection::const_iterator scan =
+           context.profiles_to_state.begin();
+       scan != context.profiles_to_state.end();
+       ++scan) {
+    DictionaryPrefUpdate pref_update(scan->first->GetPrefs(),
+                                     prefs::kSafeBrowsingIncidentsSent);
+    MarkIncidentsAsReported(scan->second, pref_update.Get());
   }
 }
 
 void IncidentReportingService::OnReportUploadResult(
     UploadContext* context,
     IncidentReportUploader::Result result,
     scoped_ptr<ClientIncidentResponse> response) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   UMA_HISTOGRAM_ENUMERATION(
@@ skipping 29 matching lines @@
       if (!profile->IsOffTheRecord())
         OnProfileDestroyed(profile);
       break;
     }
     default:
       break;
   }
 }
 
 }  // namespace safe_browsing