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

BUG=396398

Review URL: https://codereview.chromium.org/411793004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285491 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/prefs/chrome_pref_service_factory.cc

@@ -195,10 +195,15 @@ const PrefHashFilter::TrackedPreferenceMetadata kTrackedPrefs[] = {
     PrefHashFilter::ENFORCE_ON_LOAD,
     PrefHashFilter::TRACKING_STRATEGY_ATOMIC
   },
+  {
+    18, prefs::kSafeBrowsingIncidentsSent,
+    PrefHashFilter::ENFORCE_ON_LOAD,
+    PrefHashFilter::TRACKING_STRATEGY_ATOMIC
+  },
 };
 
 // The count of tracked preferences IDs across all platforms.
-const size_t kTrackedPrefsReportingIDsCount = 18;
+const size_t kTrackedPrefsReportingIDsCount = 19;
 COMPILE_ASSERT(kTrackedPrefsReportingIDsCount >= arraysize(kTrackedPrefs),
                need_to_increment_ids_count);
 

chrome/browser/profiles/profile.cc

@@ -118,6 +118,9 @@ void Profile::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
       prefs::kSafeBrowsingIncidentReportSent,
       false,
       user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterDictionaryPref(
+      prefs::kSafeBrowsingIncidentsSent,
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 #if defined(ENABLE_GOOGLE_NOW)
   registry->RegisterBooleanPref(
       prefs::kGoogleGeolocationAccessEnabled,

chrome/browser/safe_browsing/incident_handler_util.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_handler_util.h"
+
+#include <string>
+
+#include "base/hash.h"
+#include "base/logging.h"
+#include "third_party/protobuf/src/google/protobuf/message_lite.h"
+
+namespace safe_browsing {
+
+// Computes a simple hash digest over the serialized form of |message|.
+// |message| must be in a canonical form.
+uint32_t HashMessage(const google::protobuf::MessageLite& message) {
+  std::string message_string;
+  if (!message.SerializeToString(&message_string)) {
+    NOTREACHED();
+    return 0;
+  }
+  return base::Hash(message_string);
+}
+
+}  // namespace safe_browsing

chrome/browser/safe_browsing/incident_handler_util.h

+// 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.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_INCIDENT_HANDLER_UTIL_H_
+#define CHROME_BROWSER_SAFE_BROWSING_INCIDENT_HANDLER_UTIL_H_
+
+#include <stdint.h>
+
+namespace google {
+namespace protobuf {
+
+class MessageLite;
+
+}  // namespace protobuf
+}  // namespace google
+
+namespace safe_browsing {
+
+// Computes a simple hash digest over the serialized form of |message|.
+// |message| must be in a canonical form. For example, fields set to their
+// default values should be cleared.
+uint32_t HashMessage(const google::protobuf::MessageLite& message);
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_INCIDENT_HANDLER_UTIL_H_

chrome/browser/safe_browsing/incident_reporting_service_unittest.cc

@@ -135,6 +135,7 @@ class IncidentReportingServiceTest : public testing::Test {
   static const int64 kIncidentTimeMsec;
   static const char kFakeOsName[];
   static const char kFakeDownloadToken[];
+  static const char kTestTrackedPrefPath[];
 
   IncidentReportingServiceTest()
       : task_runner_(new base::TestSimpleTaskRunner),
@@ -212,7 +213,9 @@ class IncidentReportingServiceTest : public testing::Test {
     scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
         new safe_browsing::ClientIncidentReport_IncidentData());
     incident->set_incident_time_msec(kIncidentTimeMsec);
-    incident->mutable_tracked_preference();
+    safe_browsing::ClientIncidentReport_IncidentData_TrackedPreferenceIncident*
+        tp_incident = incident->mutable_tracked_preference();
+    tp_incident->set_path(kTestTrackedPrefPath);
     return incident.Pass();
   }
 
@@ -230,6 +233,11 @@ class IncidentReportingServiceTest : public testing::Test {
       ASSERT_TRUE(uploaded_report_->incident(i).has_incident_time_msec());
       ASSERT_EQ(kIncidentTimeMsec,
                 uploaded_report_->incident(i).incident_time_msec());
+      ASSERT_TRUE(uploaded_report_->incident(i).has_tracked_preference());
+      ASSERT_TRUE(
+          uploaded_report_->incident(i).tracked_preference().has_path());
+      ASSERT_EQ(std::string(kTestTrackedPrefPath),
+                uploaded_report_->incident(i).tracked_preference().path());
     }
     ASSERT_TRUE(uploaded_report_->has_environment());
     ASSERT_TRUE(uploaded_report_->environment().has_os());
@@ -440,6 +448,7 @@ base::LazyInstance<base::ThreadLocalPointer<
 const int64 IncidentReportingServiceTest::kIncidentTimeMsec = 47LL;
 const char IncidentReportingServiceTest::kFakeOsName[] = "fakedows";
 const char IncidentReportingServiceTest::kFakeDownloadToken[] = "fakedlt";
+const char IncidentReportingServiceTest::kTestTrackedPrefPath[] = "some_pref";
 
 // Tests that an incident added during profile initialization when safe browsing
 // is on is uploaded.
@@ -543,8 +552,8 @@ TEST_F(IncidentReportingServiceTest, NoProfilesNoUpload) {
   EXPECT_FALSE(DownloadFinderDestroyed());
 }
 
-// Tests that an incident added after upload is not uploaded again.
-TEST_F(IncidentReportingServiceTest, OnlyOneUpload) {
+// Tests that an identical incident added after upload is not uploaded again.
+TEST_F(IncidentReportingServiceTest, OneIncidentOneUpload) {
   // Create the profile, thereby causing the test to begin.
   Profile* profile = CreateProfile(
       "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_ADD_INCIDENT);
@@ -566,6 +575,33 @@ TEST_F(IncidentReportingServiceTest, OnlyOneUpload) {
   AssertNoUpload();
 }
 
+// Tests that two incidents of the same type with different payloads lead to two
+// uploads.
+TEST_F(IncidentReportingServiceTest, TwoIncidentsTwoUploads) {
+  // Create the profile, thereby causing the test to begin.
+  Profile* profile = CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_ADD_INCIDENT);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that report upload took place and contained the incident and
+  // environment data.
+  ExpectTestIncidentUploaded(1);
+
+  // Add a variation on the incident to the service.
+  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
+      MakeTestIncident());
+  incident->mutable_tracked_preference()->set_atomic_value("leeches");
+  instance_->GetAddIncidentCallback(profile).Run(incident.Pass());
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that an additional report upload took place.
+  ExpectTestIncidentUploaded(1);
+}
+
 // Tests that the same incident added for two different profiles in sequence
 // results in two uploads.
 TEST_F(IncidentReportingServiceTest, TwoProfilesTwoUploads) {
@@ -613,6 +649,37 @@ TEST_F(IncidentReportingServiceTest, ProfileDestroyedDuringUpload) {
   // the service while handling the upload response.
 }
 
+// Tests that no upload takes place if the old pref was present.
+TEST_F(IncidentReportingServiceTest, MigrateOldPref) {
+  Profile* profile = CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // This is a legacy profile.
+  profile->GetPrefs()->SetBoolean(prefs::kSafeBrowsingIncidentReportSent, true);
+
+  // Add the test incident.
+  AddTestIncident(profile);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // No upload should have taken place.
+  AssertNoUpload();
+
+  // The legacy pref should have been cleared.
+  ASSERT_FALSE(
+      profile->GetPrefs()->GetBoolean(prefs::kSafeBrowsingIncidentReportSent));
+
+  // Adding the same incident again should still result in no upload.
+  AddTestIncident(profile);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // No upload should have taken place.
+  AssertNoUpload();
+}
+
 // Parallel uploads
 // Shutdown during processing
 // environment colection taking longer than incident delay timer

chrome/browser/safe_browsing/tracked_preference_incident_handlers.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/tracked_preference_incident_handlers.h"
+
+#include "base/logging.h"
+#include "chrome/browser/safe_browsing/incident_handler_util.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+
+namespace safe_browsing {
+
+std::string GetTrackedPreferenceIncidentKey(
+    const ClientIncidentReport_IncidentData& incident_data) {
+  DCHECK(incident_data.has_tracked_preference());
+  DCHECK(incident_data.tracked_preference().has_path());
+  return incident_data.tracked_preference().path();
+}
+
+uint32_t GetTrackedPreferenceIncidentDigest(
+    const ClientIncidentReport_IncidentData& incident_data) {
+  DCHECK(incident_data.has_tracked_preference());
+
+  // Tracked preference incidents are sufficiently canonical (and have no
+  // default values), so it's safe to serialize the incident as given.
+  return HashMessage(incident_data.tracked_preference());
+}
+
+}  // namespace safe_browsing

chrome/browser/safe_browsing/tracked_preference_incident_handlers.h

+// 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.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_TRACKED_PREFERENCE_INCIDENT_HANDLERS_H_
+#define CHROME_BROWSER_SAFE_BROWSING_TRACKED_PREFERENCE_INCIDENT_HANDLERS_H_
+
+#include <stdint.h>
+
+#include <string>
+
+namespace safe_browsing {
+
+class ClientIncidentReport_IncidentData;
+
+// Returns the path of the tracked preference.
+std::string GetTrackedPreferenceIncidentKey(
+    const ClientIncidentReport_IncidentData& incident_data);
+
+// Returns a digest computed over the tracked preference incident data.
+uint32_t GetTrackedPreferenceIncidentDigest(
+    const ClientIncidentReport_IncidentData& incident_data);
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_TRACKED_PREFERENCE_INCIDENT_HANDLERS_H_

chrome/browser/safe_browsing/tracked_preference_incident_handlers_unittest.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/tracked_preference_incident_handlers.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "chrome/common/safe_browsing/csd.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> MakeIncident() {
+  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
+      new safe_browsing::ClientIncidentReport_IncidentData);
+
+  incident->mutable_tracked_preference()->set_path("foo");
+  incident->mutable_tracked_preference()->set_atomic_value("bar");
+  incident->mutable_tracked_preference()->set_value_state(
+      safe_browsing::
+          ClientIncidentReport_IncidentData_TrackedPreferenceIncident_ValueState_CLEARED);
+  return incident.Pass();
+}
+
+}  // namespace
+
+// Tests that GetKey returns the preference path.
+TEST(GetTrackedPreferenceIncidentKey, KeyIsPath) {
+  safe_browsing::ClientIncidentReport_IncidentData incident;
+
+  incident.mutable_tracked_preference()->set_path("foo");
+  ASSERT_EQ(std::string("foo"),
+            safe_browsing::GetTrackedPreferenceIncidentKey(incident));
+}
+
+// Tests that GetDigest returns the same value for the same incident.
+TEST(GetTrackedPreferenceIncidentDigest, SameIncidentSameDigest) {
+  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
+      MakeIncident());
+
+  uint32_t digest =
+      safe_browsing::GetTrackedPreferenceIncidentDigest(*incident);
+  ASSERT_EQ(digest,
+            safe_browsing::GetTrackedPreferenceIncidentDigest(*MakeIncident()));
+}
+
+// Tests that GetDigest returns the same value for the same incident.
+TEST(GetTrackedPreferenceIncidentDigest, DifferentIncidentDifferentDigest) {
+  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
+      MakeIncident());
+
+  uint32_t digest =
+      safe_browsing::GetTrackedPreferenceIncidentDigest(*incident);
+
+  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident2(
+      MakeIncident());
+  incident2->mutable_tracked_preference()->set_value_state(
+      safe_browsing::
+          ClientIncidentReport_IncidentData_TrackedPreferenceIncident_ValueState_WEAK_LEGACY);
+
+  ASSERT_NE(digest,
+            safe_browsing::GetTrackedPreferenceIncidentDigest(*incident2));
+}

chrome/chrome_browser.gypi

@@ -2672,6 +2672,8 @@
       'browser/safe_browsing/environment_data_collection.h',
       'browser/safe_browsing/environment_data_collection_win.cc',
       'browser/safe_browsing/environment_data_collection_win.h',
+      'browser/safe_browsing/incident_handler_util.cc',
+      'browser/safe_browsing/incident_handler_util.h',
       'browser/safe_browsing/incident_reporting_service.cc',
       'browser/safe_browsing/incident_reporting_service.h',
       'browser/safe_browsing/incident_report_uploader.cc',
@@ -2700,6 +2702,8 @@
       'browser/safe_browsing/safe_browsing_store.h',
       'browser/safe_browsing/sandboxed_zip_analyzer.cc',
       'browser/safe_browsing/sandboxed_zip_analyzer.h',
+      'browser/safe_browsing/tracked_preference_incident_handlers.cc',
+      'browser/safe_browsing/tracked_preference_incident_handlers.h',
       'browser/safe_browsing/two_phase_uploader.cc',
       'browser/safe_browsing/two_phase_uploader.h',
     ],

