Wire up CertificateReportingService to handle report uploads

chrome/browser/safe_browsing/certificate_reporting_service_unittest.cc

 // Copyright 2016 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/certificate_reporting_service.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/thread_test_helper.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/test_safe_browsing_service.h"
+#include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/network_delegate_impl.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/test/url_request/url_request_mock_data_job.h"
 #include "net/url_request/url_request_filter.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using certificate_reporting_test_utils::ReportExpectation;
+
 namespace {
 
 // Maximum number of reports kept in the certificate reporting service's retry
 // queue.
 const size_t kMaxReportCountInQueue = 3;
 
+const char* kFailedReportHistogram = "SSL.CertificateErrorReportFailure";
+
 void ClearURLHandlers() {
   net::URLRequestFilter::GetInstance()->ClearHandlers();
 }
 
+// A network delegate used to observe URL request destructions. The tests check
+// that no outstanding URL request is present during tear down.
+class TestNetworkDelegate : public net::NetworkDelegateImpl {
+ public:
+  TestNetworkDelegate(
+      const base::Callback<void()>& url_request_destroyed_callback)
+      : url_request_destroyed_callback_(url_request_destroyed_callback) {}
+
+  ~TestNetworkDelegate() override {}
+
+  // net::NetworkDelegate method:
+  void OnURLRequestDestroyed(net::URLRequest* request) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+    content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                     url_request_destroyed_callback_);
+  }
+
+ private:
+  base::Callback<void()> url_request_destroyed_callback_;
+};
+
+// Base class for histogram testing. The failed report histogram is checked once
+// after teardown to ensure all in flight requests have completed.
+class HistogramTestBase {
+ protected:
+  virtual ~HistogramTestBase() {}
+
+  // Sets the expected histogram value to be checked during teardown.

[estark, 2016/12/16 01:57:59]

nit: maybe make this SetExpectedHistogramFailedCountOnTeardown or something with
"failed" in it somewhere. It's a little confusing down below where it says
SetExpectedHistogramCount(0) but then reports get sent -- not clear that the
histogram only counts failed reports.

[meacer, 2016/12/16 20:26:36]

Changed it to SetExpectedFailedReportCount, added a helper method to the tests
with OnTearDown suffix.

+  void SetExpectedHistogramCountOnTeardown(int num_expected_failed_report) {
+    num_expected_failed_report_ = num_expected_failed_report;
+  }
+
+  void CheckHistogram() {
+    if (num_expected_failed_report_ != 0) {
+      histogram_tester_.ExpectUniqueSample(kFailedReportHistogram,
+                                           -net::ERR_SSL_PROTOCOL_ERROR,
+                                           num_expected_failed_report_);
+    } else {
+      histogram_tester_.ExpectTotalCount(kFailedReportHistogram, 0);
+    }
+  }
+
+ private:
+  int num_expected_failed_report_ = 0;

[estark, 2016/12/16 01:57:59]

unsigned int

[meacer, 2016/12/16 20:26:36]

Done.

+  base::HistogramTester histogram_tester_;
+};
+
 }  // namespace
 
 TEST(CertificateReportingServiceReportListTest, BoundedReportList) {
   // Create a report list with maximum of 2 items.
   CertificateReportingService::BoundedReportList list(2 /* max_size */);
   EXPECT_EQ(0u, list.items().size());
 
   // Add a ten minute old report.
   list.Add(CertificateReportingService::Report(
       1, base::Time::Now() - base::TimeDelta::FromMinutes(10),
@@ skipping 22 matching lines @@
   list.Add(CertificateReportingService::Report(
       0, base::Time::Now() - base::TimeDelta::FromMinutes(
                                  10) /* 5 minutes older than report2 */,
       std::string("report0_ten_minutes_old")));
   EXPECT_EQ(2u, list.items().size());
   EXPECT_EQ("report3_zero_minutes_old", list.items()[0].serialized_report);
   EXPECT_EQ("report2_five_minutes_old", list.items()[1].serialized_report);
 }
 
 class CertificateReportingServiceReporterOnIOThreadTest
-    : public ::testing::Test {
+    : public ::testing::Test,
+      public HistogramTestBase {

[estark, 2016/12/16 01:57:59]

Instead of the multiple inheritance, could the test fixtures have a
HistogramTestBase? (i.e. so they call histogram_test_->CheckHistogram() where
you're currently calling CheckHistogram())

[meacer, 2016/12/16 20:26:36]

Done.

  public:
   void SetUp() override {
     message_loop_.reset(new base::MessageLoopForIO());
     io_thread_.reset(new content::TestBrowserThread(content::BrowserThread::IO,
                                                     message_loop_.get()));
     url_request_context_getter_ =
         new net::TestURLRequestContextGetter(message_loop_->task_runner());
     net::URLRequestFailedJob::AddUrlHandler();
     net::URLRequestMockDataJob::AddUrlHandler();
   }
 
-  void TearDown() override { ClearURLHandlers(); }
+  void TearDown() override {
+    ClearURLHandlers();
+    // Check the histogram as the last thing. This makes sure no in-flight
+    // report is missed.
+    CheckHistogram();
+  }
 
  protected:
   net::URLRequestContextGetter* url_request_context_getter() {
     return url_request_context_getter_.get();
   }
 
  private:
   std::unique_ptr<base::MessageLoopForIO> message_loop_;
   std::unique_ptr<content::TestBrowserThread> io_thread_;
 
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
 };
 
 TEST_F(CertificateReportingServiceReporterOnIOThreadTest,
        Reporter_RetriesEnabled) {
+  SetExpectedHistogramCountOnTeardown(6);
+
   std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock());
   base::Time reference_time = base::Time::Now();
   clock->SetNow(reference_time);
 
   const GURL kFailureURL =
       net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_SSL_PROTOCOL_ERROR);
   certificate_reporting::ErrorReporter* certificate_error_reporter =
       new certificate_reporting::ErrorReporter(
           url_request_context_getter()->GetURLRequestContext(), kFailureURL,
           net::ReportSender::DO_NOT_SEND_COOKIES);
@@ skipping 77 matching lines @@
   clock->Advance(base::TimeDelta::FromSeconds(1));
   reporter.SendPending();
   EXPECT_EQ(1u, reporter.inflight_report_count_for_testing());
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(0u, list->items().size());
 }
 
 // Same as above, but retries are disabled.
 TEST_F(CertificateReportingServiceReporterOnIOThreadTest,
        Reporter_RetriesDisabled) {
+  SetExpectedHistogramCountOnTeardown(2);
+
   std::unique_ptr<base::SimpleTestClock> clock(new base::SimpleTestClock());
   base::Time reference_time = base::Time::Now();
   clock->SetNow(reference_time);
 
   const GURL kFailureURL =
       net::URLRequestFailedJob::GetMockHttpsUrl(net::ERR_SSL_PROTOCOL_ERROR);
   certificate_reporting::ErrorReporter* certificate_error_reporter =
       new certificate_reporting::ErrorReporter(
           url_request_context_getter()->GetURLRequestContext(), kFailureURL,
           net::ReportSender::DO_NOT_SEND_COOKIES);
@@ skipping 28 matching lines @@
   // Send pending reports. Nothing should be sent.
   clock->Advance(base::TimeDelta::FromSeconds(10));
   reporter.SendPending();
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(0u, list->items().size());
 }
 
 class CertificateReportingServiceTest
     : public ::testing::Test,
       public certificate_reporting_test_utils::
-          CertificateReportingServiceTestBase {
+          CertificateReportingServiceTestBase,
+      public HistogramTestBase {

[estark, 2016/12/16 01:57:59]

same here

[meacer, 2016/12/16 20:26:36]

Done.

  public:
   CertificateReportingServiceTest()
       : CertificateReportingServiceTestBase(),
         thread_bundle_(content::TestBrowserThreadBundle::REAL_IO_THREAD),
         io_task_runner_(content::BrowserThread::GetTaskRunnerForThread(
             content::BrowserThread::IO)) {}
 
   ~CertificateReportingServiceTest() override {}
 
   void SetUp() override {
     SetUpInterceptor();
     WaitForIOThread();
 
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
         base::Bind(
             &CertificateReportingServiceTest::SetUpURLRequestContextOnIOThread,
             base::Unretained(this)));
     WaitForIOThread();
 
-    clock_ = new base::SimpleTestClock();
+    safe_browsing::SafeBrowsingService::RegisterFactory(&sb_service_factory);
+    sb_service_ = sb_service_factory.CreateSafeBrowsingService();
+
+    clock_.reset(new base::SimpleTestClock());
     service_.reset(new CertificateReportingService(
-        url_request_context_getter(), server_public_key(),
-        server_public_key_version(), kMaxReportCountInQueue,
-        base::TimeDelta::FromHours(24), std::unique_ptr<base::Clock>(clock_)));
+        sb_service_.get(), url_request_context_getter(), &profile_,
+        server_public_key(), server_public_key_version(),
+        kMaxReportCountInQueue, base::TimeDelta::FromHours(24), clock_.get()));
     // Wait for service reset.
     WaitForIOThread();
   }
 
-  void SetUpURLRequestContextOnIOThread() {
-    std::unique_ptr<net::TestURLRequestContext> url_request_context(
-        new net::TestURLRequestContext(true));
-    url_request_context->set_network_delegate(network_delegate());
-    url_request_context->Init();
-    url_request_context_getter_ = new net::TestURLRequestContextGetter(
-        io_task_runner_, std::move(url_request_context));
-  }
-
   void TearDown() override {
     WaitForIOThread();
     EXPECT_TRUE(interceptor()->successful_reports().empty());
     EXPECT_TRUE(interceptor()->failed_reports().empty());
     EXPECT_TRUE(interceptor()->delayed_reports().empty());
     EXPECT_EQ(0u, service()
                       ->GetReporterForTesting()
                       ->inflight_report_count_for_testing());
 
     service_->Shutdown();
     WaitForIOThread();
 
     service_.reset(nullptr);
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&CertificateReportingServiceTest::TearDownOnIOThread,
+                   base::Unretained(this)));

