Site URL for file://<host>/... needs to match renderer-side origin.

The site URL is used in browser-side isolation enforcements and compared
against the origin requested by the renderer (e.g. when a renderer
tries to open localStorage for an origin, or [in the future -
https://crrev.com/c/769647] when a renderer wants to commit a navigation
with a specific origin).

Before this CL, browser-side isolation enforcement code would calculate
site URL for file: URLs as follows:
1. file:///home/lukasza/file.txt => site url = file:
2. file://localhost/home/lukasza/file.txt => site url = file://localhost/

Behavior before this CL was problematic, because the origin requested
by the renderer is the same in both cases above - this means that the
requested origin doesn't match the site URL in the 2nd case (and this
leads to renderer kills, like the one observed in
https://crbug.com/776160).

This CL changes how site URL is calculated by the browser process.
After the change, the same site URL (file:) is used for both the cases
outlined above.  Because of this change, the browser-side and
renderer-side notion of the origin is kept in sync (and we avoid
renderer kills).

Bug: 776160
Change-Id: I99ce397fede346b2278f053e0fa01e8e314741e2
Reviewed-on: https://chromium-review.googlesource.com/827550
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524255}

content/browser/dom_storage/dom_storage_browsertest.cc

@@ -18,6 +18,7 @@
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_launcher.h"
 #include "content/shell/browser/shell.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace content {
 
@@ -119,6 +120,37 @@ IN_PROC_BROWSER_TEST_F(MojoDOMStorageBrowserTest, MAYBE_DataPersists) {
   SimpleTest(GetTestUrl("dom_storage", "verify_data.html"), kNotIncognito);
 }
 
+// On Windows file://localhost/C:/src/chromium/src/content/test/data/title1.html
+// doesn't work.
+#if !defined(OS_WIN)
+// Regression test for https://crbug.com/776160.  The test verifies that there
+// is no disagreement between 1) site URL used for browser-side isolation
+// enforcement and 2) the origin requested by Blink.  Before this bug was fixed,
+// (1) was file://localhost/ and (2) was file:// - this led to renderer kills.
+IN_PROC_BROWSER_TEST_F(MojoDOMStorageBrowserTest, FileUrlWithHost) {
+  // Navigate to file://localhost/.../title1.html
+  GURL regular_file_url = GetTestUrl(nullptr, "title1.html");
+  GURL::Replacements host_replacement;
+  host_replacement.SetHostStr("localhost");
+  GURL file_with_host_url =
+      regular_file_url.ReplaceComponents(host_replacement);
+  EXPECT_TRUE(NavigateToURL(shell(), file_with_host_url));
+  EXPECT_THAT(shell()->web_contents()->GetLastCommittedURL().spec(),
+              testing::StartsWith("file://localhost/"));
+  EXPECT_THAT(shell()->web_contents()->GetLastCommittedURL().spec(),
+              testing::EndsWith("/title1.html"));
+
+  // Verify that window.localStorage works fine.
+  std::string result;
+  std::string script = R"(
+      localStorage["foo"] = "bar";
+      domAutomationController.send(localStorage["foo"]);
+  )";
+  EXPECT_TRUE(ExecuteScriptAndExtractString(shell(), script, &result));
+  EXPECT_EQ("bar", result);
+}
+#endif
+
 class DOMStorageMigrationBrowserTest : public DOMStorageBrowserTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {

content/browser/site_instance_impl.cc

@@ -4,6 +4,8 @@
 
 #include "content/browser/site_instance_impl.h"
 
+#include <string>
+
 #include "base/command_line.h"
 #include "base/debug/crash_logging.h"
 #include "base/macros.h"
@@ -409,8 +411,10 @@ GURL SiteInstance::GetSiteForURL(BrowserContext* browser_context,
     return isolated_origin.GetURL();
   }
 
-  // If the url has a host, then determine the site.
-  if (!origin.host().empty()) {
+  // If the url has a host, then determine the site.  Skip file URLs to avoid a
+  // situation where site URL of file://localhost/ would mismatch Blink's origin
+  // (which ignores the hostname in this case - see https://crbug.com/776160).
+  if (!origin.host().empty() && origin.scheme() != url::kFileScheme) {
     // Only keep the scheme and registered domain of |origin|.
     std::string domain = net::registry_controlled_domains::GetDomainAndRegistry(
         origin.host(),

content/browser/site_instance_impl_unittest.cc

@@ -331,11 +331,14 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   EXPECT_EQ("file", site_url.scheme());
   EXPECT_FALSE(site_url.has_host());
 
-  // Some file URLs have hosts in the path.
+  // Some file URLs have hosts in the path.  For consistency with Blink (which
+  // maps *all* file://... URLs into "file://" origin) such file URLs still need
+  // to map into "file:" site URL.  See also https://crbug.com/776160.
   test_url = GURL("file://server/path");
   site_url = SiteInstanceImpl::GetSiteForURL(nullptr, test_url);
-  EXPECT_EQ(GURL("file://server"), site_url);
-  EXPECT_EQ("server", site_url.host());
+  EXPECT_EQ(GURL("file:"), site_url);
+  EXPECT_EQ("file", site_url.scheme());
+  EXPECT_FALSE(site_url.has_host());
 
   // Data URLs should include the scheme.
   test_url = GURL("data:text/html,foo");