Safe browsing incident reporting service improvements.

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 <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/lazy_instance.h"
 #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"
 
 // A test fixture that sets up a test task runner and makes it the thread's
 // runner. The fixture implements a fake envrionment data collector and a fake
 // report uploader.
 class IncidentReportingServiceTest : public testing::Test {
  protected:
   // An IRS class that allows a test harness to provide a fake environment
   // collector and report uploader via callbacks.
@@ skipping 40 matching lines @@
       current().collect_environment_callback_.Run(data);
     };
 
     static base::LazyInstance<base::ThreadLocalPointer<
         TestIncidentReportingService> >::Leaky test_instance_;
 
     CollectEnvironmentCallback collect_environment_callback_;
     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,
                        base::Unretained(this)),
             base::Bind(&IncidentReportingServiceTest::StartUpload,
                        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
   // 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 15 matching lines @@
       callback_.Run(result_,
                     scoped_ptr<safe_browsing::ClientIncidentResponse>());
     }
 
     base::Closure on_deleted_;
     safe_browsing::IncidentReportUploader::Result result_;
 
     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(
         static_cast<safe_browsing::ClientIncidentReport_EnvironmentData*>(NULL),
         data);
     data->mutable_os()->set_os_name(kFakeOsName);
     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) {
     // Remember the report that is being uploaded.
     uploaded_report_.reset(new safe_browsing::ClientIncidentReport(report));
     return scoped_ptr<safe_browsing::IncidentReportUploader>(new FakeUploader(
         base::Bind(&IncidentReportingServiceTest::OnUploaderDestroyed,
                    base::Unretained(this)),
         callback,
         upload_result_));
   }
 
   void OnUploaderDestroyed() { uploader_destroyed_ = true; }
 };
 
 // static
 base::LazyInstance<base::ThreadLocalPointer<
     IncidentReportingServiceTest::TestIncidentReportingService> >::Leaky
     IncidentReportingServiceTest::TestIncidentReportingService::test_instance_ =
         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
 // environment colection taking longer than incident delay timer
 // environment colection taking longer than incident delay timer, and then
 // another incident arriving