[estark, 2016/12/16 01:57:59]

This looks a little sketchy to me. I'd think we need to wait, otherwise `this`
could be destroyed before TearDownOnIOThread() runs.

Needs a WaitForIOThread afterwards, maybe?

[meacer, 2016/12/16 20:26:36]

Done.

     content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
                                      base::Bind(&ClearURLHandlers));
-    TearDownInterceptor();
+
+    CheckHistogram();
   }
 
  protected:
+  void WaitForRequestsDestroyed(const ReportExpectation& expectation) {
+    wait_helper_.Wait(expectation.num_reports());
+    EXPECT_EQ(expectation.successful_reports,
+              interceptor()->successful_reports());
+    EXPECT_EQ(expectation.failed_reports, interceptor()->failed_reports());
+    EXPECT_EQ(expectation.delayed_reports, interceptor()->delayed_reports());
+    interceptor()->ClearObservedReports();
+  }
+
   net::URLRequestContextGetter* url_request_context_getter() {
     return url_request_context_getter_.get();
   }
 
   void WaitForIOThread() {
     scoped_refptr<base::ThreadTestHelper> io_helper(
         new base::ThreadTestHelper(io_task_runner_));
     ASSERT_TRUE(io_helper->Run());
   }
 
+  CertificateReportingService* service() { return service_.get(); }
+
   // Sets service enabled state and waits for a reset event.
   void SetServiceEnabledAndWait(bool enabled) {
     service()->SetEnabled(enabled);
     WaitForIOThread();
   }
 
   void AdvanceClock(base::TimeDelta delta) {
     content::BrowserThread::PostTask(
         content::BrowserThread::IO, FROM_HERE,
-        base::Bind(&base::SimpleTestClock::Advance, base::Unretained(clock_),
-                   delta));
+        base::Bind(&base::SimpleTestClock::Advance,
+                   base::Unretained(clock_.get()), delta));
   }
 
   void SetNow(base::Time now) {
-    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                     base::Bind(&base::SimpleTestClock::SetNow,
-                                                base::Unretained(clock_), now));
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(&base::SimpleTestClock::SetNow,
+                   base::Unretained(clock_.get()), now));
   }
 
