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

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 d3dcc190d1fa72737cd0d23ae684293c0e715d50..ea0597faa21cc696ec34153b7acefb6bd822fc5a 100644
--- a/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
@@ -56,7 +56,10 @@ class IncidentReportingServiceTest : public testing::Test {
         const CollectEnvironmentCallback& collect_environment_callback,
         const CreateDownloadFinderCallback& create_download_finder_callback,
         const StartUploadCallback& start_upload_callback)
-        : IncidentReportingService(NULL, NULL),
+        : IncidentReportingService(NULL,
+                                   NULL,
+                                   base::TimeDelta::FromMilliseconds(5),
+                                   task_runner),
           pre_profile_add_callback_(pre_profile_add_callback),
           collect_environment_callback_(collect_environment_callback),
           create_download_finder_callback_(create_download_finder_callback),
@@ -132,6 +135,13 @@ class IncidentReportingServiceTest : public testing::Test {
     ON_CREATE_DOWNLOAD_FINDER_NO_PROFILES,
   };
 
+  // A type for specifying the action to be taken by the test fixture when its
+  // delayed analysis callback is run.
+  enum OnDelayedAnalysisAction {
+    ON_DELAYED_ANALYSIS_NO_ACTION,
+    ON_DELAYED_ANALYSIS_ADD_INCIDENT,  // Add an incident to the service.
+  };
+
   static const int64 kIncidentTimeMsec;
   static const char kFakeOsName[];
   static const char kFakeDownloadToken[];
@@ -153,11 +163,13 @@ class IncidentReportingServiceTest : public testing::Test {
                        base::Unretained(this)))),
         on_create_download_finder_action_(
             ON_CREATE_DOWNLOAD_FINDER_DOWNLOAD_FOUND),
+        on_delayed_analysis_action_(ON_DELAYED_ANALYSIS_NO_ACTION),
         upload_result_(safe_browsing::IncidentReportUploader::UPLOAD_SUCCESS),
         environment_collected_(),
         download_finder_created_(),
         download_finder_destroyed_(),
