Safe browsing incident reporting service improvements.

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 58bee748f7184ef58f8b34a5581497eb19b3e655..f184a0dc848644107da09a86e0c251e888dca8d9 100644
--- a/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
+++ b/chrome/browser/safe_browsing/incident_reporting_service_unittest.cc
@@ -12,8 +12,18 @@
 #include "base/test/test_simple_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_local.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/prefs/browser_prefs.h"
 #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 "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -74,11 +84,47 @@ class IncidentReportingServiceTest : public testing::Test {
     StartUploadCallback start_upload_callback_;
   };
 
+  // A class that registers for PROFILE_CREATED notifications and invokes a
+  // callback upon receipt. This is used by the test fixture to provide a means
+  // to submit an incident during profile initialization before the reporting
+  // service notices that profile creation has completed.

[mattm, 2014/06/18 21:07:18]

Is it guaranteed that this observer will be called before the one that the
IncidentReportingService registers?

[grt (UTC plus 2), 2014/06/18 22:47:58]

In the current implementation, yes. It's not documented, but ObserverList
appends observers, so they are called in the order they're added. I looked for a
way to hook into profile initialization, but the obvious thing (passing a
Delegate to TestingProfile) has a comment making it look like there's a movement
to do away with it, so I chose this route. Are you okay with leaving this in
place until it becomes an issue?

[mattm, 2014/06/18 23:05:38]

Realistically it's probably fine.. main concern would be if it changes, and the
test continues to pass but just not actually testing what it meant to test
anymore. Is it easy to make the test fail if things happen in the wrong order?

[grt (UTC plus 2), 2014/06/19 02:24:39]

I've removed the notification service magic in favor of making OnProfileCreated
virtual in IncidentReportingService so that the test can override it and inject
an incident before calling the base implementation. This is less fragile at the
expense of coupling the test to the implementation a bit more (but this is
probably a better tradeoff anyway since it was previously coupled to an
implementation detail of ObserverList). What do you think?

+  class PreProfileInitRegistrar : public content::NotificationObserver {
+   public:
+    PreProfileInitRegistrar(
+        const base::Callback<void(Profile*)>& profile_created_closure)
+        : profile_created_closure_(profile_created_closure) {
+      notification_registrar_.Add(this,
+                                  chrome::NOTIFICATION_PROFILE_CREATED,
+                                  content::NotificationService::AllSources());
+    }
+
+   private:
+    virtual void Observe(int type,
+                         const content::NotificationSource& source,
+                         const content::NotificationDetails& details) OVERRIDE {
+      if (type == chrome::NOTIFICATION_PROFILE_CREATED) {
+        Profile* profile = content::Source<Profile>(source).ptr();
+        if (!profile->IsOffTheRecord())
+          profile_created_closure_.Run(profile);
+      }
+    }
+
+    content::NotificationRegistrar notification_registrar_;
+    base::Callback<void(Profile*)> profile_created_closure_;
+
+    DISALLOW_COPY_AND_ASSIGN(PreProfileInitRegistrar);
+  };
+
+  static const int64 kIncidentTimeMsec;
   static const char kFakeOsName[];
 
   IncidentReportingServiceTest()
       : task_runner_(new base::TestSimpleTaskRunner),
         thread_task_runner_handle_(task_runner_),