-  CertificateReportingService* service() { return service_.get(); }
+ private:
+  void SetUpURLRequestContextOnIOThread() {
+    network_delegate_.reset(new TestNetworkDelegate(
+        base::Bind(&CertificateReportingServiceTest::OnURLRequestDestroyed,
+                   base::Unretained(this))));
 
- private:
+    std::unique_ptr<net::TestURLRequestContext> url_request_context(
+        new net::TestURLRequestContext(true));
+    url_request_context->set_network_delegate(network_delegate_.get());
+    url_request_context->Init();
+    url_request_context_getter_ = new net::TestURLRequestContextGetter(
+        io_task_runner_, std::move(url_request_context));
+  }
+
+  void TearDownOnIOThread() {
+    url_request_context_getter_ = nullptr;
+    network_delegate_.reset(nullptr);
+  }
+
+  void OnURLRequestDestroyed() {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    wait_helper_.OnEvent();
+  }
+
   // Must be initialized before url_request_context_getter_
   content::TestBrowserThreadBundle thread_bundle_;
 
+  std::unique_ptr<TestNetworkDelegate> network_delegate_;
+  certificate_reporting_test_utils::ReportWaitHelper wait_helper_;
+
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
 
   std::unique_ptr<CertificateReportingService> service_;
-  base::SimpleTestClock* clock_;
+  std::unique_ptr<base::SimpleTestClock> clock_;
+
+  scoped_refptr<safe_browsing::SafeBrowsingService> sb_service_;
+  safe_browsing::TestSafeBrowsingServiceFactory sb_service_factory;
+  TestingProfile profile_;
 };
 
 TEST_F(CertificateReportingServiceTest, Send) {
+  SetExpectedHistogramCountOnTeardown(4);
+
   WaitForIOThread();
   // Let all reports fail.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_FAIL);
 
   // Send two reports. Both should fail and get queued.
   service()->Send("report0");
   WaitForRequestsDestroyed(ReportExpectation::Failed({"report0"}));
 
   service()->Send("report1");
@@ skipping 13 matching lines @@
   WaitForRequestsDestroyed(ReportExpectation::Successful({"report2"}));
 
   // Send pending reports. Previously failed and queued two reports should be
   // observed.
   service()->SendPending();
   WaitForRequestsDestroyed(
       ReportExpectation::Successful({"report0", "report1"}));
 }
 
 TEST_F(CertificateReportingServiceTest, Disabled_ShouldNotSend) {
+  SetExpectedHistogramCountOnTeardown(0);
+
   // Let all reports succeed.
   SetFailureMode(certificate_reporting_test_utils::ReportSendingResult::
                      REPORTS_SUCCESSFUL);
 
   // Disable the service.
   SetServiceEnabledAndWait(false);
 
   // Send a report. Report attempt should be cancelled and no sent reports
   // should be observed.
   service()->Send("report0");
 
-  // Enable the service and send a report again.
+  // Enable the service and send a report again. It should be sent successfully.
   SetServiceEnabledAndWait(true);
 
   service()->Send("report1");
   WaitForRequestsDestroyed(ReportExpectation::Successful({"report1"}));
 }
 
 TEST_F(CertificateReportingServiceTest, Disabled_ShouldClearPendingReports) {
+  SetExpectedHistogramCountOnTeardown(1);
+
   // Let all reports fail.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_FAIL);
 
   service()->Send("report0");
   WaitForRequestsDestroyed(ReportExpectation::Failed({"report0"}));
 
   // Disable the service.
   SetServiceEnabledAndWait(false);
 
-  // Sending has no effect while disabled, wait for a single cancelled event.
+  // Sending has no effect while disabled.
   service()->SendPending();
 
   // Re-enable the service and send pending reports. Pending reports should have
   // been cleared when the service was disabled, so no report should be seen.
   SetServiceEnabledAndWait(true);
 
   // Sending with empty queue has no effect.
   service()->SendPending();
 }
 
 TEST_F(CertificateReportingServiceTest, DontSendOldReports) {
+  SetExpectedHistogramCountOnTeardown(5);
+
   SetNow(base::Time::Now());
   // Let all reports fail.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_FAIL);
 
   // Send a report.
   service()->Send("report0");
   WaitForRequestsDestroyed(ReportExpectation::Failed({"report0"}));
 
   // Advance the clock a bit and trigger another report.
@@ skipping 24 matching lines @@
 
   // Advance the clock 20 hours again so that report2 is 25 hours old and is
   // older than max age (24 hours)
   AdvanceClock(base::TimeDelta::FromHours(20));
   // Send pending reports. report2 should be discarded since it's too old. No
   // other reports remain.
   service()->SendPending();
 }
 
 TEST_F(CertificateReportingServiceTest, DiscardOldReports) {
+  SetExpectedHistogramCountOnTeardown(7);
+
   SetNow(base::Time::Now());
   // Let all reports fail.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_FAIL);
 
   // Send a failed report.
   service()->Send("report0");
   WaitForRequestsDestroyed(ReportExpectation::Failed({"report0"}));
 
   // Send three more reports within five hours of each other. After this:
@@ skipping 34 matching lines @@
   service()->SendPending();
   WaitForRequestsDestroyed(
       ReportExpectation::Successful({"report2", "report3"}));
 
   // Do a final send. No reports should be sent.
   service()->SendPending();
 }
 
 // A delayed report should successfully upload when it's resumed.
 TEST_F(CertificateReportingServiceTest, Delayed_Resumed) {
+  SetExpectedHistogramCountOnTeardown(0);
+
   // Let reports hang.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_DELAY);
   // Send a report. The report upload hangs, so no error or success callbacks
   // should be called.
   service()->Send("report0");
 
   // Resume the report upload and run the callbacks. The report should be
   // successfully sent.
-  ResumeDelayedRequest();
+  ResumeDelayedRequest(base::Bind(&base::DoNothing));
   WaitForRequestsDestroyed(ReportExpectation::Delayed({"report0"}));
 }
 
 // Delayed reports should cleaned when the service is reset.
 TEST_F(CertificateReportingServiceTest, Delayed_Reset) {
+  SetExpectedHistogramCountOnTeardown(0);
+
   // Let reports hang.
   SetFailureMode(
       certificate_reporting_test_utils::ReportSendingResult::REPORTS_DELAY);
   // Send a report. The report is triggered but hangs, so no error or success
   // callbacks should be called.
   service()->Send("report0");
 
   // Disable the service. This should reset the reporting service and
   // clear all pending reports.
   SetServiceEnabledAndWait(false);
 
   // Resume delayed report. No report should be observed since the service
   // should have reset and all pending reports should be cleared. If any report
   // is observed, the next WaitForRequestsDestroyed() will fail.
-  ResumeDelayedRequest();
+  ResumeDelayedRequest(base::Bind(&base::DoNothing));
 
   // Enable the service.
   SetServiceEnabledAndWait(true);
 
   // Send a report. The report is triggered but hangs, so no error or success
   // callbacks should be called. The report id is again 0 since the pending
   // report queue has been cleared above.
   service()->Send("report1");
 
-  // Resume delayed report. The report should be observed.
-  ResumeDelayedRequest();
+  // Resume delayed report. Two reports are successfully sent.
+  ResumeDelayedRequest(base::Bind(&base::DoNothing));
   WaitForRequestsDestroyed(ReportExpectation::Delayed({"report0", "report1"}));
 }