Add unit test for notifying WebContents when SSLStatus changes due to HTTP-bad

content/browser/ssl/ssl_manager.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_manager.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/macros.h"
@@ skipping 94 matching lines @@
     }
     return;
   }
 
   SSLManager* manager =
       static_cast<NavigationControllerImpl*>(&web_contents->GetController())
           ->ssl_manager();
   manager->OnCertError(std::move(handler));
 }
 
+// Updates |entry|'s flags to account for the presence of insecure
+// content (mixed content or subresources with certificate errors).
+void UpdateEntryForInsecureContent(
+    NavigationEntryImpl* entry,
+    WebContentsImpl* web_contents_impl,
+    SSLHostStateDelegate* ssl_host_state_delegate) {
+  // Update the entry's flags for insecure content.
+  if (!web_contents_impl->DisplayedInsecureContent())
+    entry->GetSSL().content_status &= ~SSLStatus::DISPLAYED_INSECURE_CONTENT;
+  if (web_contents_impl->DisplayedInsecureContent())

[elawrence, 2016/10/12 15:22:47]

Can the value of this change from line 123? If not, could we test just once,
e.g.

if (web_contents_impl->DisplayedInsecureContent())
 entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT;
else 
 entry->GetSSL().content_status &= ~SSLStatus::DISPLAYED_INSECURE_CONTENT;

?

[estark, 2016/10/12 16:20:16]

Done.

+    entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT;
+  if (!web_contents_impl->DisplayedContentWithCertErrors()) {
+    entry->GetSSL().content_status &=
+        ~SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS;
+  }
+  if (web_contents_impl->DisplayedContentWithCertErrors()) {

[elawrence, 2016/10/12 15:22:46]

ditto

[estark, 2016/10/12 16:20:16]

Done.

+    entry->GetSSL().content_status |=
+        SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS;
+  }
+
+  SiteInstance* site_instance = entry->site_instance();
+  // Note that |site_instance| can be NULL here because NavigationEntries don't
+  // necessarily have site instances.  Without a process, the entry can't
+  // possibly have insecure content.  See bug http://crbug.com/12423.

[elawrence, 2016/10/12 15:22:46]

HTTPS for all URLs please. :)

Utterly trivial, but is there a reason that Googlers often use two spaces after
a sentence?

[estark, 2016/10/12 16:20:16]

Neither of those are my fault, I was just moving old code! :P Fixed both. I feel
like I see two spaces in a lot of older code, but ISTR some style guide or
chromium-dev thread encouraging one space (which is also my personal
preference).

+  if (site_instance && ssl_host_state_delegate &&

[elawrence, 2016/10/12 15:22:46]

Similar to earlier, is there a reason we shouldn't test once, e.g.

   if (site_instance && ssl_host_state_delegate) 
   {
      if (ssl_host_state_delegate->DidHostRunInsecureContent(Mixed))
content_status |= Ran_Insecure_content);

      if (ssl_host_state_delegate->DidHostRunInsecureContent(Errors))
content_status |= Ran_cert_errors);
   }

... potentially even pulling in the .host() and GetID() results into a local to
avoid calling multiple times?

[estark, 2016/10/12 16:20:16]

Done.

