Implement main CertificateReportingService code and add unit tests.

chrome/browser/safe_browsing/certificate_reporting_service.h

 // 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.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_CERTIFICATE_REPORTING_SERVICE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_CERTIFICATE_REPORTING_SERVICE_H_
 
 #include <map>
 #include <memory>
 #include <string>
 #include <vector>
 
+#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "components/certificate_reporting/error_reporter.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace base {
 class Clock;
 }
 
+namespace net {
+class URLRequestContext;
+class URLRequestContextGetter;
+}
+
 // This service initiates uploads of invalid certificate reports and retries any
-// failed uploads.
+// failed uploads. Each report is retried until it's older than a certain time
+// to live (TTL). Reports older than this TTL are dropped and no more retried,
+// so that the retry list doesn't grow indefinitely.
+//
+// Lifetime and dependencies:
+//
+// CertificateReportingService uses the url request context from SafeBrowsing
+// service. SafeBrowsing service is created before this service, but is also
+// shut down before any KeyedService is shut down. This means that this class
+// cannot depend on SafeBrowsing's url request being available at all times, and
+// it should know when SafeBrowsing shuts down. ChromeContentBrowserClient
+// subscribes this service to SafeBrowsing service shut downs during
+// initialization, and this service shuts down when SafeBrowsing shuts down.
 class CertificateReportingService : public KeyedService {
  public:
-  // Represent a report to be sent.
+  // Represents a report to be sent.
   struct Report {
     int report_id;
     base::Time creation_time;
     std::string serialized_report;
     Report(int report_id,
            base::Time creation_time,
            const std::string& serialized_report)
         : report_id(report_id),
           creation_time(creation_time),
           serialized_report(serialized_report) {}
@@ skipping 18 matching lines @@
 
    private:
     // Maximum number of reports in the list. If the number of reports in the
     // list is smaller than this number, a new item is immediately added to the
     // list. Otherwise, the item is compared to the items in the list and only
     // added when it's newer than the oldest item in the list.
     const size_t max_size_;
 
     std::vector<Report> items_;
     base::ThreadChecker thread_checker_;
+
+    DISALLOW_COPY_AND_ASSIGN(BoundedReportList);
+  };
+
+  // A class to observe events by the service. Used for testing.
+  class EventObserver {
+   public:
+    EventObserver() {}
+    virtual ~EventObserver() {}
+
+    // Called when sending of a report is attempted. If attempt was cancelled,
+    // |completed| is false. Otherwise, it's true.
+    virtual void OnSendAttempt(bool completed) {}
+    // Called when sending a report is completed. If attempt was successful,
+    // |success| is true. Otherwise, it's false.
+    virtual void OnSendComplete(int report_id, bool success) {}
+    // Called when reporter is created. This can happen when changing
+    // SafeBrowsing or extended reporting preferences.
+    virtual void OnCreated() {}
+    // The service is being reset because SafeBrowsing preferences have changed.
+    virtual void OnReset() {}
   };
 
   // Class that handles report uploads and implements the upload retry logic.
   class Reporter {

[estark, 2016/12/01 01:46:53]

Why does this need to be its own class?

[meacer, 2016/12/06 02:22:34]

I think there is better separation this way. In particular:

- I can unit test the Reporter class independent of the Service itself. The
previous CL only implemented the reporter and had a unittest for it without
having the service.
- The reporter gets recreated each time SB preferences change, clearing the
queue and pending requests.
- The reporter lives in the IO thread so it's a bit easier to reason about. The
service just calls reporter methods on the right thread.

    public:
     Reporter(
         std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_,
         std::unique_ptr<BoundedReportList> retry_list,
         base::Clock* clock,
-        base::TimeDelta report_ttl);
+        base::TimeDelta report_ttl,
+        EventObserver* observer,
+        bool retries_enabled);
     ~Reporter();
 
     // Sends a report. If the send fails, the report will be added to the retry
     // list.
     void Send(const std::string& serialized_report);
 
     // Sends all pending reports. Skips reports older than the |report_ttl|
     // provided in the constructor. Failed reports will be added to the retry
     // list.
     void SendPending();
 
     // Getter and setters for testing:
     size_t inflight_report_count_for_testing() const;
     BoundedReportList* GetQueueForTesting() const;
+    void SetEventObserverForTesting(EventObserver* observer);
 
    private:
     void SendInternal(const Report& report);
     void ErrorCallback(int report_id, const GURL& url, int error);
     void SuccessCallback(int report_id);
 
     std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_;
     std::unique_ptr<BoundedReportList> retry_list_;
     base::Clock* test_clock_;
+    // Maximum age of a queued report. Reports older than this are discarded in
+    // the next |SendPending| call.

[estark, 2016/12/01 01:46:53]

very tiny nit: |blah| is only supposed to be used for variables IIRC, so
|SendPending| => SendPending()

[meacer, 2016/12/07 21:37:33]

Done.

     const base::TimeDelta report_ttl_;
+    // Event observer to observe events generated by the reporter. Can be null.
+    EventObserver* event_observer_;
+    // If true, retries are enabled.

[estark, 2016/12/01 01:46:53]

nit: I think you could remove this comment, doesn't add much beyond the variable
name

[meacer, 2016/12/07 21:37:33]

Done.

+    const bool retries_enabled_;
+    // Current report id, starting from zero and monotonically incrementing.
     int current_report_id_;
 
     std::map<int, Report> inflight_reports_;
 
     base::WeakPtrFactory<Reporter> weak_factory_;
 
-    DISALLOW_IMPLICIT_CONSTRUCTORS(Reporter);
+    DISALLOW_COPY_AND_ASSIGN(Reporter);
   };
