Don't create unexpected OOPIFs in --isolate-extensions.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 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 "base/bind.h"
+#include "base/command_line.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/frame_navigation_entry.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/shell/common/shell_switches.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/url_request/url_request_failed_job.h"
 
 namespace content {
 
 class NavigationControllerBrowserTest : public ContentBrowserTest {
  protected:
   void SetUpOnMainThread() override {
@@ skipping 2634 matching lines @@
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
 
   subframe_entry = controller.GetLastCommittedEntry()->GetFrameEntry(subframe);
   int64 isn_4 = subframe_entry->item_sequence_number();
   int64 dsn_4 = subframe_entry->document_sequence_number();
   EXPECT_NE(-1, isn_4);
   EXPECT_NE(isn_3, isn_4);
   EXPECT_EQ(dsn_3, dsn_4);
 }
 
+// Support a set of tests that isolate only a subset of sites with
+// out-of-process iframes (OOPIFs).
+class NavigationControllerOopifBrowserTest
+    : public NavigationControllerBrowserTest {
+ public:
+  NavigationControllerOopifBrowserTest() {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // Enable the OOPIF framework but only isolate sites from a single TLD.
+    command_line->AppendSwitchASCII(switches::kIsolateSitesForTesting, "*.is");
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NavigationControllerOopifBrowserTest);
+};
+
+// Verify that restoring a NavigationEntry with cross-site subframes does not
+// create out-of-process iframes unless the current SiteIsolationPolicy says to.
+IN_PROC_BROWSER_TEST_F(NavigationControllerOopifBrowserTest,
+                       RestoreWithoutExtraOopifs) {
+  // 1. Start on a page with a data URL iframe.
+  GURL main_url_a(embedded_test_server()->GetURL(
+      "a.com", "/navigation_controller/page_with_data_iframe.html"));
+  GURL data_url("data:text/html,Subframe");
+  EXPECT_TRUE(NavigateToURL(shell(), main_url_a));
+  const NavigationControllerImpl& controller =
+      static_cast<const NavigationControllerImpl&>(
+          shell()->web_contents()->GetController());
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  EXPECT_EQ(main_url_a, root->current_url());
+  EXPECT_EQ(data_url, root->child_at(0)->current_url());
+
+  // 2. Navigate the iframe cross-site.
+  GURL frame_url_b(embedded_test_server()->GetURL(
+      "b.com", "/navigation_controller/simple_page_1.html"));
+  {
+    TestNavigationObserver observer(shell()->web_contents());

[ncarter (slow), 2015/12/08 20:14:13]

This TNO (and its enclosing scope) might be unnecessary.

[Charlie Reis, 2015/12/08 20:34:11]

Done.

+    NavigateFrameToURL(root->child_at(0), frame_url_b);
+    observer.Wait();
+  }
+  EXPECT_EQ(main_url_a, root->current_url());
+  EXPECT_EQ(frame_url_b, root->child_at(0)->current_url());
+
+  EXPECT_EQ(2, controller.GetEntryCount());
+  EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
+  NavigationEntryImpl* entry2 = controller.GetLastCommittedEntry();
+
+  // 3. Create a NavigationEntry with the same PageState as |entry2|.
+  scoped_ptr<NavigationEntryImpl> restored_entry =
+      NavigationEntryImpl::FromNavigationEntry(
+          NavigationControllerImpl::CreateNavigationEntry(
+              main_url_a, Referrer(), ui::PAGE_TRANSITION_RELOAD, false,
+              std::string(), controller.GetBrowserContext()));
+  restored_entry->SetPageID(0);
+  EXPECT_EQ(0U, restored_entry->root_node()->children.size());
+  restored_entry->SetPageState(entry2->GetPageState());
+
+  // Verify subframe entries if they're enabled (e.g. in --site-per-process).
+  if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {

[ncarter (slow), 2015/12/08 20:14:13]

I think you can EXPECT_TRUE(SiteIsolationPolicy::UseSubframeNavigationEntries())
at the top of the test, and drop the else {} branch here.

[Charlie Reis, 2015/12/08 20:34:11]

Done, with AreCrossProcessFramesPossible.

+    // The entry should have no SiteInstance in the FrameNavigationEntry for the
+    // b.com subframe.
+    EXPECT_FALSE(
+        restored_entry->root_node()->children[0]->frame_entry->site_instance());
+  } else {
+    // There are no subframe FrameNavigationEntries by default.
+    EXPECT_EQ(0U, restored_entry->root_node()->children.size());
+  }
+
+  // 4. Restore the new entry in a new tab and verify the correct URLs load.
+  std::vector<scoped_ptr<NavigationEntry>> entries;
+  entries.push_back(restored_entry.Pass());
+  Shell* new_shell = Shell::CreateNewWindow(
+      controller.GetBrowserContext(), GURL::EmptyGURL(), nullptr, gfx::Size());
+  FrameTreeNode* new_root =
+      static_cast<WebContentsImpl*>(new_shell->web_contents())
+          ->GetFrameTree()
+          ->root();
+  NavigationControllerImpl& new_controller =
+      static_cast<NavigationControllerImpl&>(
+          new_shell->web_contents()->GetController());
+  new_controller.Restore(
+      entries.size() - 1,
+      NavigationController::RESTORE_LAST_SESSION_EXITED_CLEANLY, &entries);
+  ASSERT_EQ(0u, entries.size());
+  {
+    TestNavigationObserver restore_observer(new_shell->web_contents());
+    new_controller.LoadIfNecessary();
+    restore_observer.Wait();
+  }
+  ASSERT_EQ(1U, new_root->child_count());
+  EXPECT_EQ(main_url_a, new_root->current_url());
+  EXPECT_EQ(frame_url_b, new_root->child_at(0)->current_url());
+
+  // The subframe should only be in a different SiteInstance if OOPIFs are
+  // required for all sites.
+  if (SiteIsolationPolicy::UseDedicatedProcessesForAllSites()) {

[ncarter (slow), 2015/12/08 20:14:13]

AreAllSitesIsolatedForTesting() would be a better call here, though the
difference between the two doesn't currently exist (I expect it will reappear)

[Charlie Reis, 2015/12/08 20:34:11]

Done.

+    EXPECT_NE(new_root->current_frame_host()->GetSiteInstance(),
+              new_root->child_at(0)->current_frame_host()->GetSiteInstance());
+  } else {
+    EXPECT_EQ(new_root->current_frame_host()->GetSiteInstance(),
+              new_root->child_at(0)->current_frame_host()->GetSiteInstance());
+  }
+}
+
 namespace {
 
 // Loads |start_url|, then loads |stalled_url| which stalls. While the page is
 // stalled, an in-page navigation happens. Make sure that all the navigations
 // are properly classified.
 void DoReplaceStateWhilePending(Shell* shell,
                                 const GURL& start_url,
                                 const GURL& stalled_url,
                                 const std::string& replace_state_filename) {
   NavigationControllerImpl& controller =
@@ skipping 388 matching lines @@
     EXPECT_EQ(original_url, capturer.all_params()[1].url);
     EXPECT_EQ(original_url, shell()->web_contents()->GetLastCommittedURL());
   }
 
   // Make sure the renderer is still alive.
   EXPECT_TRUE(
       ExecuteScript(shell()->web_contents(), "console.log('Success');"));
 }
 
 }  // namespace content