Replace SiteInstance::IsSameWebSite() with a new non-static method.

Currently, SiteInstance::IsSameWebSite() is static and will need to be
modified to support dynamic isolated origins.  Those origins will
apply only to future BrowsingInstances, meaning the answer to
IsSameWebSite will depend on which frame/SiteInstance is asking this
question.  This CL replaces this method with a non-static
SiteInstance::IsSameSiteWithURL method.  This will ensure that the
internal implementation will be able to provide sufficient context
(i.e., BrowsingInstance info) in the future, without having to expose
that context outside of content/.  Note that the content-internal
version of this call, SiteInstanceImpl::IsSameWebSite, stays as-is for
now.

The only two non-test uses of this were in NaCl code.  They were
checking whether the current SiteInstance's site URL is same-site with
the URL of the NaCl file to be loaded, with both URLs expected to be
extension URLs.  There should be no behavior change in these, as the
underlying implementation doesn't change.

A few tests are also refactored to either avoid using IsSameWebSite
entirely, or, for tests inside content/, to use the internal version
of IsSameWebSite.

Bug: 905513
Change-Id: Ia2957bb1ec7a16de8c3d18ef167149f1f5a08066
Reviewed-on: https://chromium-review.googlesource.com/c/1352856Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612495}

chrome/browser/resource_coordinator/tab_manager_browsertest.cc

