Support for process-wide incidents in the 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 38 matching lines @@
     typedef base::Callback<scoped_ptr<safe_browsing::IncidentReportUploader>(
         const safe_browsing::IncidentReportUploader::OnResultCallback&,
         const safe_browsing::ClientIncidentReport& report)> StartUploadCallback;
 
     TestIncidentReportingService(
         const scoped_refptr<base::TaskRunner>& task_runner,
         const PreProfileAddCallback& pre_profile_add_callback,
         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),
           start_upload_callback_(start_upload_callback) {
       SetCollectEnvironmentHook(&CollectEnvironmentData, task_runner);
       test_instance_.Get().Set(this);
     }
 
     virtual ~TestIncidentReportingService() { test_instance_.Get().Set(NULL); }
 
@@ skipping 55 matching lines @@
   // service creates a LastDownloadFinder.
   enum OnCreateDownloadFinderAction {
     // Post a task that reports a download.
     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,
   };
 
+  // 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[];
   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,
                        base::Unretained(this)),
             base::Bind(&IncidentReportingServiceTest::CreateDownloadFinder,
                        base::Unretained(this)),
             base::Bind(&IncidentReportingServiceTest::StartUpload,
                        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();
     ASSERT_TRUE(profile_manager_.SetUp());
   }
 
   // Sets the action to be taken by the test fixture when the service creates a
   // LastDownloadFinder.
   void SetCreateDownloadFinderAction(OnCreateDownloadFinderAction action) {
     on_create_download_finder_action_ = action;
@@ skipping 46 matching lines @@
         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());
   }
 
+  // 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) {
     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());
@@ skipping 12 matching lines @@
 
     uploaded_report_.reset();
   }
 
   void AssertNoUpload() { ASSERT_FALSE(uploaded_report_); }
 
   bool HasCollectedEnvironmentData() const { return environment_collected_; }
   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_;
   TestingProfileManager profile_manager_;
   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
   // back to the incident reporting service. It also reports back to the test
   // harness via a closure when it is deleted by the incident reporting service.
   class FakeUploader : public safe_browsing::IncidentReportUploader {
    public:
     FakeUploader(
         const base::Closure& on_deleted,
         const safe_browsing::IncidentReportUploader::OnResultCallback& callback,
@@ skipping 146 matching lines @@
                    base::Bind(
                        &IncidentReportingServiceTest::OnUploaderDestroyed,
                        base::Unretained(this)),
                    callback,
                    upload_result_)).Pass();
   }
 
   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_;
 };
 
 // static
 base::LazyInstance<base::ThreadLocalPointer<
     IncidentReportingServiceTest::TestIncidentReportingService> >::Leaky
     IncidentReportingServiceTest::TestIncidentReportingService::test_instance_ =
         LAZY_INSTANCE_INITIALIZER;
 
@@ skipping 225 matching lines @@
   // 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();
 }
 
+// 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
 // environment colection taking longer than incident delay timer, and then
 // another incident arriving