Clear SSLStatus content status flags on navigation

As of https://codereview.chromium.org/2926803002, when navigating to an existing
entry, the new navigation's SSLStatus remains unchanged from the existing
entry. This is incorrect for content status flags, which depend on the content
of the page; navigating to an existing entry does not mean that the content on
the page is the same as it was when we navigated away from the existing
entry. For this reason, this CL clears content status flags when a navigation
commits. Any content status flags that do apply to the new navigation should be
re-added as the content on the page loads.

Bug: 750649
Change-Id: I41441c90ddeb85b6cbf35e4b102ce322cffb7db9
Reviewed-on: https://chromium-review.googlesource.com/597488Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491504}

chrome/browser/ssl/ssl_browser_tests.cc

@@ -87,6 +87,7 @@
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
@@ -897,6 +898,112 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, TestBrokenHTTPSWithActiveInsecureContent) {
                                  AuthState::RAN_INSECURE_CONTENT);
 }
 
+namespace {
+
+// A WebContentsObserver that allows the user to wait for a
+// DidChangeVisibleSecurityState event.
+class SecurityStateWebContentsObserver : public content::WebContentsObserver {
+ public:
+  explicit SecurityStateWebContentsObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents) {}
+  ~SecurityStateWebContentsObserver() override {}
+
+  void WaitForDidChangeVisibleSecurityState() { run_loop_.Run(); }
+
+  // WebContentsObserver:
+  void DidChangeVisibleSecurityState() override { run_loop_.Quit(); }
+
+ private:
+  base::RunLoop run_loop_;
+};
+
+// A WebContentsObserver that allows the user to wait for a same-page
+// navigation. Tests using this observer will fail if a non-same-page navigation
+// completes after calling WaitForSamePageNavigation.
+class SamePageNavigationObserver : public content::WebContentsObserver {
+ public:
+  explicit SamePageNavigationObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents) {}
+  ~SamePageNavigationObserver() override {}
+
+  void WaitForSamePageNavigation() { run_loop_.Run(); }
+
+  // WebContentsObserver:
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    ASSERT_TRUE(navigation_handle->IsSameDocument());
+    run_loop_.Quit();
+  }
+
+ private:
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
+// Tests that the mixed content flags are reset when going back to an existing
+// navigation entry that had mixed content. Regression test for
+// https://crbug.com/750649.
+IN_PROC_BROWSER_TEST_F(SSLUITest, GoBackToMixedContent) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ASSERT_TRUE(https_server_.Start());
+
+  // Navigate to a URL and dynamically load mixed content.
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_.GetURL("/ssl/google.html"));
+  CheckAuthenticatedState(tab, AuthState::NONE);
+  SecurityStateWebContentsObserver observer(tab);
+  ASSERT_TRUE(content::ExecuteScript(tab,
+                                     "var i = document.createElement('img');"
+                                     "i.src = 'http://example.test';"
+                                     "document.body.appendChild(i);"));
+  observer.WaitForDidChangeVisibleSecurityState();
+  CheckSecurityState(tab, CertError::NONE, security_state::NONE,
+                     AuthState::DISPLAYED_INSECURE_CONTENT);
+
+  // Now navigate somewhere else, and then back to the page that dynamically
+  // loaded mixed content.
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/ssl/google.html"));
+  CheckUnauthenticatedState(
+      browser()->tab_strip_model()->GetActiveWebContents(), AuthState::NONE);
+  chrome::GoBack(browser(), WindowOpenDisposition::CURRENT_TAB);
+  content::WaitForLoadStop(tab);
+  // After going back, the mixed content indicator should no longer be present.
+  CheckAuthenticatedState(tab, AuthState::NONE);
+}
+
+// Tests that the mixed content flags are not reset for an in-page navigation.
+IN_PROC_BROWSER_TEST_F(SSLUITest, MixedContentWithSamePageNavigation) {
+  ASSERT_TRUE(https_server_.Start());
+
+  // Navigate to a URL and dynamically load mixed content.
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_.GetURL("/ssl/google.html"));
+  CheckAuthenticatedState(tab, AuthState::NONE);
+  SecurityStateWebContentsObserver security_state_observer(tab);
+  ASSERT_TRUE(content::ExecuteScript(tab,
+                                     "var i = document.createElement('img');"
+                                     "i.src = 'http://example.test';"
+                                     "document.body.appendChild(i);"));
+  security_state_observer.WaitForDidChangeVisibleSecurityState();
+  CheckSecurityState(tab, CertError::NONE, security_state::NONE,
+                     AuthState::DISPLAYED_INSECURE_CONTENT);
+
+  // Initiate a same-page navigation and check that the page is still marked as
+  // having displayed mixed content.
+  SamePageNavigationObserver navigation_observer(tab);
+  ui_test_utils::NavigateToURL(browser(),
+                               https_server_.GetURL("/ssl/google.html#foo"));
+  navigation_observer.WaitForSamePageNavigation();
+  CheckSecurityState(tab, CertError::NONE, security_state::NONE,
+                     AuthState::DISPLAYED_INSECURE_CONTENT);
+}
+
 // Tests that the WebContents's flag for displaying content with cert
 // errors get cleared upon navigation.
 IN_PROC_BROWSER_TEST_F(SSLUITest,

content/browser/ssl/ssl_manager.cc

@@ -191,7 +191,8 @@ SSLManager::~SSLManager() {
 
 void SSLManager::DidCommitProvisionalLoad(const LoadCommittedDetails& details) {
   NavigationEntryImpl* entry = controller_->GetLastCommittedEntry();
-  int content_status_flags = 0;
+  int add_content_status_flags = 0;
+  int remove_content_status_flags = 0;
   if (!details.is_main_frame) {
     // If it wasn't a main-frame navigation, then carry over content
     // status flags. (For example, the mixed content flag shouldn't
@@ -199,10 +200,16 @@ void SSLManager::DidCommitProvisionalLoad(const LoadCommittedDetails& details) {
     NavigationEntryImpl* previous_entry =
         controller_->GetEntryAtIndex(details.previous_entry_index);
     if (previous_entry) {
-      content_status_flags = previous_entry->GetSSL().content_status;
+      add_content_status_flags = previous_entry->GetSSL().content_status;
     }
+  } else if (!details.is_same_document) {
+    // For main-frame non-same-page navigations, clear content status
+    // flags. These flags are set based on the content on the page, and thus
+    // should reflect the current content, even if the navigation was to an
+    // existing entry that already had content status flags set.
+    remove_content_status_flags = ~0;
   }
-  UpdateEntry(entry, content_status_flags, 0);
+  UpdateEntry(entry, add_content_status_flags, remove_content_status_flags);
   // Always notify the WebContents that the SSL state changed when a
   // load is committed, in case the active navigation entry has changed.
   NotifyDidChangeVisibleSSLState();
@@ -411,8 +418,8 @@ void SSLManager::UpdateEntry(NavigationEntryImpl* entry,
 
   SSLStatus original_ssl_status = entry->GetSSL();  // Copy!
   entry->GetSSL().initialized = true;
-  entry->GetSSL().content_status |= add_content_status_flags;
   entry->GetSSL().content_status &= ~remove_content_status_flags;
+  entry->GetSSL().content_status |= add_content_status_flags;
 
   SiteInstance* site_instance = entry->site_instance();
   // Note that |site_instance| can be NULL here because NavigationEntries don't

content/browser/ssl/ssl_manager.h

@@ -113,13 +113,14 @@ class CONTENT_EXPORT SSLManager {
   void OnCertErrorInternal(std::unique_ptr<SSLErrorHandler> handler,
                            int options_mask);
 
-  // Updates the NavigationEntry's |content_status| flags according to
-  // state in |ssl_host_state_delegate|. |add_content_status_flags| and
-  // |remove_content_status_flags| are bitmasks of
-  // SSLStatus::ContentStatusFlags that will be added or removed from
-  // the |content_status| field. (Pass 0 to add/remove no content status
-  // flags.) This method will notify the WebContents of an SSL state
-  // change if a change was actually made.
+  // Updates the NavigationEntry's |content_status| flags according to state in
+  // |ssl_host_state_delegate|. |add_content_status_flags| and
+  // |remove_content_status_flags| are bitmasks of SSLStatus::ContentStatusFlags
+  // that will be added or removed from the |content_status| field. (Pass 0 to
+  // add/remove no content status flags.) |remove_content_status_flags| are
+  // removed before |add_content_status_flags| are added. This method will
+  // notify the WebContents of an SSL state change if a change was actually
+  // made.
   void UpdateEntry(NavigationEntryImpl* entry,
                    int add_content_status_flags,
                    int remove_content_status_flags);