-        uploader_destroyed_() {}
+        uploader_destroyed_(),
+        delayed_analysis_ran_() {}
 
   virtual void SetUp() OVERRIDE {
     testing::Test::SetUp();
@@ -224,6 +236,14 @@ class IncidentReportingServiceTest : public testing::Test {
     instance_->GetAddIncidentCallback(profile).Run(MakeTestIncident().Pass());
   }
 
+  // Registers the callback to be run for delayed analysis.
+  void RegisterAnalysis(OnDelayedAnalysisAction on_delayed_analysis_action) {
+    on_delayed_analysis_action_ = on_delayed_analysis_action;
+    instance_->RegisterDelayedAnalysisCallback(
+        base::Bind(&IncidentReportingServiceTest::OnDelayedAnalysis,
+                   base::Unretained(this)));
+  }
+
   // Confirms that the test incident(s) was/were uploaded by the service, then
   // clears the instance for subsequent incidents.
   void ExpectTestIncidentUploaded(int incident_count) {
@@ -256,6 +276,7 @@ class IncidentReportingServiceTest : public testing::Test {
   bool HasCreatedDownloadFinder() const { return download_finder_created_; }
   bool DownloadFinderDestroyed() const { return download_finder_destroyed_; }
   bool UploaderDestroyed() const { return uploader_destroyed_; }
+  bool DelayedAnalysisRan() const { return delayed_analysis_ran_; }
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle thread_task_runner_handle_;
@@ -263,12 +284,14 @@ class IncidentReportingServiceTest : public testing::Test {
   scoped_ptr<safe_browsing::IncidentReportingService> instance_;
   base::Closure on_start_upload_callback_;
   OnCreateDownloadFinderAction on_create_download_finder_action_;
+  OnDelayedAnalysisAction on_delayed_analysis_action_;
   safe_browsing::IncidentReportUploader::Result upload_result_;
   bool environment_collected_;
   bool download_finder_created_;
   scoped_ptr<safe_browsing::ClientIncidentReport> uploaded_report_;
   bool download_finder_destroyed_;
   bool uploader_destroyed_;
+  bool delayed_analysis_ran_;
 
  private:
   // A fake IncidentReportUploader that posts a task to provide a given response
@@ -435,6 +458,12 @@ class IncidentReportingServiceTest : public testing::Test {
   void OnDownloadFinderDestroyed() { download_finder_destroyed_ = true; }
   void OnUploaderDestroyed() { uploader_destroyed_ = true; }
 
+  void OnDelayedAnalysis(const safe_browsing::AddIncidentCallback& callback) {
+    delayed_analysis_ran_ = true;
+    if (on_delayed_analysis_action_ == ON_DELAYED_ANALYSIS_ADD_INCIDENT)
+      callback.Run(MakeTestIncident().Pass());
+  }
+
   // A mapping of profile name to its corresponding properties.
   std::map<std::string, ProfileProperties> profile_properties_;
 };
@@ -680,6 +709,157 @@ TEST_F(IncidentReportingServiceTest, MigrateOldPref) {
   AssertNoUpload();
 }
 
+// Tests that no upload results from adding an incident that is not affiliated
+// with a profile.
+TEST_F(IncidentReportingServiceTest, ProcessWideNoProfileNoUpload) {
+  // Add the test incident.
+  AddTestIncident(NULL);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // No upload should have taken place.
+  AssertNoUpload();
+}
+
+// Tests that there is an upload when a profile is present for a proc-wide
+// incident and that pruning works.
+TEST_F(IncidentReportingServiceTest, ProcessWideOneUpload) {
+  // Add a profile that participates in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Add the test incident.
+  AddTestIncident(NULL);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // An upload should have taken place.
+  ExpectTestIncidentUploaded(1);
+
+  // Add the incident to the service again.
+  AddTestIncident(NULL);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that no additional report upload took place.
+  AssertNoUpload();
+}
+
+// Tests that there is an upload when a profile appears after a proc-wide
+// incident.
+TEST_F(IncidentReportingServiceTest, ProcessWideOneUploadAfterProfile) {
+  // Add the test incident.
+  AddTestIncident(NULL);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that no report upload took place.
+  AssertNoUpload();
+
+  // Add a profile that participates in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // An upload should have taken place.
+  ExpectTestIncidentUploaded(1);
+}
+
+// Tests that delayed analysis callbacks are called after a profile
+// participating in safe browsing is added.

[robertshield, 2014/08/06 18:27:07]

perhaps add "but not before" to this comment to differentiate between the
comment on line 797

[grt (UTC plus 2), 2014/08/07 01:46:14]

Done.

+TEST_F(IncidentReportingServiceTest, AnalysisAfterProfile) {
+  // Register a callback.
+  RegisterAnalysis(ON_DELAYED_ANALYSIS_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Not run yet.
+  ASSERT_FALSE(DelayedAnalysisRan());
+
+  // Add a profile that participates in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // And now they have.
+  ASSERT_TRUE(DelayedAnalysisRan());
+}
+
+// Tests that delayed analysis callbacks are called after a profile
+// participating in safe browsing is added.
+TEST_F(IncidentReportingServiceTest, AnalysisWhenReigsteredWithProfile) {

[mattm, 2014/08/06 22:36:12]

Reigstered

[grt (UTC plus 2), 2014/08/07 01:46:14]

Done.

+  // Add a profile that participates in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Register a callback.
+  RegisterAnalysis(ON_DELAYED_ANALYSIS_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Confirm that the callbacks were run.
+  ASSERT_TRUE(DelayedAnalysisRan());
+}
+
+// Tests that no upload results from a delayed analysis incident when no
+// safe browsing profile is present.
+TEST_F(IncidentReportingServiceTest, DelayedAnalysisNoProfileNoUpload) {
+  // Register a callback that will add an incident.
+  RegisterAnalysis(ON_DELAYED_ANALYSIS_ADD_INCIDENT);
+
+  // Add a profile that does not participate in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_OUT, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // The callback should not have been run.
+  ASSERT_FALSE(DelayedAnalysisRan());
+
+  // No upload should have taken place.
+  AssertNoUpload();
+}
+
+// Tests that there is an upload when a profile is present for a delayed
+// analysis incident and that pruning works.
+TEST_F(IncidentReportingServiceTest, DelayedAnalysisOneUpload) {
+  // Register a callback that will add an incident.
+  RegisterAnalysis(ON_DELAYED_ANALYSIS_ADD_INCIDENT);
+
+  // Add a profile that participates in safe browsing.
+  CreateProfile(
+      "profile1", SAFE_BROWSING_OPT_IN, ON_PROFILE_ADDITION_NO_ACTION);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // The callback should have been run.
+  ASSERT_TRUE(DelayedAnalysisRan());
+
+  // An upload should have taken place.
+  ExpectTestIncidentUploaded(1);
+
+  // Add the incident to the service again.
+  AddTestIncident(NULL);
+
+  // Let all tasks run.
+  task_runner_->RunUntilIdle();
+
+  // Verify that no additional report upload took place.
+  AssertNoUpload();
+}
+

[mattm, 2014/08/06 22:36:12]

The tests which add two incidents only test that the 2nd same incident gets
pruned, how bout a test in which the 2nd incident is different and that it gets
uploaded too? (eg, a test for the bug I pointed out earlier.)

[grt (UTC plus 2), 2014/08/07 01:46:14]

Sure. I'd thought that the prune test would cover the case you found since the
second call to AddIncident with a NULL profile would have hit the
DCHECK(context) in that function. As it happens, the same callback needs to be
used for both incidents to hit that exact path. I've added ProcessWideTwoUploads
to cover the case you mention.

 // Parallel uploads
 // Shutdown during processing
 // environment colection taking longer than incident delay timer