@@ -196,10 +196,10 @@ class TabManagerTest : public InProcessBrowserTest {
     // The page has 2 iframes, we will use the first one.
     content::RenderFrameHost* child_frame = content->GetAllFrames()[1];
     // Verify that the main frame and subframe are cross-site.
-    EXPECT_FALSE(content::SiteInstance::IsSameWebSite(
-        browser()->profile(), main_frame->GetLastCommittedURL(),
-        child_frame->GetLastCommittedURL()));
+    EXPECT_NE(main_frame->GetLastCommittedURL().GetOrigin(),
+              child_frame->GetLastCommittedURL().GetOrigin());
     if (content::AreAllSitesIsolatedForTesting()) {
+      EXPECT_NE(main_frame->GetSiteInstance(), child_frame->GetSiteInstance());
       EXPECT_NE(main_frame->GetProcess()->GetID(),
                 child_frame->GetProcess()->GetID());
     }
@@ -1379,10 +1379,10 @@ IN_PROC_BROWSER_TEST_F(TabManagerTest,
 
   // Sanity check that in this test page the main frame and the
   // subframe are cross-site.
-  EXPECT_FALSE(content::SiteInstance::IsSameWebSite(
-      browser()->profile(), main_frame->GetLastCommittedURL(),
-      child_frame->GetLastCommittedURL()));
+  EXPECT_NE(main_frame->GetLastCommittedURL().GetOrigin(),
+            child_frame->GetLastCommittedURL().GetOrigin());
   if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_NE(main_frame->GetSiteInstance(), child_frame->GetSiteInstance());
     EXPECT_NE(main_frame->GetProcess()->GetID(),
               child_frame->GetProcess()->GetID());
   }

chrome/browser/tab_contents/view_source_browsertest.cc

@@ -251,10 +251,11 @@ IN_PROC_BROWSER_TEST_F(ViewSourceTest, CrossSiteSubframe) {
 
   // Do a sanity check that in this particular test page the main frame and the
   // subframe are cross-site.
-  EXPECT_FALSE(content::SiteInstance::IsSameWebSite(
-      browser()->profile(), original_main_frame->GetLastCommittedURL(),
-      original_child_frame->GetLastCommittedURL()));
+  EXPECT_NE(original_main_frame->GetLastCommittedURL().GetOrigin(),
+            original_child_frame->GetLastCommittedURL().GetOrigin());
   if (content::AreAllSitesIsolatedForTesting()) {
+    EXPECT_NE(original_main_frame->GetSiteInstance(),
+              original_child_frame->GetSiteInstance());
     EXPECT_NE(original_main_frame->GetProcess()->GetID(),
               original_child_frame->GetProcess()->GetID());
   }

components/nacl/browser/nacl_file_host.cc

@@ -245,9 +245,7 @@ void OpenNaClExecutable(
     return;
   }
   content::SiteInstance* site_instance = rvh->GetSiteInstance();
-  if (!content::SiteInstance::IsSameWebSite(site_instance->GetBrowserContext(),
-                                            site_instance->GetSiteURL(),
-                                            file_url)) {
+  if (!site_instance->IsSameSiteWithURL(file_url)) {
     NotifyRendererOfError(nacl_host_message_filter.get(), reply_msg);
     return;
   }

components/nacl/browser/nacl_host_message_filter.cc

@@ -171,12 +171,8 @@ void NaClHostMessageFilter::LaunchNaClContinuation(
     GURL gurl(original_request_list[i].resource_url);
     // Important security check: Do the same check as OpenNaClExecutable()
     // in nacl_file_host.cc.
-    if (!content::SiteInstance::IsSameWebSite(
-            site_instance->GetBrowserContext(),
-            site_instance->GetSiteURL(),
-            gurl)) {
+    if (!site_instance->IsSameSiteWithURL(gurl))
       continue;
-    }
     safe_launch_params.resource_prefetch_request_list.push_back(
         original_request_list[i]);
   }

content/browser/loader/cross_site_document_blocking_browsertest.cc

@@ -17,6 +17,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "content/browser/loader/cross_site_document_resource_handler.h"
+#include "content/browser/site_instance_impl.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
@@ -764,9 +765,10 @@ class CrossSiteDocumentBlockingServiceWorkerTest : public ContentBrowserTest {
     // Sanity check of test setup - the 2 https servers should be cross-site
     // (the second server should have a different hostname because of the call
     // to SetSSLConfig with CERT_COMMON_NAME_IS_DOMAIN argument).
-    ASSERT_FALSE(SiteInstance::IsSameWebSite(
+    ASSERT_FALSE(SiteInstanceImpl::IsSameWebSite(
         shell()->web_contents()->GetBrowserContext(),
-        GetURLOnServiceWorkerServer("/"), GetURLOnCrossOriginServer("/")));
+        GetURLOnServiceWorkerServer("/"), GetURLOnCrossOriginServer("/"),
+        true /* should_use_effective_urls */));
   }
 
   GURL GetURLOnServiceWorkerServer(const std::string& path) {

content/browser/site_instance_impl.cc

@@ -333,14 +333,13 @@ bool SiteInstance::ShouldAssignSiteForURL(const GURL& url) {
   return SiteInstanceImpl::ShouldAssignSiteForURL(url);
 }
 
-// static
-bool SiteInstance::IsSameWebSite(BrowserContext* browser_context,
-                                 const GURL& real_src_url,
-                                 const GURL& real_dest_url) {
-  return SiteInstanceImpl::IsSameWebSite(browser_context, real_src_url,
-                                         real_dest_url, true);
+bool SiteInstanceImpl::IsSameSiteWithURL(const GURL& url) {
+  return SiteInstanceImpl::IsSameWebSite(
+      browsing_instance_->browser_context(), site_, url,
+      true /* should_compare_effective_urls */);
 }
 
+// static
 bool SiteInstanceImpl::IsSameWebSite(BrowserContext* browser_context,
                                      const GURL& real_src_url,
                                      const GURL& real_dest_url,

content/browser/site_instance_impl.h

@@ -45,10 +45,15 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   // not if the lock is empty or applies to an entire scheme (e.g., file://).
   static bool IsOriginLockASite(const GURL& lock_url);
 
-  // See SiteInstance::IsSameWebSite.
-  // This version allows comparing URLs without converting them to effective
-  // URLs first, which is useful for avoiding OOPIFs when otherwise same-site
-  // URLs may look cross-site via their effective URLs.
+  // 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).  Note that if the destination is a blank page,
+  // we consider that to be part of the same web site for the purposes for
+  // process assignment.  |should_compare_effective_urls| allows comparing URLs
+  // without converting them to effective URLs first.  This is useful for
+  // avoiding OOPIFs when otherwise same-site URLs may look cross-site via
+  // their effective URLs.
   static bool IsSameWebSite(content::BrowserContext* browser_context,
                             const GURL& src_url,
                             const GURL& dest_url,
@@ -64,6 +69,7 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   bool IsRelatedSiteInstance(const SiteInstance* instance) override;
   size_t GetRelatedActiveContentsCount() override;
   bool RequiresDedicatedProcess() override;
+  bool IsSameSiteWithURL(const GURL& url) override;
 
   // The policy to apply when selecting a RenderProcessHost for the
   // SiteInstance. If no suitable RenderProcessHost for the SiteInstance exists

content/public/browser/site_instance.h

@@ -130,6 +130,18 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
   // process. This only returns true under the "site per process" process model.
   virtual bool RequiresDedicatedProcess() = 0;
 
+  // Return whether this SiteInstance and the provided |url| 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). 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.) Note that if the destination is a blank page, we consider
+  // that to be part of the same web site for the purposes for process
+  // assignment.
+  virtual bool IsSameSiteWithURL(const GURL& url) = 0;
+
   // Factory method to create a new SiteInstance.  This will create a new
   // new BrowsingInstance, so it should only be used when creating a new tab
   // from scratch (or similar circumstances).
@@ -151,19 +163,6 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted<SiteInstance> {
   // chrome-native:// leave the site unassigned.
   static bool ShouldAssignSiteForURL(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).  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.)
-  // Note that if the destination is a blank page, we consider that to be part
-  // of the same web site for the purposes for process assignment.
-  static bool IsSameWebSite(content::BrowserContext* browser_context,
-                            const GURL& src_url,
-                            const GURL& dest_url);
-
   // 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. Prior to
   // determining the site, |url| is resolved to an effective URL via