Make SSLErrorHandler UI-thread-only and not-refcounted

content/browser/ssl/ssl_policy.cc

 // Copyright (c) 2012 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 "content/browser/ssl/ssl_policy.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/memory/singleton.h"
@@ skipping 17 matching lines @@
 namespace content {
 
 namespace {
 
 // Events for UMA. Do not reorder or change!
 enum SSLGoodCertSeenEvent {
   NO_PREVIOUS_EXCEPTION = 0,
   HAD_PREVIOUS_EXCEPTION = 1,
   SSL_GOOD_CERT_SEEN_EVENT_MAX = 2
 };
+
+void OnAllowCertificate(SSLErrorHandler* handler,
+                        const SSLPolicy* const policy,
+                        CertificateRequestResultType decision) {
+  DCHECK(handler->ssl_info().is_valid());
+  switch (decision) {
+    case CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE:
+      // Note that we should not call SetMaxSecurityStyle here, because the
+      // active

[nasko, 2016/08/08 20:57:06]

nit: Let's reflow the comment since it is moving anyway. No need to waste the
rest of that empty line.

[estark, 2016/08/09 05:35:01]

Done.

+      // NavigationEntry has just been deleted (in HideInterstitialPage) and the
+      // new NavigationEntry will not be set until DidNavigate.  This is ok,
+      // because the new NavigationEntry will have its max security style set
+      // within DidNavigate.
+      //
+      // While AllowCertForHost() executes synchronously on this thread,
+      // ContinueRequest() gets posted to a different thread. Calling
+      // AllowCertForHost() first ensures deterministic ordering.
+      policy->backend()->AllowCertForHost(*handler->ssl_info().cert.get(),
+                                          handler->request_url().host(),
+                                          handler->cert_error());
+      handler->ContinueRequest();
+      return;
+    case CERTIFICATE_REQUEST_RESULT_TYPE_DENY:
+      handler->DenyRequest();
+      return;
+    case CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL:
+      handler->CancelRequest();
+      return;
+  }
 }
 
+}  // namespace
+
 SSLPolicy::SSLPolicy(SSLPolicyBackend* backend)
     : backend_(backend) {
   DCHECK(backend_);
 }
 
-void SSLPolicy::OnCertError(SSLErrorHandler* handler) {
+void SSLPolicy::OnCertError(std::unique_ptr<SSLErrorHandler> handler) {
   bool expired_previous_decision = false;
   // First we check if we know the policy for this error.
   DCHECK(handler->ssl_info().is_valid());
   SSLHostStateDelegate::CertJudgment judgment =
       backend_->QueryPolicy(*handler->ssl_info().cert.get(),
                             handler->request_url().host(),
                             handler->cert_error(),
                             &expired_previous_decision);
 
   if (judgment == SSLHostStateDelegate::ALLOWED) {
@@ skipping 12 matching lines @@
     case net::ERR_CERT_WEAK_KEY:
     case net::ERR_CERT_NAME_CONSTRAINT_VIOLATION:
     case net::ERR_CERT_VALIDITY_TOO_LONG:
     case net::ERR_CERTIFICATE_TRANSPARENCY_REQUIRED:
       if (!handler->fatal())
         options_mask |= OVERRIDABLE;
       else
         options_mask |= STRICT_ENFORCEMENT;
       if (expired_previous_decision)
         options_mask |= EXPIRED_PREVIOUS_DECISION;
-      OnCertErrorInternal(handler, options_mask);
+      OnCertErrorInternal(std::move(handler), options_mask);
       break;
     case net::ERR_CERT_NO_REVOCATION_MECHANISM:
       // Ignore this error.
       handler->ContinueRequest();
       break;
     case net::ERR_CERT_UNABLE_TO_CHECK_REVOCATION:
       // We ignore this error but will show a warning status in the location
       // bar.
       handler->ContinueRequest();
       break;
     case net::ERR_CERT_CONTAINS_ERRORS:
     case net::ERR_CERT_REVOKED:
     case net::ERR_CERT_INVALID:
     case net::ERR_SSL_WEAK_SERVER_EPHEMERAL_DH_KEY:
     case net::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN:
       if (handler->fatal())
         options_mask |= STRICT_ENFORCEMENT;
       if (expired_previous_decision)
         options_mask |= EXPIRED_PREVIOUS_DECISION;
-      OnCertErrorInternal(handler, options_mask);
+      OnCertErrorInternal(std::move(handler), options_mask);
       break;
     default:
       NOTREACHED();
       handler->CancelRequest();
       break;
   }
 }
 
 void SSLPolicy::DidRunInsecureContent(NavigationEntryImpl* entry,
                                       const GURL& security_origin) {
@@ skipping 78 matching lines @@
   // Minor errors don't lower the security style to
   // SECURITY_STYLE_AUTHENTICATION_BROKEN.
   if (net::IsCertStatusError(cert_status) &&
       !net::IsCertStatusMinorError(cert_status)) {
     return SECURITY_STYLE_AUTHENTICATION_BROKEN;
   }
 
   return SECURITY_STYLE_AUTHENTICATED;
 }
 
-void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLErrorHandler> handler,
-                                   CertificateRequestResultType decision) {
-  DCHECK(handler->ssl_info().is_valid());
-  switch (decision) {
-    case CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE:
-      // Note that we should not call SetMaxSecurityStyle here, because the
-      // active
-      // NavigationEntry has just been deleted (in HideInterstitialPage) and the
-      // new NavigationEntry will not be set until DidNavigate.  This is ok,
-      // because the new NavigationEntry will have its max security style set
-      // within DidNavigate.
-      //
-      // While AllowCertForHost() executes synchronously on this thread,
-      // ContinueRequest() gets posted to a different thread. Calling
-      // AllowCertForHost() first ensures deterministic ordering.
-      backend_->AllowCertForHost(*handler->ssl_info().cert.get(),
-                                 handler->request_url().host(),
-                                 handler->cert_error());
-      handler->ContinueRequest();
-      return;
-    case CERTIFICATE_REQUEST_RESULT_TYPE_DENY:
-      handler->DenyRequest();
-      return;
-    case CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL:
-      handler->CancelRequest();
-      return;
-  }
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Certificate Error Routines
 
-void SSLPolicy::OnCertErrorInternal(SSLErrorHandler* handler,
+void SSLPolicy::OnCertErrorInternal(std::unique_ptr<SSLErrorHandler> handler,
                                     int options_mask) {
   bool overridable = (options_mask & OVERRIDABLE) != 0;
   bool strict_enforcement = (options_mask & STRICT_ENFORCEMENT) != 0;
   bool expired_previous_decision =
       (options_mask & EXPIRED_PREVIOUS_DECISION) != 0;
+
+  WebContents* web_contents = handler->web_contents();
+  int cert_error = handler->cert_error();
+  const net::SSLInfo& ssl_info = handler->ssl_info();
+  const GURL& request_url = handler->request_url();
+  ResourceType resource_type = handler->resource_type();
   GetContentClient()->browser()->AllowCertificateError(
-      handler->GetManager()->controller()->GetWebContents(),
-      handler->cert_error(), handler->ssl_info(), handler->request_url(),
-      handler->resource_type(), overridable, strict_enforcement,
-      expired_previous_decision,
-      base::Bind(&SSLPolicy::OnAllowCertificate, base::Unretained(this),
-                 make_scoped_refptr(handler)));
+      web_contents, cert_error, ssl_info, request_url, resource_type,
+      overridable, strict_enforcement, expired_previous_decision,
+      base::Bind(&OnAllowCertificate, base::Owned(handler.release()), this));
 }
 
 void SSLPolicy::InitializeEntryIfNeeded(NavigationEntryImpl* entry) {
   if (entry->GetSSL().security_style != SECURITY_STYLE_UNKNOWN)
     return;
 
   entry->GetSSL().security_style = GetSecurityStyleForResource(
       entry->GetURL(), entry->GetSSL().cert_id, entry->GetSSL().cert_status);
 }
 
 void SSLPolicy::OriginRanInsecureContent(const std::string& origin, int pid) {
   GURL parsed_origin(origin);
   if (parsed_origin.SchemeIsCryptographic())
     backend_->HostRanInsecureContent(parsed_origin.host(), pid);
 }
 
 }  // namespace content