+
+  CertificateReportingService(
+      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      std::unique_ptr<EventObserver> event_observer,
+      size_t max_queued_report_count,
+      base::TimeDelta max_report_age,
+      base::Clock* test_clock);
+
+  ~CertificateReportingService() override;
+
+  // KeyedService implementation:
+  void Shutdown() override;
+
+  // Sends a serialized report. If the report upload fails, the upload is
+  // retried in a future time.

[estark, 2016/12/01 01:46:53]

nit: in => at

[meacer, 2016/12/07 21:37:32]

Done.

+  void Send(const std::string& serialized_report);
+
+  // Sends pending reports that are in the retry queue.
+  void SendPending();
+
+  // Enables or disables reporting. When disabled, pending report queue is
+  // cleared and incoming reports are ignored. Reporting is enabled by default
+  // once the service is initialized.
+  void SetEnabled(bool enabled);
+
+  // Called when a send attempt is made. Public so that clients can notify this
+  // service that they abandoned a send attempt.
+  void DidAttemptSend(bool sent);
+
+  // Getters and setters for testing.
+  Reporter* get_reporter_for_testing() const;
+  GURL GetReportingURLForTesting() const;
+  void SetEventObserverForTesting(std::unique_ptr<EventObserver> observer);
+  void SetMaxQueuedReportCountForTesting(size_t max_report_count);
+  void SetClockForTesting(base::Clock* clock);
+  void SetMaxReportAgeForTesting(base::TimeDelta max_report_age);
+
+  // URL to upload invalid certificate chain reports. An HTTP URL is

[estark, 2016/12/01 01:46:53]

nit: I don't think this detail about the HTTP URL is necessary for the public
header. Maybe drop the |Insecure| from the name, and move this comment into the
.cc where you set it to an http:// URL?

[meacer, 2016/12/07 21:37:33]

Done.

+  // used because a client seeing an invalid cert might not be able to
+  // make an HTTPS connection to report it.
+  static const char kExtendedReportingUploadUrlInsecure[];
+
+ private:
+  void Reset(const base::Callback<void()>& callback);
+
+  void InitializeOnIOThread(
+      bool enabled,
+      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      size_t max_queued_report_count,
+      base::TimeDelta max_report_age,
+      base::Clock* test_clock,
+      EventObserver* event_observer);
+
+  // Resets the reporter on the IO thread. Changes in SafeBrowsing or extended
+  // reporting enabled states cause the reporter to be reset.
+  // If |enabled| is false or |url_request_context_getter| is null, report is
+  // set to null, effectively cancelling all in flight uploads and clearing the
+  // pending reports queue.
+  void ResetOnIOThread(bool enabled,
+                       net::URLRequestContext* url_request_context,
+                       size_t max_queued_report_count,
+                       base::TimeDelta max_report_age,
+                       base::Clock* test_clock,
+                       EventObserver* event_observer);
+
+  // If true, reporting is enabled. When SafeBrowsing preferences change, this
+  // might be set to false.
+  bool enabled_;
+
+  // scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;

[estark, 2016/12/01 01:46:53]

should be deleted?

[meacer, 2016/12/07 21:37:33]

Done.

+  net::URLRequestContext* url_request_context_;
+
+  std::unique_ptr<Reporter> reporter_;
+
+  // Subscription for url request context shutdowns. When this subscription is
+  // notified, it means that the SafeBrowsing service is shutting down, and this
+  // service must also shut down.
+  std::unique_ptr<base::CallbackList<void(void)>::Subscription>
+      safe_browsing_service_shutdown_subscription_;

[estark, 2016/12/01 01:46:53]

this is not hooked up yet, correct? (no change needed, just wanting to make sure
I understand)

[meacer, 2016/12/07 21:37:33]

Yes, removed from this CL.

+
+  // Observes events from this service. Default implementation doesn't do
+  // anything. Tests use this to keep track of sent/failed reports etc.
+  std::unique_ptr<EventObserver> event_observer_;
+
+  // Maximum number of reports to be queued for retry.
+  size_t max_queued_report_count_;
+
+  // Maximum age of the reports to be queued for retry, from the time the
+  // certificate error was first encountered by the user. Any report older than
+  // this age is ignored and is not re-uploaded.
+  base::TimeDelta max_report_age_;
+
+  // Test clock. If null, system clock is used.
+  base::Clock* test_clock_;
+
+  // Whether a send has ever been made. Used to verify that test setters are
+  // only called after initialization.
+  bool made_send_attempt_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertificateReportingService);
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_CERTIFICATE_REPORTING_SERVICE_H_