Teach SSLHostStateDelegate about subresources with cert errors

content/browser/ssl/ssl_policy_backend.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_backend.h"
 
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/ssl_host_state_delegate.h"
 
 namespace content {
 
 SSLPolicyBackend::SSLPolicyBackend(NavigationControllerImpl* controller)
     : ssl_host_state_delegate_(
           controller->GetBrowserContext()->GetSSLHostStateDelegate()),
       controller_(controller) {
   DCHECK(controller_);
 }
 
 void SSLPolicyBackend::HostRanInsecureContent(const std::string& host, int id) {
   if (ssl_host_state_delegate_)
-    ssl_host_state_delegate_->HostRanInsecureContent(host, id);
+    ssl_host_state_delegate_->HostRanInsecureContent(
+        host, id, SSLHostStateDelegate::MIXED_CONTENT);

[jww, 2016/08/11 18:50:15]

Okay, this has me very confused. I think maybe that you're planning a later
patch or something, but can you explain to me why this only has the possibility
of marking mixed content, and not cert errors?

If this is just to simulate current behavior, then that's fine, but please add a
comment somewhere to clarify it, and add a TODO saying that you're going to fix
it.

If this code is only reachable by mixed content (although I don't think that's
the case), that's also fine, but then I'd like to see the methods renamed to
reflect that, since otherwise it gives the impression that this magically
handles all cases.

[jam, 2016/08/11 19:53:12]

jww: since i got the next patch to review in this series,
https://codereview.chromium.org/2226363002/diff/1/content/browser/ssl/ssl_pol...
does what you suspect of adding new methods which when it lands will make this
easy to read i think

[estark, 2016/08/11 20:58:38]

Urgh, yeah, I tried to split up a giant CL into smallers CLs to make it easier
to review and might have gone too far to the other side of the line. :) Sorry
for the confusion, Joel. To explain a little more, this CL just gives
SSLHostStateDelegate the ability to track subresources with cert errors, but
doesn't actually tell SSLHostStateDelegate when a site has subresources with
cert errors.

The follow-up CL that jam linked to hooks everything up, so that when the
renderer tells the browser that it loaded a subresource with cert errors,
WebContents tells SSLManager who tells SSLPolicy who tells SSLPolicyBackend who
tells SSLHostStateDelegate that there's CONTENT_WITH_CERT_ERRORS.

Does that make any more sense? I'll add a TODO in SSLHostStateDelegate to
explain this as well. (The TODO will be done in the follow-up CL, but I'll add
it in case any people from the future are looking at this commit in isolation
and become similarly confused.)

(Also, next on my list of content/browser/ssl clean-up is to see if we can
collapse SSLManager, SSLPolicy, and SSLPolicyBackend, since AFAICT there's no
real reason for them to be split up and they all just kind of shuttle method
calls from one to the next.)

Finally, you raise a good point that these SSLManager/SSLPolicy/SSLPolicyBackend
methods should be renamed. Once this CL and the follow-up CL land,
HostRanInsecureContent() will handle only mixed content, not content with cert
errors, so it should be renamed to HostRanMixedContent(), and similarly for
DidHostRunInsecureContent(). I will file a bug for that but would like to do it
in a follow-up CL, if it's okay with you, since there's already so many moving
parts here.

[jww, 2016/08/11 21:13:24]

Great, thanks for the clarifications!

[jam, 2016/08/11 21:14:04]

That would be really great for readability. I've been confused about the
differences between them.

   SSLManager::NotifySSLInternalStateChanged(controller_->GetBrowserContext());
 }
 
 bool SSLPolicyBackend::DidHostRunInsecureContent(const std::string& host,
                                                  int pid) const {
   if (!ssl_host_state_delegate_)
     return false;
 
-  return ssl_host_state_delegate_->DidHostRunInsecureContent(host, pid);
+  return ssl_host_state_delegate_->DidHostRunInsecureContent(

[jww, 2016/08/11 18:50:15]

This is especially confusing. I would expect this to check
DidHostRunInsecureContent for *both* MIXED_CONTENT and CERT_ERRORS_CONTENT.

[estark, 2016/08/11 20:58:38]

Same explanation as above, I think. In the follow-up CL, there's a separate
SSLPolicyBackend::DidHostRunContentWithCertErrors() method that passes
SSLHostStateDelegate::CONTENT_WITH_CERT_ERRORS.

+      host, pid, SSLHostStateDelegate::MIXED_CONTENT);
 }
 
 void SSLPolicyBackend::RevokeUserAllowExceptions(const std::string& host) {
   if (!ssl_host_state_delegate_)
     return;
 
   ssl_host_state_delegate_->RevokeUserAllowExceptions(host);
 }
 
 bool SSLPolicyBackend::HasAllowException(const std::string& host) {
@@ skipping 15 matching lines @@
     const std::string& host,
     net::CertStatus error,
     bool* expired_previous_decision) {
   return ssl_host_state_delegate_ ?
       ssl_host_state_delegate_->QueryPolicy(
           host, cert, error, expired_previous_decision) :
       SSLHostStateDelegate::DENIED;
 }
 
 }  // namespace content