Fix crash during CertificateReportingService shutdown

CertificateReportingService::ShutDown posts a task to the IO thread to
delete reporter_. If the service's destructor is already run, this will
cause a crash during shutdown. Instead, delete reporter_ using a static
method and without referencing CertificateReportingService's |this|.

BUG=677329

Review-Url: https://codereview.chromium.org/2601203002
Cr-Commit-Position: refs/heads/master@{#440933}

chrome/browser/safe_browsing/certificate_reporting_service.cc

@@ -35,6 +35,11 @@ void RecordUMAOnFailure(int net_error) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("SSL.CertificateErrorReportFailure", -net_error);
 }
 
+void CleanupOnIOThread(
+    std::unique_ptr<CertificateReportingService::Reporter> reporter) {
+  reporter.reset();
+}
+
 }  // namespace
 
 CertificateReportingService::BoundedReportList::BoundedReportList(
@@ -164,7 +169,6 @@ CertificateReportingService::CertificateReportingService(
     base::TimeDelta max_report_age,
     base::Clock* clock)
     : pref_service_(*profile->GetPrefs()),
-      enabled_(true),
       url_request_context_(nullptr),
       max_queued_report_count_(max_queued_report_count),
       max_report_age_(max_report_age),
@@ -187,7 +191,7 @@ CertificateReportingService::CertificateReportingService(
   content::BrowserThread::PostTask(
       content::BrowserThread::IO, FROM_HERE,
       base::Bind(&CertificateReportingService::InitializeOnIOThread,
-                 base::Unretained(this), enabled_, url_request_context_getter,
+                 base::Unretained(this), true, url_request_context_getter,
                  max_queued_report_count_, max_report_age_, clock_,
                  server_public_key_, server_public_key_version_));
 }
@@ -199,8 +203,10 @@ CertificateReportingService::~CertificateReportingService() {
 void CertificateReportingService::Shutdown() {
   // Shutdown will be called twice: Once after SafeBrowsing shuts down, and once
   // when all KeyedServices shut down. All calls after the first one are no-op.
-  enabled_ = false;
-  Reset();
+  url_request_context_ = nullptr;
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&CleanupOnIOThread, base::Passed(std::move(reporter_))));
 }
 
 void CertificateReportingService::Send(const std::string& serialized_report) {
@@ -243,8 +249,16 @@ void CertificateReportingService::InitializeOnIOThread(
 
 void CertificateReportingService::SetEnabled(bool enabled) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  enabled_ = enabled;
-  Reset();
+  // Don't reset if the service is already shut down.
+  if (!url_request_context_)
+    return;
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&CertificateReportingService::ResetOnIOThread,
+                 base::Unretained(this), enabled, url_request_context_,
+                 max_queued_report_count_, max_report_age_, clock_,
+                 server_public_key_, server_public_key_version_));
 }
 
 CertificateReportingService::Reporter*
@@ -257,15 +271,6 @@ GURL CertificateReportingService::GetReportingURLForTesting() {
   return GURL(kExtendedReportingUploadUrl);
 }
 
-void CertificateReportingService::Reset() {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::Bind(&CertificateReportingService::ResetOnIOThread,
-                 base::Unretained(this), enabled_, url_request_context_,
-                 max_queued_report_count_, max_report_age_, clock_,
-                 server_public_key_, server_public_key_version_));
-}
-
 void CertificateReportingService::ResetOnIOThread(
     bool enabled,
     net::URLRequestContext* url_request_context,
@@ -277,7 +282,7 @@ void CertificateReportingService::ResetOnIOThread(
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   // url_request_context_ is null during shutdown.
   if (!enabled || !url_request_context) {
-    reporter_.reset(nullptr);
+    reporter_.reset();
     return;
   }
   std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter;
@@ -293,7 +298,6 @@ void CertificateReportingService::ResetOnIOThread(
         url_request_context, GURL(kExtendedReportingUploadUrl),
         net::ReportSender::DO_NOT_SEND_COOKIES));
   }
-
   reporter_.reset(
       new Reporter(std::move(error_reporter),
                    std::unique_ptr<BoundedReportList>(

chrome/browser/safe_browsing/certificate_reporting_service.h

@@ -104,7 +104,7 @@ class CertificateReportingService : public KeyedService {
     Reporter(
         std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_,
         std::unique_ptr<BoundedReportList> retry_list,
-        base::Clock* clock,
+        base::Clock* const clock,
         base::TimeDelta report_ttl,
         bool retries_enabled);
     ~Reporter();
@@ -129,7 +129,7 @@ class CertificateReportingService : public KeyedService {
 
     std::unique_ptr<certificate_reporting::ErrorReporter> error_reporter_;
     std::unique_ptr<BoundedReportList> retry_list_;
-    base::Clock* clock_;
+    base::Clock* const clock_;
     // Maximum age of a queued report. Reports older than this are discarded in
     // the next SendPending() call.
     const base::TimeDelta report_ttl_;
@@ -176,14 +176,12 @@ class CertificateReportingService : public KeyedService {
   static GURL GetReportingURLForTesting();
 
  private:
-  void Reset();
-
   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* clock,
+      base::Clock* const clock,
       uint8_t* server_public_key,
       uint32_t server_public_key_version);
 
@@ -196,18 +194,13 @@ class CertificateReportingService : public KeyedService {
                        net::URLRequestContext* url_request_context,
                        size_t max_queued_report_count,
                        base::TimeDelta max_report_age,
-                       base::Clock* clock,
+                       base::Clock* const clock,
                        uint8_t* server_public_key,
                        uint32_t server_public_key_version);
 
   void OnPreferenceChanged();
 
   const PrefService& pref_service_;
-
-  // If true, reporting is enabled. When SafeBrowsing preferences change, this
-  // might be set to false.
-  bool enabled_;
-
   net::URLRequestContext* url_request_context_;
   std::unique_ptr<Reporter> reporter_;
 
@@ -224,14 +217,14 @@ class CertificateReportingService : public KeyedService {
       safe_browsing_state_subscription_;
 
   // Maximum number of reports to be queued for retry.
-  size_t max_queued_report_count_;
+  const 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_;
+  const base::TimeDelta max_report_age_;
 
-  base::Clock* clock_;
+  base::Clock* const clock_;
 
   // Encryption parameters.
   uint8_t* server_public_key_;