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

chrome/browser/content_settings/mixed_content_settings.cc

+// 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),

[nasko, 2017/01/12 18:32:37]

nit: You could initialize these in the header file these days with the new C++
features allowed.

[carlosk, 2017/01/21 02:54:58]

Done.

+      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.

[nasko, 2017/01/12 18:32:37]

Hmm, I think we now track openers at the frame level even browser side. Do we
want to have frame granularity here? If not, then the comment might be more
misleading than helpful the way it is now.

[carlosk, 2017/01/21 02:54:58]

We want to keep frame granularity at tab-level as it is today.

I tried clarifying the comment. WDYT?

[nasko, 2017/02/10 01:08:20]

Acknowledged.

+    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())

[nasko, 2017/01/12 18:32:37]

What happens if the commit was for an error page?

[carlosk, 2017/01/21 02:54:58]

In PlzNavigate, the RenderFrame that would render the error page would be the
same as if the navigation had succeeded: either the current one if it was a
same-site navigation or a new one if it was cross-site. I confirmed this with
clamy@. So letting this execute in the error page case should be the right thing
to do.

[nasko, 2017/01/23 22:32:37]

Do we have a test that ensures that is the case and we don't regress it?

[carlosk, 2017/02/08 02:59:02]

To clarify: this specific check will control if mixed contents user controlled
settings are reset or not in a navigation to an error page. Two points here:
- I don't think the expected behavior is specified anywhere.
- I don't think this is a very important case and that either behavior is
better.

There's no existing test that checks this. I did add this as an item to my list
of "tests to be added later".

[nasko, 2017/02/10 01:08:20]

Ok. Where would one find that list?

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

It's currently in a task in my Asana task tracker. I will move it out of there
and into a doc soon(TM).

+    return;
+
+  // Resets mixed-content settings on a successful navigation of the main frame
+  // to a new site instance.

[nasko, 2017/01/12 18:32:37]

nit: s/site instance/SiteInstance/

[carlosk, 2017/01/21 02:54:58]

Done. This comment also grew considerably to explain why the settings are reset
on cross-site navigations.

[nasko, 2017/01/23 22:32:37]

Thanks! That makes it a lot more clear why.

+  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;
+  }
+}