+        local_state_(TestingBrowserProcess::GetGlobal()),
+        pre_profile_init_registrar_(
+            base::Bind(&IncidentReportingServiceTest::PreProfileInit,
+                       base::Unretained(this))),
         instance_(new TestIncidentReportingService(
             task_runner_,
             base::Bind(&IncidentReportingServiceTest::CollectEnvironmentData,
@@ -87,20 +133,70 @@ class IncidentReportingServiceTest : public testing::Test {
                        base::Unretained(this)))),
         upload_result_(safe_browsing::IncidentReportUploader::UPLOAD_SUCCESS),
         environment_collected_(),
-        uploader_destroyed_() {
-    instance_->SetEnabled(true);
+        uploader_destroyed_() {}
+
+  // Begins the test by creating a profile. An incident will be created within
+  // PreProfileInit. Tasks are run to allow the service to operate to
+  // completion.
+  void CreateProfileAndRunTest(bool safe_browsing_enabled) {
+    // 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 (PreProfileInit will be called).
+    TestingProfile::Builder builder;
+    builder.SetPrefService(prefs.PassAs<PrefServiceSyncable>());
+    testing_profile_ = builder.Build().Pass();
+
+    // Let all tasks run.
+    task_runner_->RunUntilIdle();

[mattm, 2014/06/18 21:07:18]

I didn't notice before, but how does this work? Shouldn't the reporting service
be waiting one minute before it uploads the report?

[grt (UTC plus 2), 2014/06/18 22:47:58]

I had the same "huh?" moment. TestSimpleTaskRunner ignores the delay.

[mattm, 2014/06/18 23:05:38]

Ah ha, thanks.

   }
 
+  // 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();
+    return incident.Pass();
+  }
+
+  // Confirms that the test incident was uploaded by the service.
+  void ExpectTestIncidentUploaded() {
+    ASSERT_TRUE(uploaded_report_);
+    ASSERT_EQ(1, uploaded_report_->incident_size());
+    ASSERT_TRUE(uploaded_report_->incident(0).has_incident_time_msec());
+    ASSERT_EQ(kIncidentTimeMsec,
+              uploaded_report_->incident(0).incident_time_msec());
+    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());
+  }
+
+  void ExpectNoUpload() { ASSERT_FALSE(uploaded_report_); }
+
   bool HasCollectedEnvironmentData() const { return environment_collected_; }
   bool UploaderDestroyed() const { return uploader_destroyed_; }
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle thread_task_runner_handle_;
+  ScopedTestingLocalState local_state_;
+
+  // This registrar must be constructed before the service instance below.
+  PreProfileInitRegistrar pre_profile_init_registrar_;
+
+  // This service instance must be constructed after the registrar above.
   scoped_ptr<safe_browsing::IncidentReportingService> instance_;
   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
@@ -136,6 +232,17 @@ class IncidentReportingServiceTest : public testing::Test {
     DISALLOW_COPY_AND_ASSIGN(FakeUploader);
   };
 
+  // A callback run by the test fixture when a profile is created. An incident
+  // is added.
+  void PreProfileInit(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());
+  }
+
+  // A fake CollectEnvironmentData implementation invoked by the service during
+  // operation.
   void CollectEnvironmentData(
       safe_browsing::ClientIncidentReport_EnvironmentData* data) {
     ASSERT_NE(
@@ -145,6 +252,7 @@ class IncidentReportingServiceTest : public testing::Test {
     environment_collected_ = true;
   }
 
+  // A fake StartUpload implementation invoked by the service during operation.
   scoped_ptr<safe_browsing::IncidentReportUploader> StartUpload(
       const safe_browsing::IncidentReportUploader::OnResultCallback& callback,
       const safe_browsing::ClientIncidentReport& report) {
@@ -167,43 +275,36 @@ base::LazyInstance<base::ThreadLocalPointer<
         LAZY_INSTANCE_INITIALIZER;
 
 // static
+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) {
-  // Make up a dummy incident.
-  const int64 kIncidentTimeMsec = 47LL;
-  scoped_ptr<safe_browsing::ClientIncidentReport_IncidentData> incident(
-      new safe_browsing::ClientIncidentReport_IncidentData());
-  incident->set_incident_time_msec(kIncidentTimeMsec);
-
-  // Add it to the service.
-  instance_->GetAddIncidentCallback().Run(incident.Pass());
-
-  // Let all tasks run.
-  task_runner_->RunUntilIdle();
+  // Create the profile, thereby causing the test to begin.
+  CreateProfileAndRunTest(true);
 
   // Verify that environment collection took place.
   EXPECT_TRUE(HasCollectedEnvironmentData());
 
   // Verify that report upload took place and contained the incident and
   // environment data.
-  ASSERT_TRUE(uploaded_report_);
-  ASSERT_EQ(1, uploaded_report_->incident_size());
-  ASSERT_TRUE(uploaded_report_->incident(0).has_incident_time_msec());
-  ASSERT_EQ(kIncidentTimeMsec,
-            uploaded_report_->incident(0).incident_time_msec());
-  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());
+  ExpectTestIncidentUploaded();
 
   // Verify that the uploader was destroyed.
   ASSERT_TRUE(UploaderDestroyed());
 }
 
-// Nothing happens when disabled
-// Nothing is sent when disabled before/after environment collection
+// Tests that an incident added during profile initialization when safe browsing
+// is off is not uploaded.
+TEST_F(IncidentReportingServiceTest, NoSafeBrowsing) {
+  // Create the profile, thereby causing the test to begin.
+  CreateProfileAndRunTest(false);
+
+  // Verify that no report upload took place.
+  ExpectNoUpload();
+}
+
 // Duplicate incidents are stripped
 // Parallel uploads
 // Shutdown during processing