Prune all safe browsing incidents when already reported.

chrome/browser/safe_browsing/incident_reporting_service_unittest.cc

Index: chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
diff --git a/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc b/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
index 40dd948937885663ff6a2bc2cbb7ecc27fd57340..8693b0e853ac9534959b40782d15ca47b947328d 100644
--- a/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
@@ -4,11 +4,13 @@
 
 #include "chrome/browser/safe_browsing/incident_reporting_service.h"
 
+#include <map>
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/lazy_instance.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_local.h"
@@ -16,10 +18,10 @@
 #include "chrome/browser/safe_browsing/incident_report_uploader.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
-#include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_pref_service_syncable.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/testing_profile_manager.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -90,13 +92,19 @@ class IncidentReportingServiceTest : public testing::Test {
     StartUploadCallback start_upload_callback_;
   };
 
+  // Handy constants to make calls to CreateProfile easy to understand.

[mattm, 2014/06/20 05:44:34]

If you use enums it'll also be type-safe

[grt (UTC plus 2), 2014/06/20 14:52:53]

Indeed. I chose not to since it would be more text for not much more gain. That
said, consider it done.

+  static const bool kSafeBrowsingOptIn;
+  static const bool kSafeBrowsingOptOut;
+  static const bool kOnCreationAddIncident;
+  static const bool kOnCreationDoNotAddIncident;
+
   static const int64 kIncidentTimeMsec;
   static const char kFakeOsName[];
 
   IncidentReportingServiceTest()
       : task_runner_(new base::TestSimpleTaskRunner),
         thread_task_runner_handle_(task_runner_),
