Wire up CertificateReportingService to handle report uploads

chrome/browser/safe_browsing/safe_browsing_service.h

Index: chrome/browser/safe_browsing/safe_browsing_service.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index 27e15b03e9de51ac031f5fda6557acc9ee0be8dc..83381b87f64fbf6bc35ac53a749639ea32a78988 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -121,7 +121,7 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
     return services_delegate_->GetDownloadService();
   }
 
-  net::URLRequestContextGetter* url_request_context();
+  scoped_refptr<net::URLRequestContextGetter> url_request_context();

[estark, 2016/12/16 23:39:04]

why is this change needed?

[meacer, 2016/12/17 01:23:21]

This is so that we can pass the request context between threads when
initializing CertificateReportingService. The service is created in the UI
thread but its reporter (which uses the request context) is initialized on the
IO thread.

The alternative is to defer initialization of CertificateReportingService, add
an Initialize method to it and pass the URL request context to that method. The
Initialize method will need to be called on the IO thread which seemed ugly to
me, as the public interface of CertificateReportingService is only called on the
UI thread.

Also, I should note that CertificateReportingService doesn't hold on to this
getter. It gets the URLRequestContext from this getter as a raw pointer and uses
it in the IO thread, without adding a reference to url_request_context (which is
actually a URLRequestContextGetter and not a URLRequestContext).

[meacer, 2016/12/19 23:16:45]

I think the current implementation is buggy: There's existing code that's
keeping scoped_refptr to his url_request_context. There is a small chance that
those could drop all references which would free url_request_context even though
it shouldn't be freed.

E.g.
https://cs.chromium.org/chromium/src/chrome/browser/extensions/blacklist_stat...

|url_request_context_getter_| is a scoped_refptr and can end up freeing
SafeBrowsingService's url_request_context_getter.

[estark, 2016/12/22 00:11:44]

Acknowledged.

 
   const scoped_refptr<SafeBrowsingUIManager>& ui_manager() const;
 
@@ -159,6 +159,7 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
 
   // Type for subscriptions to SafeBrowsing service state.
   typedef base::CallbackList<void(void)>::Subscription StateSubscription;
+  typedef base::CallbackList<void(void)>::Subscription ShutdownSubscription;
 
   // Adds a listener for when SafeBrowsing preferences might have changed.
   // To get the current state, the callback should call enabled_by_prefs().
@@ -166,6 +167,12 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   std::unique_ptr<StateSubscription> RegisterStateCallback(
       const base::Callback<void(void)>& callback);
 
+  // Adds a listener for when SafeBrowsingService starts shutting down.
+  // The callbacks run on the UI thread, and give the subscribers an opportunity
+  // to clean up any references they hold to SafeBrowsingService.
+  std::unique_ptr<ShutdownSubscription> RegisterShutdownCallback(
+      const base::Callback<void(void)>& callback);
+
   // Sends serialized download report to backend.
   virtual void SendSerializedDownloadReport(const std::string& report);
 
@@ -283,6 +290,10 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   // Should only be accessed on the UI thread.
   base::CallbackList<void(void)> state_callback_list_;
 
+  // Callbacks when SafeBrowsing service starts shutting down.
+  // Should only be accessed on the UI thread.
+  base::CallbackList<void(void)> shutdown_callback_list_;
+
   // The UI manager handles showing interstitials.  Accessed on both UI and IO
   // thread.
   scoped_refptr<SafeBrowsingUIManager> ui_manager_;