Manage insecure content flags in SSLManager, not WebContentsImpl

content/browser/ssl/ssl_manager.cc

Index: content/browser/ssl/ssl_manager.cc
diff --git a/content/browser/ssl/ssl_manager.cc b/content/browser/ssl/ssl_manager.cc
index f2145870a2f0d1bfc5cefa6d2c8bf76257b96417..60fe39e6a9ff6212af9b7b07aaccd54b883c715d 100644
--- a/content/browser/ssl/ssl_manager.cc
+++ b/content/browser/ssl/ssl_manager.cc
@@ -22,7 +22,6 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/ssl_host_state_delegate.h"
-#include "content/public/browser/ssl_status.h"
 #include "net/url_request/url_request.h"
 
 namespace content {
@@ -151,17 +150,6 @@ void SSLManager::OnSSLCertificateSubresourceError(
                         ssl_info, fatal);
 }
 
-// static
-void SSLManager::NotifySSLInternalStateChanged(BrowserContext* context) {
-  SSLManagerSet* managers = static_cast<SSLManagerSet*>(
-      context->GetUserData(kSSLManagerKeyName));
-
-  for (std::set<SSLManager*>::iterator i = managers->get().begin();
-       i != managers->get().end(); ++i) {
-    (*i)->UpdateEntry((*i)->controller()->GetLastCommittedEntry());
-  }
-}
-
 SSLManager::SSLManager(NavigationControllerImpl* controller)
     : controller_(controller),
       ssl_host_state_delegate_(
@@ -185,13 +173,36 @@ SSLManager::~SSLManager() {
 
 void SSLManager::DidCommitProvisionalLoad(const LoadCommittedDetails& details) {
   NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
-  UpdateEntry(entry);
+  UpdateEntry(entry, SSLStatus::NORMAL_CONTENT);
   // Always notify the WebContents that the SSL state changed when a
   // load is committed, in case the active navigation entry has changed.
   NotifyDidChangeVisibleSSLState();
 }
 
-void SSLManager::DidRunInsecureContent(const GURL& security_origin) {
+void SSLManager::DidDisplayMixedContent() {
+  UpdateLastCommittedEntry(SSLStatus::DISPLAYED_INSECURE_CONTENT);
+}
+
+void SSLManager::DidDisplayContentWithCertErrors() {
+  NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
+  if (!entry)
+    return;
+  // Only record information about subresources with cert errors if the
+  // main page is HTTPS with a certificate.
+  if (entry->GetURL().SchemeIsCryptographic() && entry->GetSSL().certificate) {
+    UpdateLastCommittedEntry(SSLStatus::DISPLAYED_CONTENT_WITH_CERT_ERRORS);
+  }
+}
+
+void SSLManager::DidShowPasswordInputOnHttp() {
+  UpdateLastCommittedEntry(SSLStatus::DISPLAYED_PASSWORD_FIELD_ON_HTTP);
+}
+
+void SSLManager::DidShowCreditCardInputOnHttp() {
+  UpdateLastCommittedEntry(SSLStatus::DISPLAYED_CREDIT_CARD_FIELD_ON_HTTP);
+}
+
+void SSLManager::DidRunMixedContent(const GURL& security_origin) {
   NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
   if (!entry)
     return;
@@ -205,7 +216,7 @@ void SSLManager::DidRunInsecureContent(const GURL& security_origin) {
         security_origin.host(), site_instance->GetProcess()->GetID(),
         SSLHostStateDelegate::MIXED_CONTENT);
   }
-  UpdateEntry(entry);
+  UpdateEntry(entry, SSLStatus::NORMAL_CONTENT);
   NotifySSLInternalStateChanged(controller_->GetBrowserContext());
 }
 
@@ -223,7 +234,7 @@ void SSLManager::DidRunContentWithCertErrors(const GURL& security_origin) {
         security_origin.host(), site_instance->GetProcess()->GetID(),
         SSLHostStateDelegate::CERT_ERRORS_CONTENT);
   }
-  UpdateEntry(entry);
+  UpdateEntry(entry, SSLStatus::NORMAL_CONTENT);
   NotifySSLInternalStateChanged(controller_->GetBrowserContext());
 }
 