+      ssl_host_state_delegate->DidHostRunInsecureContent(
+          entry->GetURL().host(), site_instance->GetProcess()->GetID(),
+          SSLHostStateDelegate::MIXED_CONTENT)) {
+    entry->GetSSL().content_status |= SSLStatus::RAN_INSECURE_CONTENT;
+  }
+
+  if (site_instance && ssl_host_state_delegate &&
+      ssl_host_state_delegate->DidHostRunInsecureContent(
+          entry->GetURL().host(), site_instance->GetProcess()->GetID(),
+          SSLHostStateDelegate::CERT_ERRORS_CONTENT)) {
+    entry->GetSSL().content_status |= SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS;
+  }
+}
+
 }  // namespace
 
 // static
 void SSLManager::OnSSLCertificateError(
     const base::WeakPtr<SSLErrorHandler::Delegate>& delegate,
     const ResourceType resource_type,
     const GURL& url,
     const base::Callback<WebContents*(void)>& web_contents_getter,
     const net::SSLInfo& ssl_info,
     bool fatal) {
@@ skipping 232 matching lines @@
   if (web_contents_impl->DisplayedPasswordFieldOnHttp()) {
     entry->GetSSL().content_status |=
         SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP;
   }
 
   if (web_contents_impl->DisplayedCreditCardFieldOnHttp()) {
     entry->GetSSL().content_status |=
         SSLStatus::DISPLAYED_CREDIT_CARD_FIELD_ON_HTTP;
   }
 
-  // Do not record information about insecure subresources if the main
-  // page is HTTP or HTTPS without a certificate.
-  if (!entry->GetURL().SchemeIsCryptographic() || !entry->GetSSL().certificate)
-    return;
-
-  // Update the entry's flags for insecure content.
-  if (!web_contents_impl->DisplayedInsecureContent())
-    entry->GetSSL().content_status &= ~SSLStatus::DISPLAYED_INSECURE_CONTENT;
-  if (web_contents_impl->DisplayedInsecureContent())
-    entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT;
-  if (!web_contents_impl->DisplayedContentWithCertErrors()) {
-    entry->GetSSL().content_status &=
-        ~SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS;
-  }
-  if (web_contents_impl->DisplayedContentWithCertErrors()) {
-    entry->GetSSL().content_status |=
-        SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS;
+  // Only record information about insecure subresources if the main
+  // page is HTTPS with a certificate.
+  if (entry->GetURL().SchemeIsCryptographic() && entry->GetSSL().certificate) {
+    UpdateEntryForInsecureContent(entry, web_contents_impl,
+                                  ssl_host_state_delegate_);
   }
 
-  SiteInstance* site_instance = entry->site_instance();
-  // Note that |site_instance| can be NULL here because NavigationEntries don't
-  // necessarily have site instances.  Without a process, the entry can't
-  // possibly have insecure content.  See bug http://crbug.com/12423.
-  if (site_instance && ssl_host_state_delegate_ &&
-      ssl_host_state_delegate_->DidHostRunInsecureContent(
-          entry->GetURL().host(), site_instance->GetProcess()->GetID(),
-          SSLHostStateDelegate::MIXED_CONTENT)) {
-    entry->GetSSL().content_status |= SSLStatus::RAN_INSECURE_CONTENT;
+  if (!entry->GetSSL().Equals(original_ssl_status)) {

[elawrence, 2016/10/12 15:22:46]

Will this be reached much more often than it was previously? The very first
thing we do in this function is make a copy of the SSLStatus and then we toggle
a bit in the original. 

In the past, an early return meant that we wouldn't subsequently call
NotifyDidChangeVisibleState if the page wasn't HTTPS, but now we do.

Or is GetSSL().initialized always expected to be true upon entry to this
function, making line 384 redundant?

[estark, 2016/10/12 16:20:16]

Ohh, nice catch. I thought that in the common case it would already be
initialized during navigation
(https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resour...),
but that is only so for HTTPS.

I think we should clean this up -- I'm not sure if this |initialized| thing is
even necessary -- but I vote for doing it in a separate CL. The cost here is at
most one notification per navigation which isn't too big a deal.

[elawrence, 2016/10/12 16:28:58]

Acknowledged.

+    NotifyDidChangeVisibleSSLState();
   }
-
-  if (site_instance && ssl_host_state_delegate_ &&
-      ssl_host_state_delegate_->DidHostRunInsecureContent(
-          entry->GetURL().host(), site_instance->GetProcess()->GetID(),
-          SSLHostStateDelegate::CERT_ERRORS_CONTENT)) {
-    entry->GetSSL().content_status |= SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS;
-  }
-
-  if (!entry->GetSSL().Equals(original_ssl_status))
-    NotifyDidChangeVisibleSSLState();
 }
 
 void SSLManager::NotifyDidChangeVisibleSSLState() {
   WebContentsImpl* contents =
       static_cast<WebContentsImpl*>(controller_->delegate()->GetWebContents());
   contents->DidChangeVisibleSSLState();
 }
 
 }  // namespace content