Disable top-document-isolation if the parent SiteInstance is a hosted app.

chrome/browser/ui/extensions/hosted_app_browsertest.cc

Index: chrome/browser/ui/extensions/hosted_app_browsertest.cc
diff --git a/chrome/browser/ui/extensions/hosted_app_browsertest.cc b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
index 80b59a1c740362ac593876a564f83739a9203893..ff1f39d815cd4218953be6c3256ea97538055d5e 100644
--- a/chrome/browser/ui/extensions/hosted_app_browsertest.cc
+++ b/chrome/browser/ui/extensions/hosted_app_browsertest.cc
@@ -6,10 +6,12 @@
 
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
+#include "base/strings/stringprintf.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
@@ -17,11 +19,17 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 
 #if defined(OS_MACOSX)
 #include "chrome/common/chrome_features.h"
@@ -60,9 +68,12 @@ class HostedAppTest : public ExtensionBrowserTest {
 
  protected:
   void SetupApp(const std::string& app_folder, bool is_bookmark_app) {
+    SetupApp(test_data_dir_.AppendASCII(app_folder), is_bookmark_app);
+  }
+
+  void SetupApp(const base::FilePath& app_folder, bool is_bookmark_app) {
     const Extension* app = InstallExtensionWithSourceAndFlags(
-        test_data_dir_.AppendASCII(app_folder), 1,
-        extensions::Manifest::INTERNAL,
+        app_folder, 1, extensions::Manifest::INTERNAL,
         is_bookmark_app ? extensions::Extension::FROM_BOOKMARK
                         : extensions::Extension::NO_FLAGS);
     ASSERT_TRUE(app);
@@ -186,3 +197,153 @@ IN_PROC_BROWSER_TEST_F(HostedAppTest,
   NavigateAndCheckForLocationBar(
       app_browser_, "http://www.foo.com/blah", true);
 }
+
+class HostedAppVsTdiTest : public HostedAppTest {
+ public:
+  HostedAppVsTdiTest() {}
+  ~HostedAppVsTdiTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    HostedAppTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kTopDocumentIsolation);
+  }
+
+  void SetUpOnMainThread() override {
+    HostedAppTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+};
+
+// Tests that even with --top-document-isolation, app.site.com (covered by app's
+// web extent) main frame and foo.site.com (not covered by app's web extent)
+// share the same renderer process.  See also https://crbug.com/679011.
+//
+// Relevant frames in the test:
+// - |app| - app.site.com/frame_tree/cross_origin_but_same_site_frames.html
+//           Main frame, launch URL of the hosted app (i.e. app.launch.web_url).
+// - |same_dir| - app.site.com/frame_tree/simple.htm
+//                Another URL, but still covered by hosted app's web extent
+//                (i.e. by app.urls).
+// - |diff_dir| - app.site.com/save_page/a.htm
+//                Same origin as |same_dir| and |app|, but not covered by app's
+//                extent.
+// - |same_site| - other.site.com/title1.htm
+//                 Different domain, but same site as |app|, |same_dir|,
+//                 |diff_dir|.
+// - |cross_site| - cross.domain.com/title1.htm
+//                  Cross-site from all the other frames.
+//
+// Verifications of |*_site| (e.g. EXPECT_EQ(same_dir_site, app_site) are
+// sanity checks of the test setup.
+//
+// First real verification in the test is whether |same_dir| and |diff_dir| can
+// script each other (despite their effective URLs being cross-site, because
+// |same_dir| is mapped to a chrome-extension URL).  This was a functionality
+// problem caused by https://crbug.com/679011.
+//
+// The test also verifies that all same-site frames (i.e. |app|, |same_dir|,
+// |diff_dir|, |same_site|) share the same renderer process.  This was a small
+// performance problem caused by https://crbug.com/679011.
+IN_PROC_BROWSER_TEST_F(HostedAppVsTdiTest, ProcessAllocation) {
+  //
+  // Setup and launch the app.
+  //
+
+  GURL url = embedded_test_server()->GetURL(
+      "app.site.com", "/frame_tree/cross_origin_but_same_site_frames.html");
+
+  extensions::TestExtensionDir test_app_dir;
+  test_app_dir.WriteManifestWithSingleQuotes(
+      base::StringPrintf("{\n"
+                         "  'name': 'Hosted App vs TDI Test',\n"
+                         "  'version': '1',\n"
+                         "  'manifest_version': 2,\n"
+                         "  'app': {\n"
+                         "    'launch': {\n"
+                         "      'web_url': '%s'\n"
+                         "    },\n"
+                         "    'urls': ['*://app.site.com/frame_tree']\n"
+                         "  }\n"
+                         "}\n",
+                         url.spec().c_str()));

[ncarter (slow), 2017/04/28 18:50:14]

You could also use C++11 raw string literals here:

  test_app_dir.WriteManifestWithSingleQuotes(base::StringPrintf(
      R"({
          'name': 'Hosted App vs TDI Test',
          'version': '1',
          'manifest_version': 2,
          'app': {
            'launch': {
              'web_url': '%s'
            },
            'urls': ['*://app.site.com/frame_tree']
          }
      })",
      url.spec().c_str()));

