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..11a3de439e16a63093fe1f5307f583fb0cc3585c 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,166 @@ 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 hosted app.
+  //

[Devlin, 2017/05/04 17:20:45]

style nit: I don't think these warrant
//
// Three-line-comments
//

<code>

Maybe just
// Do foo
<code>

[Łukasz Anforowicz, 2017/05/04 18:58:20]

Done.  I did the header-style comments, because I wasn't sure if it is obvious
that the comment applies to all the statements until the next comment (not just
to the first blank line).

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

[Devlin, 2017/05/04 17:20:45]

Optional: Now that we can use raw string literals, it's sometimes cleaner to do
that (and the SingleQuotes method is a little less useful).  e.g.,

base::StringPrintf(
    R"(
       ...
    )"

It avoids the need for the repeated \ns.

[Łukasz Anforowicz, 2017/05/04 18:58:20]

Done.

Also - what I have in the latest patchset was formatted manually.  Raw strings
are (understandably I guess) problematic for clang-format - I've opened
https://bugs.llvm.org/show_bug.cgi?id=32928

+                         "  '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()));
+  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));
+  };
+  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| have the same origin according to
+  // |window.origin| (even though they have different |same_dir_site| and
+  // |diff_dir_site|).
+  //
+
+  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);
+
+  //
+  // Verify scriptability and process placement.
+  //
+
+  EXPECT_EQ(same_dir->GetProcess(), app->GetProcess());
+  if (!content::AreAllSitesIsolatedForTesting()) {
+    // Verify that |same_dir| and |diff_dir| can script each other.
+    // (they should - they have the same 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(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.

[Devlin, 2017/05/04 17:20:45]

I think we should add a TODO to address this.

[Łukasz Anforowicz, 2017/05/04 18:58:20]

Done (in the TODO, I am referring to the newly opened https://crbug.com/718516).

+  } else {
+    // Process policy is not well-defined / settled wrt relationship between 1)
+    // hosted apps and 2) same-site web content outside of hosted app's extent.
+    // When this test was authored --site-per-process would put |app| in a
+    // separate renderer process from |diff_dir| and |same_site|, even though
+    // such process placement can be problematic (if |app| tries to
+    // synchronously script |diff_dir| and/or |same_site|).

[Devlin, 2017/05/04 17:20:45]

Should we try to fix that?

[Łukasz Anforowicz, 2017/05/04 18:58:20]

Yes, although the fix is not urgent given that --site-per-process is a test-only
mode and the issue doesn't affect --isolate-extensions and (after this CL)
--top-document-isolation.  I've added a reference to https://crbug.com/718516 in
the comment.

+  }
+}