@@ -335,7 +346,9 @@ void SSLManager::OnCertErrorInternal(std::unique_ptr<SSLErrorHandler> handler,
                  ssl_host_state_delegate_));
 }
 
-void SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
+void SSLManager::UpdateEntry(
+    NavigationEntryImpl* entry,
+    SSLStatus::ContentStatusFlags additional_content_status_flags) {
   // We don't always have a navigation entry to update, for example in the
   // case of the Web Inspector.
   if (!entry)
@@ -343,45 +356,7 @@ void SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
 
   SSLStatus original_ssl_status = entry->GetSSL();  // Copy!
   entry->GetSSL().initialized = true;
-
-  WebContentsImpl* web_contents_impl =
-      static_cast<WebContentsImpl*>(controller_->delegate()->GetWebContents());
-
-  // For sensitive inputs (password, credit card) on HTTP, do not clear
-  // the |content_status| flag when the WebContents no longer has the
-  // flag set. This is different from how DISPLAYED_INSECURE_CONTENT and
-  // DISPLAYED_CONTENT_WITH_CERT_ERRORS are handled below. For sensitive
-  // inputs on HTTP, once the NavigationEntry has been marked as having
-  // displayed a sensitive input, it stays that way, even if the
-  // sensitive input is subsequently removed from the page.
-  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;

[estark, 2016/10/12 07:01:19]

You might notice here that we used to clear flags in some cases in UpdateEntry()
and we no longer ever clear flags.

AFAICT this logic existed to deal with the odd navigation flow that I described
in the CL description. The flow before went like this:
1. Navigation commits, UpdateEntry() is called and uses the WebContentsImpl
flags from the previous navigation.
2. Post-commit tasks run, WebContentsImpl clears its flags, UpdateEntry() is
called again and now has to clear the flags that it set in step #1 because they
are no longer set on the WebContentsImpl.

Now, when a navigation commits, UpdateEntry() is called and does *not* read
WebContentsImpl flags from the previous navigation, so there is no need for a
step #2 to clear them.

(Other than that odd navigation flow, these flags never need to get cleared once
they are set on an SSLStatus.)

-  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;
-  }
+  entry->GetSSL().content_status |= additional_content_status_flags;
 
   SiteInstance* site_instance = entry->site_instance();
   // Note that |site_instance| can be NULL here because NavigationEntries don't
@@ -394,7 +369,10 @@ void SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
     entry->GetSSL().content_status |= SSLStatus::RAN_INSECURE_CONTENT;
   }
 
-  if (site_instance && ssl_host_state_delegate_ &&
+  // Only record information about subresources with cert errors if the
+  // main page is HTTPS with a certificate.
+  if (entry->GetURL().SchemeIsCryptographic() && entry->GetSSL().certificate &&
+      site_instance && ssl_host_state_delegate_ &&
       ssl_host_state_delegate_->DidHostRunInsecureContent(
           entry->GetURL().host(), site_instance->GetProcess()->GetID(),
           SSLHostStateDelegate::CERT_ERRORS_CONTENT)) {
@@ -405,10 +383,30 @@ void SSLManager::UpdateEntry(NavigationEntryImpl* entry) {
     NotifyDidChangeVisibleSSLState();
 }
 
+void SSLManager::UpdateLastCommittedEntry(
+    SSLStatus::ContentStatusFlags additional_content_status_flags) {
+  NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
+  if (!entry)
+    return;
+  UpdateEntry(entry, additional_content_status_flags);
+}
+
 void SSLManager::NotifyDidChangeVisibleSSLState() {
   WebContentsImpl* contents =
       static_cast<WebContentsImpl*>(controller_->delegate()->GetWebContents());
   contents->DidChangeVisibleSSLState();
 }
 
+// static
+void SSLManager::NotifySSLInternalStateChanged(BrowserContext* context) {
+  SSLManagerSet* managers =
+      static_cast<SSLManagerSet*>(context->GetUserData(kSSLManagerKeyName));
+
+  for (std::set<SSLManager*>::iterator i = managers->get().begin();
+       i != managers->get().end(); ++i) {
+    (*i)->UpdateEntry((*i)->controller()->GetLastCommittedEntry(),
+                      SSLStatus::NORMAL_CONTENT);
+  }
+}
+
 }  // namespace content