Adapt MixedContentChecker for remote frames

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index 9d2a95b9c413b5f5eccd3305c357aa3390150b49..e1acd1a29d9b908264a4f82e1870c71a939406f7 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -554,6 +554,20 @@ class SitePerProcessHighDPIBrowserTest : public SitePerProcessBrowserTest {
   }
 };
 
+// SitePerProcessIgnoreCertErrorsBrowserTest
+
+class SitePerProcessIgnoreCertErrorsBrowserTest
+    : public SitePerProcessBrowserTest {
+ public:
+  SitePerProcessIgnoreCertErrorsBrowserTest() {}
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    SitePerProcessBrowserTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kIgnoreCertificateErrors);
+  }
+};
+
 // Ensure that navigating subframes in --site-per-process mode works and the
 // correct documents are committed.
 IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, CrossSiteIframe) {
@@ -4661,4 +4675,38 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, ParentDetachRemoteChild) {
   EXPECT_TRUE(watcher.did_exit_normally());
 }
 
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, PassiveMixedContent) {

[alexmos, 2016/01/16 00:49:50]

Hmm, it doesn't look like this test exercises any OOPIF functionality, though it
does seem useful to have.  I'd suggest moving it to something like
web_contents_impl_browsertest.cc, except that it'd be nice to keep all of these
tests together.  Actually, do you think the other tests would be useful to run
without --site-per-process as well?  If so, something like
web_contents_impl_browsertest or render_frame_host_manager_browsertest (or even
a new test file) might be a good home for them.  These would run without
--site-per-process normally, and with --site-per-process on our FYI and try bots
(which should be enabled the CQ some time soon).

[estark, 2016/01/20 05:56:22]

Hmm... actually, maybe we don't need this test at all, since as you say, it
doesn't really test OOPIFs, and mixed content in general is covered pretty well
by layout tests. So maybe I should just remove this one? For the rest of the
tests I added here, it certainly wouldn't hurt to run them without
--site-per-process, but I do think we already have non-site-per-process test
coverage (in the form of layout tests) for this same functionality. So, maybe
they should stay here and only run for --site-per-process? What do you think?

[alexmos, 2016/01/20 22:43:55]

That sounds good to me.  Let's remove this one and keep the others in this file.

[estark, 2016/01/21 04:40:13]

Done.

+  net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_server.ServeFilesFromSourceDirectory("content/test/data");
+  ASSERT_TRUE(https_server.Start());
+
+  GURL url(https_server.GetURL("/mixed-content/basic-passive.html"));
+  NavigateToURL(shell(), url);
+  EXPECT_TRUE(shell()->web_contents()->DisplayedInsecureContent());
+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessIgnoreCertErrorsBrowserTest,

[alexmos, 2016/01/16 00:49:50]

A comment about why these tests need to ignore cert errors would be nice.

[estark, 2016/01/20 05:56:22]

Done.

+                       PassiveMixedContentInIframe) {

[alexmos, 2016/01/20 22:43:55]

Do you plan to add similar tests for active mixed content?  Seems like that
might be worth having.

[estark, 2016/01/21 04:40:13]

I should have done that earlier, sorry. I just added a couple, but the only way
I could figure out to do them is to put them in
chrome_site_per_process_browsertest.cc so that I could use the
|kAllowRunningInsecureContent| switch in chrome/. It's a bummer to have the
passive mixed content ones here and the active somewhere else... What do you
think of that? Do you know of any way to set a webkit setting from a content
browser test?

[alexmos, 2016/01/21 18:13:28]

What do you think about flipping things to instead test that the active mixed
content gets blocked correctly by default?  That would better cover the bug we
originally had with failing open in 486936, and if you use an <iframe> instead
of <script>, you could just check whether an additional FrameTreeNode was
created or not.  This won't require specifying kAllowRunningInsecureContent so
you could do it from content.

For covering the kAllowRunningInsecureContent path, it seems we may already some
tests (e.g., ChromeSecurityStateModelClientTest.MixedContent), though I didn't
check if they end up using OOPIFs for their frames when run with
--site-per-process.

[alexmos, 2016/01/21 18:21:51]

Also, if you did want to override that setting from content/, maybe you could do
something like this?

  WebPreferences prefs =
      GetWebContents()->GetRenderViewHost()->GetWebkitPreferences();
  prefs.allow_running_insecure_content = true;
  GetWebContents()->GetRenderViewHost()->UpdateWebkitPreferences(prefs);

No idea whether that'd work, but BrowserPluginGuest::InitInternal appears to use
something like this
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...)

+  net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_server.ServeFilesFromSourceDirectory("content/test/data");
+  ASSERT_TRUE(https_server.Start());
+
+  GURL iframe_url(
+      https_server.GetURL("/mixed-content/basic-passive-in-iframe.html"));
+  NavigateToURL(shell(), iframe_url);
+  EXPECT_TRUE(shell()->web_contents()->DisplayedInsecureContent());

[alexmos, 2016/01/16 00:49:50]

Does navigating the subframe to another (secure) cross-site URL clear
DisplayedInsecureContent?  What about navigating the main frame?  This may be
worth checking in the test.

[estark, 2016/01/20 05:56:22]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(SitePerProcessIgnoreCertErrorsBrowserTest,
+                       PassiveMixedContentInIframeWithStrictBlocking) {
+  net::EmbeddedTestServer https_server(net::EmbeddedTestServer::TYPE_HTTPS);
+  https_server.ServeFilesFromSourceDirectory("content/test/data");
+  ASSERT_TRUE(https_server.Start());
+
+  GURL iframe_url_with_strict_blocking(https_server.GetURL(
+      "/mixed-content/basic-passive-in-iframe-with-strict-blocking.html"));
+  NavigateToURL(shell(), iframe_url_with_strict_blocking);
+  EXPECT_FALSE(shell()->web_contents()->DisplayedInsecureContent());

[alexmos, 2016/01/16 00:49:50]

Maybe also check the strict mixed content flags stored on the FrameTreeNodes in
current_replication_state()?

[estark, 2016/01/20 05:56:22]

Done.

+}
+
 }  // namespace content