-        local_state_(TestingBrowserProcess::GetGlobal()),
+        profile_manager_(TestingBrowserProcess::GetGlobal()),
         instance_(new TestIncidentReportingService(
             task_runner_,
             base::Bind(&IncidentReportingServiceTest::PreProfileCreate,
@@ -109,23 +117,48 @@ class IncidentReportingServiceTest : public testing::Test {
         environment_collected_(),
         uploader_destroyed_() {}
 
-  // Begins the test by creating a profile. An incident will be created within
-  // PreProfileCreate. Tasks are run to allow the service to operate to
-  // completion.
-  void CreateProfileAndRunTest(bool safe_browsing_enabled) {
+  virtual void SetUp() OVERRIDE {
+    testing::Test::SetUp();
+    ASSERT_TRUE(profile_manager_.SetUp());
+  }
+
+  // Creates and returns a profile (owned by the profile manager) with or
+  // without safe browsing enabled. An incident will be created within
+  // PreProfileCreate if requested. See the handy constants above for making
+  // consumption of this easy to understand. Take care, though, they aren't
+  // typesafe.
+  TestingProfile* CreateProfile(const std::string& profile_name,
+                                bool safe_browsing_opt_in,
+                                bool on_creation) {
     // Create prefs for the profile with safe browsing enabled or not.
     scoped_ptr<TestingPrefServiceSyncable> prefs(
         new TestingPrefServiceSyncable);
     chrome::RegisterUserProfilePrefs(prefs->registry());
-    prefs->SetBoolean(prefs::kSafeBrowsingEnabled, safe_browsing_enabled);
-
-    // Build the test profile (PreProfileCreate will be called).
-    TestingProfile::Builder builder;
-    builder.SetPrefService(prefs.PassAs<PrefServiceSyncable>());
-    testing_profile_ = builder.Build().Pass();
+    prefs->SetBoolean(prefs::kSafeBrowsingEnabled,
+                      safe_browsing_opt_in == kSafeBrowsingOptIn);
+
+    // Remember whether or not to create an incident.
+    profile_properties_[profile_name].on_creation_add_incident =
+        (on_creation == kOnCreationAddIncident);
+
+    // Boom (or fizzle).
+    return profile_manager_.CreateTestingProfile(
+        profile_name,
+        prefs.PassAs<PrefServiceSyncable>(),
+        base::ASCIIToUTF16(profile_name),
+        0,
+        std::string(),

[mattm, 2014/06/20 05:44:34]

Add comments with the parameter names.

[grt (UTC plus 2), 2014/06/20 14:52:53]

Done.

+        TestingProfile::TestingFactories());
+  }
 
-    // Let all tasks run.
-    task_runner_->RunUntilIdle();
+  // Configures a callback to run when the next upload is started that will post
+  // a task to delete the profile. This task will run before the upload
+  // finishes.
+  void DeleteProfileOnUpload(Profile* profile) {

[mattm, 2014/06/20 05:44:34]

add an EXPECT/ASSERT_TRUE(on_start_upload_callback_.is_null())

[grt (UTC plus 2), 2014/06/20 14:52:53]

Done.

+    on_start_upload_callback_ =
+        base::Bind(&IncidentReportingServiceTest::DelayedDeleteProfile,
+                   base::Unretained(this),
+                   profile);
   }
 
   // Returns an incident suitable for testing.
@@ -138,7 +171,13 @@ class IncidentReportingServiceTest : public testing::Test {
     return incident.Pass();
   }
 
-  // Confirms that the test incident was uploaded by the service.
+  // Adds a test incident to the service.
+  void AddTestIncident(Profile* profile) {
+    instance_->GetAddIncidentCallback(profile).Run(MakeTestIncident().Pass());
+  }
+
+  // Confirms that the test incident was uploaded by the service, then clears
+  // the instance for subsequent incidents.
   void ExpectTestIncidentUploaded() {
     ASSERT_TRUE(uploaded_report_);
     ASSERT_EQ(1, uploaded_report_->incident_size());
@@ -150,6 +189,8 @@ class IncidentReportingServiceTest : public testing::Test {
     ASSERT_TRUE(uploaded_report_->environment().os().has_os_name());
     ASSERT_EQ(std::string(kFakeOsName),
               uploaded_report_->environment().os().os_name());
+
+    uploaded_report_.reset();
   }
 
   void ExpectNoUpload() { ASSERT_FALSE(uploaded_report_); }
@@ -159,13 +200,13 @@ class IncidentReportingServiceTest : public testing::Test {
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle thread_task_runner_handle_;
-  ScopedTestingLocalState local_state_;
+  TestingProfileManager profile_manager_;
   scoped_ptr<safe_browsing::IncidentReportingService> instance_;
+  base::Closure on_start_upload_callback_;
   safe_browsing::IncidentReportUploader::Result upload_result_;
   bool environment_collected_;
   scoped_ptr<safe_browsing::ClientIncidentReport> uploaded_report_;
   bool uploader_destroyed_;
-  scoped_ptr<TestingProfile> testing_profile_;
 
  private:
   // A fake IncidentReportUploader that posts a task to provide a given response
@@ -201,13 +242,41 @@ class IncidentReportingServiceTest : public testing::Test {
     DISALLOW_COPY_AND_ASSIGN(FakeUploader);
   };
 
+  // Properties for a profile that impact the behavior of the test.
+  struct ProfileProperties {
+    ProfileProperties() : on_creation_add_incident() {}
+
+    // If true (kOnCreationAddIncident), the test fixture will add a test
+    // incident to the service during profile initialization (before
+    // NOTIFICATION_PROFILE_CREATED is sent).
+    bool on_creation_add_incident;
+  };
+
+  // Returns the name of a profile as provided to CreateProfile.
+  static std::string GetProfileName(Profile* profile) {
+    // Cannot reliably use profile->GetProfileName() since the test needs the
+    // name before the profile manager sets it (which happens after profile
+    // creation).
+    return profile->GetPath().BaseName().AsUTF8Unsafe();
+  }
+
+  // Posts a task to delete the profile.
+  void DelayedDeleteProfile(Profile* profile) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&TestingProfileManager::DeleteTestingProfile,
+                   base::Unretained(&profile_manager_),
+                   GetProfileName(profile)));
+  }
+
   // A callback run by the test fixture when a profile is created. An incident
   // is added.
   void PreProfileCreate(Profile* profile) {
     // The instance must have already been created.
     ASSERT_TRUE(instance_);
-    // Add a test incident to the service.
-    instance_->GetAddIncidentCallback(profile).Run(MakeTestIncident().Pass());
+    // Add a test incident to the service if requested.
+    if (profile_properties_[GetProfileName(profile)].on_creation_add_incident)
+      AddTestIncident(profile);
   }
 
   // A fake CollectEnvironmentData implementation invoked by the service during
@@ -227,6 +296,11 @@ class IncidentReportingServiceTest : public testing::Test {
       const safe_browsing::ClientIncidentReport& report) {
     // Remember the report that is being uploaded.
     uploaded_report_.reset(new safe_browsing::ClientIncidentReport(report));
+    // Run and clear the OnStartUpload callback, if provided.
+    if (!on_start_upload_callback_.is_null()) {
+      on_start_upload_callback_.Run();
+      on_start_upload_callback_ = base::Closure();
+    }
     return scoped_ptr<safe_browsing::IncidentReportUploader>(new FakeUploader(
         base::Bind(&IncidentReportingServiceTest::OnUploaderDestroyed,
                    base::Unretained(this)),
@@ -235,6 +309,9 @@ class IncidentReportingServiceTest : public testing::Test {
   }
 
   void OnUploaderDestroyed() { uploader_destroyed_ = true; }
+
+  // A mapping of profile name to its corresponding properties.
+  std::map<std::string, ProfileProperties> profile_properties_;
 };
 
 // static
@@ -244,14 +321,21 @@ base::LazyInstance<base::ThreadLocalPointer<
         LAZY_INSTANCE_INITIALIZER;
 
 // static
+const bool IncidentReportingServiceTest::kSafeBrowsingOptIn = true;
+const bool IncidentReportingServiceTest::kSafeBrowsingOptOut = false;
+const bool IncidentReportingServiceTest::kOnCreationAddIncident = true;
+const bool IncidentReportingServiceTest::kOnCreationDoNotAddIncident = false;
+
 const int64 IncidentReportingServiceTest::kIncidentTimeMsec = 47LL;
 const char IncidentReportingServiceTest::kFakeOsName[] = "fakedows";
 
 // Tests that an incident added during profile initialization when safe browsing
 // is on is uploaded.
 TEST_F(IncidentReportingServiceTest, AddIncident) {
-  // Create the profile, thereby causing the test to begin.
-  CreateProfileAndRunTest(true);
+  CreateProfile("profile1", kSafeBrowsingOptIn, kOnCreationAddIncident);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
 
   // Verify that environment collection took place.
   EXPECT_TRUE(HasCollectedEnvironmentData());
@@ -268,12 +352,83 @@ TEST_F(IncidentReportingServiceTest, AddIncident) {
 // is off is not uploaded.
 TEST_F(IncidentReportingServiceTest, NoSafeBrowsing) {
   // Create the profile, thereby causing the test to begin.
-  CreateProfileAndRunTest(false);
+  CreateProfile("profile1", kSafeBrowsingOptOut, kOnCreationAddIncident);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
 
   // Verify that no report upload took place.
   ExpectNoUpload();
 }
 
+// Tests that an incident added after upload is not uploaded again.
+TEST_F(IncidentReportingServiceTest, OnlyOneUpload) {
+  // Create the profile, thereby causing the test to begin.
+  Profile* profile =
+      CreateProfile("profile1", kSafeBrowsingOptIn, kOnCreationAddIncident);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that report upload took place and contained the incident and
+  // environment data.
+  ExpectTestIncidentUploaded();
+
+  // Add the incident to the service again.
+  AddTestIncident(profile);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that no additional report upload took place.
+  ExpectNoUpload();
+}
+
+// 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", kSafeBrowsingOptIn, kOnCreationAddIncident);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that report upload took place and contained the incident and
+  // environment data.
+  ExpectTestIncidentUploaded();
+
+  // Create a second profile with its own incident on creation.
+  CreateProfile("profile2", kSafeBrowsingOptIn, kOnCreationAddIncident);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that a second report upload took place.
+  ExpectTestIncidentUploaded();
+}
+
+// Tests that an upload succeeds if the profile is destroyed while it is
+// pending.
+TEST_F(IncidentReportingServiceTest, ProfileDestroyedDuringUpload) {
+  // Create a profile for which an incident will be added.
+  Profile* profile =
+      CreateProfile("profile1", kSafeBrowsingOptIn, kOnCreationAddIncident);
+
+  // Hook up a callback to run when the upload is started that will post a task
+  // to delete the profile. This task will run before the upload finishes.
+  DeleteProfileOnUpload(profile);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that report upload took place and contained the incident and
+  // environment data.
+  ExpectTestIncidentUploaded();
+
+  // The lack of a crash indicates that the deleted profile was not accessed by
+  // the service while handling the upload response.
+}
+
 // Parallel uploads
 // Shutdown during processing
 // environment colection taking longer than incident delay timer