PlzNavigate: Move navigation-level mixed content checks to the browser.

chrome/browser/content_settings/mixed_content_settings.cc

Index: chrome/browser/content_settings/mixed_content_settings.cc
diff --git a/chrome/browser/content_settings/mixed_content_settings.cc b/chrome/browser/content_settings/mixed_content_settings.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7d46ac344be9e019e217c3ec30d9aa199391806a
--- /dev/null
+++ b/chrome/browser/content_settings/mixed_content_settings.cc
@@ -0,0 +1,59 @@
+// Copyright (c) 2016 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 "chrome/browser/content_settings/mixed_content_settings.h"
+
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/site_instance.h"
+
+using content::BrowserThread;
+using content::WebContents;
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(MixedContentSettings);
+
+MixedContentSettings::MixedContentSettings(WebContents* tab)
+    : content::WebContentsObserver(tab),
+      insecure_content_site_instance_(nullptr),
+      insecure_content_allowed_running_(false) {
+  if (tab->HasOpener()) {
+    // Note: using the opener WebContents to override these settings only works
+    // because Chrome controls them at the tab level instead of at the frame
+    // level as Blink does.
+    MixedContentSettings* opener_settings =
+        MixedContentSettings::FromWebContents(tab->GetOpener());
+    if (opener_settings) {
+      insecure_content_site_instance_ =
+          opener_settings->insecure_content_site_instance_;
+      insecure_content_allowed_running_ =
+          opener_settings->insecure_content_allowed_running_;
+    }
+  }
+}
+
+MixedContentSettings::~MixedContentSettings() {}
+
+void MixedContentSettings::AllowRunningOfInsecureContent() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!insecure_content_site_instance_ ||
+         insecure_content_site_instance_ == web_contents()->GetSiteInstance());
+  insecure_content_site_instance_ = web_contents()->GetSiteInstance();
+  insecure_content_allowed_running_ = true;
+}
+
+void MixedContentSettings::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())
+    return;
+
+  // Resets mixed-content settings on a successful navigation of the main frame
+  // to a new site instance.

[jam, 2017/01/11 16:38:06]

I don't understand why site instance is involved here. i.e. why aren't we just
resetting this value on every navigation to a different page (i.e. adding ||
!IsSamePage() to above check). The reason I say this is that presumably a user
bubble action is just for the page, and its effects go away when going to a
different page?

[carlosk, 2017/01/11 22:15:02]

That's not how the mixed content settings are reset on the renderer. I somewhat
described this in a section [1] in my initial design doc for this change (Google
internal). There's a link to the code that does this but in short these settings
are reset on the renderer only when the main RenderFrame is replaced.

So it seems to me that a reasonable translation to browser/browser is that cross
site navigations cause the reset hence the monitoring of SiteInstance changes.

You can test it yourself with test sites for mixed content, like for instance
[2]. Navigate to that site, allow mixed content scripts to run and the do this:

* Case 1: Click the "Test" button by the password field, which will cause a same
site navigation, then hit back. Active mixed content is still allowed.
* Case 2: Navigate to wikipedia.org from the omnibar and then hit back. Active
mixed content is not allowed anymore.

I did ask about this many times while working on this and never got a clear
answer. I don't think anyone has a definitive understanding or opinion on how
this is supposed to work.

[1]
https://docs.google.com/document/d/1bNZG5ZN9flm1_-03sShI4466heqpBTVSnjc-HJtvw...

[2] https://www.bennish.net/mixed-content.html

[jam, 2017/01/12 01:39:00]

Ah, thanks for the explanation. I had assumed it works like other bubbles but
now I see it's different.

ok your change makes sense then. Since this is tricky, please add a comment to
say this matches the renderer behavior which doesn't reset it on navigation

[carlosk, 2017/01/12 17:44:02]

That was indeed missing. I added a detailed explanation here that will be in my
next patch set.

+  content::SiteInstance* new_site =
+      navigation_handle->GetRenderFrameHost()->GetSiteInstance();
+  if (new_site != insecure_content_site_instance_) {
+    insecure_content_site_instance_ = nullptr;
+    insecure_content_allowed_running_ = false;
+  }
+}