(technically you could use the non-singlequote version too, but I think it's
easier to read with singlequotes, and works better with syntax highlighters that
aren't hip to C++11 string literals)

+  SetupApp(test_app_dir.UnpackedPath(), false);
+
+  content::WebContents* web_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  content::WaitForLoadStop(web_contents);
+
+  auto find_frame = [web_contents](const std::string& name) {
+    return content::FrameMatchingPredicate(
+        web_contents, base::Bind(&content::FrameMatchesName, name));
+  };

[Łukasz Anforowicz, 2017/04/28 17:30:27]

I guess the above might need to be refactored depending on where we go with
https://codereview.chromium.org/2849603005.

+  content::RenderFrameHost* app = web_contents->GetMainFrame();
+  content::RenderFrameHost* same_dir = find_frame("SameOrigin-SamePath");
+  content::RenderFrameHost* diff_dir = find_frame("SameOrigin-DifferentPath");
+  content::RenderFrameHost* same_site = find_frame("OtherSubdomain-SameSite");
+  content::RenderFrameHost* cross_site = find_frame("CrossSite");
+
+  //
+  // Sanity-check sites of all relevant frames to verify test setup.
+  //
+
+  GURL app_site = content::SiteInstance::GetSiteForURL(
+      app_browser_->profile(), app->GetLastCommittedURL());
+  EXPECT_EQ(extensions::kExtensionScheme, app_site.scheme());
+
+  GURL same_dir_site = content::SiteInstance::GetSiteForURL(
+      app_browser_->profile(), same_dir->GetLastCommittedURL());
+  EXPECT_EQ(extensions::kExtensionScheme, same_dir_site.scheme());
+  EXPECT_EQ(same_dir_site, app_site);
+
+  GURL diff_dir_site = content::SiteInstance::GetSiteForURL(
+      app_browser_->profile(), diff_dir->GetLastCommittedURL());
+  EXPECT_NE(extensions::kExtensionScheme, diff_dir_site.scheme());
+  EXPECT_NE(diff_dir_site, app_site);
+
+  GURL same_site_site = content::SiteInstance::GetSiteForURL(
+      app_browser_->profile(), same_site->GetLastCommittedURL());
+  EXPECT_NE(extensions::kExtensionScheme, same_site_site.scheme());
+  EXPECT_NE(same_site_site, app_site);
+  EXPECT_EQ(same_site_site, diff_dir_site);
+
+  GURL cross_site_site = content::SiteInstance::GetSiteForURL(
+      app_browser_->profile(), cross_site->GetLastCommittedURL());
+  EXPECT_NE(cross_site_site, app_site);
+  EXPECT_NE(cross_site_site, same_site_site);
+
+  //
+  // Verify that |same_dir| and |diff_dir| can script each other.
+  // (they should - they have the same origin).
+  //
+
+  std::string same_dir_origin;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+      same_dir, "domAutomationController.send(window.origin)",
+      &same_dir_origin));
+
+  std::string diff_dir_origin;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+      diff_dir, "domAutomationController.send(window.origin)",
+      &diff_dir_origin));
+
+  EXPECT_EQ(diff_dir_origin, same_dir_origin);
+
+  std::string inner_text_from_other_frame;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+      diff_dir,
+      "var w = window.open('', 'SameOrigin-SamePath')\n"
+      "domAutomationController.send(w.document.body.innerText)",
+      &inner_text_from_other_frame));
+  EXPECT_EQ("Simple test page.", inner_text_from_other_frame);
+
+  //
+  // Verify there are no additional processes for same-site frames.
+  //
+
+  EXPECT_EQ(same_dir->GetProcess(), app->GetProcess());
+  EXPECT_EQ(diff_dir->GetProcess(), app->GetProcess());
+  EXPECT_EQ(same_site->GetProcess(), app->GetProcess());
+  // For now it is okay for |cross_site| to be in any process - see
+  // https://crbug.com/679011#c2.
+}