chrome/chrome_tests_unit.gypi

@@ -1266,6 +1266,7 @@
         'browser/safe_browsing/safe_browsing_store_file_unittest.cc',
         'browser/safe_browsing/safe_browsing_store_unittest.cc',
         'browser/safe_browsing/safe_browsing_util_unittest.cc',
+        'browser/safe_browsing/tracked_preference_incident_handlers_unittest.cc',
         'browser/safe_browsing/two_phase_uploader_unittest.cc',
         'browser/search/hotword_service_unittest.cc',
         'browser/search/iframe_source_unittest.cc',

chrome/common/pref_names.cc

@@ -315,6 +315,9 @@ const char kSafeBrowsingProceedAnywayDisabled[] =
 const char kSafeBrowsingIncidentReportSent[] =
     "safebrowsing.incident_report_sent";
 
+// A dictionary mapping incident types to a dict of incident key:digest pairs.
+const char kSafeBrowsingIncidentsSent[] = "safebrowsing.incidents_sent";
+
 // Enum that specifies whether Incognito mode is:
 // 0 - Enabled. Default behaviour. Default mode is available on demand.
 // 1 - Disabled. Used cannot browse pages in Incognito mode.

chrome/common/pref_names.h

@@ -138,6 +138,7 @@ extern const char kSafeBrowsingDownloadFeedbackEnabled[];
 extern const char kSafeBrowsingReportingEnabled[];
 extern const char kSafeBrowsingProceedAnywayDisabled[];
 extern const char kSafeBrowsingIncidentReportSent[];
+extern const char kSafeBrowsingIncidentsSent[];
 extern const char kIncognitoModeAvailability[];
 extern const char kSearchSuggestEnabled[];
 #if defined(OS_ANDROID)