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

chrome/browser/safe_browsing/incident_reporting_service_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/incident_reporting_service.h"
 
 #include <map>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 117 matching lines @@
     ON_CREATE_DOWNLOAD_FINDER_DOWNLOAD_FOUND,
     // Post a task that reports no downloads found.
     ON_CREATE_DOWNLOAD_FINDER_NO_DOWNLOADS,
     // Immediately return due to a lack of eligible profiles.
     ON_CREATE_DOWNLOAD_FINDER_NO_PROFILES,
   };
 
   static const int64 kIncidentTimeMsec;
   static const char kFakeOsName[];
   static const char kFakeDownloadToken[];
+  static const char kTestTrackedPrefPath[];
 
   IncidentReportingServiceTest()
       : task_runner_(new base::TestSimpleTaskRunner),
         thread_task_runner_handle_(task_runner_),
         profile_manager_(TestingBrowserProcess::GetGlobal()),
         instance_(new TestIncidentReportingService(
             task_runner_,
             base::Bind(&IncidentReportingServiceTest::PreProfileAdd,
                        base::Unretained(this)),
             base::Bind(&IncidentReportingServiceTest::CollectEnvironmentData,
@@ skipping 57 matching lines @@
                    base::Unretained(this),
                    profile);
   }
 
   // Returns an incident suitable for testing.
   scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData>
   MakeTestIncident() {
     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();
   }
 
   // Adds a test incident to the service.
   void AddTestIncident(Profile* profile) {
     instance_->GetAddIncidentCallback(profile).Run(MakeTestIncident().Pass());
   }
 
   // Confirms that the test incident(s) was/were uploaded by the service, then
   // clears the instance for subsequent incidents.
   void ExpectTestIncidentUploaded(int incident_count) {
     ASSERT_TRUE(uploaded_report_);
     ASSERT_EQ(incident_count, uploaded_report_->incident_size());
     for (int i = 0; i < incident_count; ++i) {
       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());
     ASSERT_TRUE(uploaded_report_->environment().os().has_os_name());
     ASSERT_EQ(std::string(kFakeOsName),
               uploaded_report_->environment().os().os_name());
     ASSERT_EQ(std::string(kFakeDownloadToken),
               uploaded_report_->download().token());
 
     uploaded_report_.reset();
@@ skipping 190 matching lines @@
 
 // static
 base::LazyInstance<base::ThreadLocalPointer<
     IncidentReportingServiceTest::TestIncidentReportingService> >::Leaky
     IncidentReportingServiceTest::TestIncidentReportingService::test_instance_ =
         LAZY_INSTANCE_INITIALIZER;
 
 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.
 TEST_F(IncidentReportingServiceTest, AddIncident) {
   CreateProfile(
       "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_ADD_INCIDENT);
 
   // Let all tasks run.
   task_runner_->RunUntilIdle();
 
@@ skipping 83 matching lines @@
 
   // Verify that the download finder was run but that no report upload took
   // place.
   EXPECT_TRUE(HasCreatedDownloadFinder());
   AssertNoUpload();
   // Although CreateDownloadFinder was called, no instance was returned so there
   // is nothing to have been destroyed.
   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);
 
   // Let all tasks run.
   task_runner_->RunUntilIdle();
 
   // Verify that report upload took place and contained the incident and
   // environment data.
   ExpectTestIncidentUploaded(1);
 
   // Add the incident to the service again.
   AddTestIncident(profile);
 
   // Let all tasks run.
   task_runner_->RunUntilIdle();
 
   // Verify that no additional report upload took place.
   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) {
   // Create the profile, thereby causing the test to begin.
   CreateProfile(
       "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_ADD_INCIDENT);
 
   // Let all tasks run.
   task_runner_->RunUntilIdle();
 
@@ skipping 27 matching lines @@
   task_runner_->RunUntilIdle();
 
   // Verify that report upload took place and contained the incident and
   // environment data.
   ExpectTestIncidentUploaded(1);
 
   // The lack of a crash indicates that the deleted profile was not accessed by
   // 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.

[gab, 2014/07/23 13:58:11]

But adding another one should result in an upload, I think you should test that
as well.

[grt (UTC plus 2), 2014/07/23 14:45:18]

i don't think that's needed. since this test validates that the legacy pref is
cleared, adding another different incident would be testing the same thing that
TwoIncidentsTwoUploads tests.

[gab, 2014/07/23 15:00:23]

Good point, not worth it indeed, given you verify the pref is cleared.

+  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
 // environment colection taking longer than incident delay timer, and then
 // another incident arriving