Don't create separate SiteInstances for pages from the same domain and scheme

but from different ports.  (These pages can still access each other.)

(This is copied from http://codereview.chromium.org/12443, which has already
been reviewed by darin and abarth.  Just had to commit from a different
checked out codebase.)

BUG=4792
R=darin,abarth


Review URL: http://codereview.chromium.org/12451

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@6014 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 32 insertions, 14 deletions

diff --git a/chrome/browser/site_instance.cc b/chrome/browser/site_instance.cc
index a42c790..63517d8 100644
--- a/chrome/browser/site_instance.cc
+++ b/chrome/browser/site_instance.cc
@@ -85,9 +85,17 @@ GURL SiteInstance::GetSiteForURL(const GURL& url) {
 
   // If the url has a host, then determine the site.
   if (url.has_host()) {
-    // Only keep the scheme, registered domain, and port as given by GetOrigin.
+    // Only keep the scheme and registered domain as given by GetOrigin.  This
+    // may also include a port, which we need to drop.
     site = url.GetOrigin();
 
+    // Remove port, if any.
+    if (site.has_port()) {
+      GURL::Replacements rep;
+      rep.ClearPort();
+      site = site.ReplaceComponents(rep);
+    }
+
     // If this URL has a registered domain, we only want to remember that part.
     std::string domain =
         net::RegistryControlledDomainService::GetDomainAndRegistry(url);
@@ -103,7 +111,9 @@ GURL SiteInstance::GetSiteForURL(const GURL& url) {
 /*static*/
 bool SiteInstance::IsSameWebSite(const GURL& url1, const GURL& url2) {
   // We infer web site boundaries based on the registered domain name of the
-  // top-level page, as well as the scheme and the port.
+  // top-level page and the scheme.  We do not pay attention to the port if
+  // one is present, because pages served from different ports can still
+  // access each other if they change their document.domain variable.
 
   // We must treat javascript: URLs as part of the same site, regardless of
   // the site.
@@ -125,8 +135,8 @@ bool SiteInstance::IsSameWebSite(const GURL& url1, const GURL& url2) {
     return false;
   }
 
-  // If the scheme or port differ, they aren't part of the same site.
-  if (url1.scheme() != url2.scheme() || url1.port() != url2.port()) {
+  // If the schemes differ, they aren't part of the same site.
+  if (url1.scheme() != url2.scheme()) {
     return false;
   }
 
diff --git a/chrome/browser/site_instance.h b/chrome/browser/site_instance.h
index 904d52f..6759fe3 100644
--- a/chrome/browser/site_instance.h
+++ b/chrome/browser/site_instance.h
@@ -15,7 +15,7 @@
 //
 // A SiteInstance is a data structure that is associated with all pages in a
 // given instance of a web site.  Here, a web site is identified by its
-// registered domain name, scheme, and port.  An instance includes all pages
+// registered domain name and scheme.  An instance includes all pages
 // that are connected (i.e., either a user or a script navigated from one
 // to the other).  We represent instances using the BrowsingInstance class.
 //
@@ -70,8 +70,9 @@ class SiteInstance : public base::RefCounted<SiteInstance> {
   RenderProcessHost* GetProcess();
 
   // Set / Get the web site that this SiteInstance is rendering pages for.
-  // This includes the scheme, registered domain, and port.  If the URL does
-  // not have a valid registered domain, then the full hostname is stored.
+  // This includes the scheme and registered domain, but not the port.  If the
+  // URL does not have a valid registered domain, then the full hostname is
+  // stored.
   void SetSite(const GURL& url);
   const GURL& site() const { return site_; }
   bool has_site() const { return has_site_; }
@@ -100,15 +101,14 @@ class SiteInstance : public base::RefCounted<SiteInstance> {
   // Darin suggests.
   static SiteInstance* CreateSiteInstance(Profile* profile);
 
-  // Returns the site for the given URL, which includes only the scheme,
-  // registered domain, and port.  Returns an empty GURL if the URL has no
-  // host.
+  // Returns the site for the given URL, which includes only the scheme and
+  // registered domain.  Returns an empty GURL if the URL has no host.
   static GURL GetSiteForURL(const GURL& url);
 
   // Return whether both URLs are part of the same web site, for the purpose of
   // assigning them to processes accordingly.  The decision is currently based
   // on the registered domain of the URLs (google.com, bbc.co.uk), as well as
-  // the scheme (https, http) and port.  This ensures that two pages will be in
+  // the scheme (https, http).  This ensures that two pages will be in
   // the same process if they can communicate with other via JavaScript.
   // (e.g., docs.google.com and mail.google.com have DOM access to each other
   // if they both set their document.domain properties to google.com.)
diff --git a/chrome/browser/site_instance_unittest.cc b/chrome/browser/site_instance_unittest.cc
index 69c83fa..dc0297e 100644
--- a/chrome/browser/site_instance_unittest.cc
+++ b/chrome/browser/site_instance_unittest.cc
@@ -202,13 +202,15 @@ TEST_F(SiteInstanceTest, SetSite) {
 
 // Test to ensure GetSiteForURL properly returns sites for URLs.
 TEST_F(SiteInstanceTest, GetSiteForURL) {
+  // Pages are irrelevant.
   GURL test_url = GURL("http://www.google.com/index.html");
   EXPECT_EQ(GURL("http://google.com"), SiteInstance::GetSiteForURL(test_url));
 
+  // Ports are irrlevant.
   test_url = GURL("https://www.google.com:8080");
-  EXPECT_EQ(GURL("https://google.com:8080"),
-            SiteInstance::GetSiteForURL(test_url));
+  EXPECT_EQ(GURL("https://google.com"), SiteInstance::GetSiteForURL(test_url));
 
+  // Javascript URLs have no site.
   test_url = GURL("javascript:foo();");
   EXPECT_EQ(GURL::EmptyGURL(), SiteInstance::GetSiteForURL(test_url));
 
@@ -238,9 +240,15 @@ TEST_F(SiteInstanceTest, IsSameWebSite) {
   GURL url_hang = GURL("about:hang");
   GURL url_shorthang = GURL("about:shorthang");
 
+  // Same scheme and port -> same site.
   EXPECT_TRUE(SiteInstance::IsSameWebSite(url_foo, url_foo2));
+
+  // Different scheme -> different site.
   EXPECT_FALSE(SiteInstance::IsSameWebSite(url_foo, url_foo_https));
-  EXPECT_FALSE(SiteInstance::IsSameWebSite(url_foo, url_foo_port));
+
+  // Different port -> same site.
+  // (Changes to document.domain make renderer ignore the port.)
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(url_foo, url_foo_port));
 
   // JavaScript links should be considered same site for anything.
   EXPECT_TRUE(SiteInstance::IsSameWebSite(url_javascript, url_foo));