Change UnmatchedServiceWorkerProcessTracker to handle default SiteInstance.

- Added code that enables the default SiteInstance to use an unmatched
  service worker process for a site that is handled by the default SiteInstance.
- Remove default SiteInstance specific conditions from tests that were
  testing process reuse and unmatched service worker process behavior.
- Added code to BrowsingInstance that tracks what site URLs are associated
  with its default SiteInstance. This allows process reuse to be constrained
  to sites explicitly associated with a specific default SiteInstance.

Bug: 958060
Change-Id: I9ce95dbd5ecc192cba8cd4e6046787d248ff3532
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1668449Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672681}

content/browser/browsing_instance.cc

@@ -55,6 +55,10 @@ bool BrowsingInstance::IsDefaultSiteInstance(
   return site_instance != nullptr && site_instance == default_site_instance_;
 }
 
+bool BrowsingInstance::IsSiteInDefaultSiteInstance(const GURL& site_url) const {
+  return site_url_set_.find(site_url) != site_url_set_.end();
+}
+
 bool BrowsingInstance::HasSiteInstance(const GURL& url) {
   std::string site = GetSiteForURL(url).possibly_invalid_spec();
   return site_instance_map_.find(site) != site_instance_map_.end();
@@ -100,8 +104,8 @@ void BrowsingInstance::GetSiteAndLockForURL(const GURL& url,
 scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
     const GURL& url,
     bool allow_default_instance) {
-  std::string site = GetSiteForURL(url).possibly_invalid_spec();
-  auto i = site_instance_map_.find(site);
+  const GURL site_url = GetSiteForURL(url);
+  auto i = site_instance_map_.find(site_url.possibly_invalid_spec());
   if (i != site_instance_map_.end())
     return i->second;
 
@@ -132,6 +136,10 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
 
       site_instance->SetSite(SiteInstanceImpl::GetDefaultSiteURL());
     }
+
+    // Add |site_url| to the set so we can keep track of all the sites the
+    // the default SiteInstance has been returned for.
+    site_url_set_.insert(site_url);
     return site_instance;
   }
 

content/browser/browsing_instance.h

@@ -161,6 +161,14 @@ class CONTENT_EXPORT BrowsingInstance final
 
   bool IsDefaultSiteInstance(const SiteInstanceImpl* site_instance) const;
 
+  // Returns true if |site_url| has been used to get a SiteInstance from this
+  // object and the default SiteInstance was returned. This simply indicates
+  // the site may be directed to the default SiteInstance process, but it does
+  // not indicate that the site has already been committed to that process.
+  // Returns false if no request for |site_url| has resulted in this object
+  // returning the default SiteInstance.
+  bool IsSiteInDefaultSiteInstance(const GURL& site_url) const;
+
   // Helper function used by other methods in this class to ensure consistent
   // mapping between |url| and site URL.
   // Note: This should not be used by code outside this class.
@@ -207,6 +215,10 @@ class CONTENT_EXPORT BrowsingInstance final
   // BrowsingInstance and the SiteInstanceImpl.
   SiteInstanceImpl* default_site_instance_;
 
+  // Keeps track of the site URLs that this object mapped to the
+  // |default_site_instance_|.
+  std::set<GURL> site_url_set_;
+
   DISALLOW_COPY_AND_ASSIGN(BrowsingInstance);
 };
 

content/browser/renderer_host/render_process_host_impl.cc

@@ -1012,6 +1012,10 @@ class UnmatchedServiceWorkerProcessTracker
   }
 
  private:
+  using ProcessID = int;
+  using SiteProcessIDPair = std::pair<GURL, ProcessID>;
+  using SiteProcessIDPairSet = std::set<SiteProcessIDPair>;
+
   void RegisterProcessForSite(RenderProcessHost* host,
                               SiteInstanceImpl* site_instance) {
     if (!HasProcess(host))
@@ -1022,7 +1026,15 @@ class UnmatchedServiceWorkerProcessTracker
 
   RenderProcessHost* TakeFreshestProcessForSite(
       SiteInstanceImpl* site_instance) {
-    RenderProcessHost* host = FindFreshestProcessForSite(site_instance);
+    SiteProcessIDPair site_process_pair =
+        FindFreshestProcessForSite(site_instance);
+
+    if (site_process_pair.first.is_empty())
+      return nullptr;
+
+    RenderProcessHost* host =
+        RenderProcessHost::FromID(site_process_pair.second);
+
     if (!host)
       return nullptr;
 
@@ -1037,20 +1049,31 @@ class UnmatchedServiceWorkerProcessTracker
                                      site_url, site_instance->lock_url()))
       return nullptr;
 
-    site_process_set_.erase(SiteProcessIDPair(site_url, host->GetID()));
+    site_process_set_.erase(site_process_pair);
     if (!HasProcess(host))
       host->RemoveObserver(this);
     return host;
   }
 
-  RenderProcessHost* FindFreshestProcessForSite(
+  SiteProcessIDPair FindFreshestProcessForSite(
       SiteInstanceImpl* site_instance) const {
-    GURL site_url(site_instance->GetSiteURL());
-    for (const auto& site_process_pair : base::Reversed(site_process_set_)) {
-      if (site_process_pair.first == site_url)
-        return RenderProcessHost::FromID(site_process_pair.second);
+    const auto reversed_site_process_set = base::Reversed(site_process_set_);
+    if (site_instance->IsDefaultSiteInstance()) {
+      // See if we can find an entry that maps to a site associated with the
+      // default SiteInstance. This allows the default SiteInstance to reuse a
+      // service worker process for any site that has been associated with it.
+      for (const auto& site_process_pair : reversed_site_process_set) {
+        if (site_instance->IsSiteInDefaultSiteInstance(site_process_pair.first))
+          return site_process_pair;
+      }
+    } else {
+      const GURL site_url(site_instance->GetSiteURL());
+      for (const auto& site_process_pair : reversed_site_process_set) {
+        if (site_process_pair.first == site_url)
+          return site_process_pair;
+      }
     }
-    return nullptr;
+    return SiteProcessIDPair();
   }
 
   // Returns true if this tracker contains the process ID |host->GetID()|.
@@ -1063,10 +1086,6 @@ class UnmatchedServiceWorkerProcessTracker
     return false;
   }
 
-  using ProcessID = int;
-  using SiteProcessIDPair = std::pair<GURL, ProcessID>;
-  using SiteProcessIDPairSet = std::set<SiteProcessIDPair>;
-
   // Use std::set because duplicates don't need to be tracked separately (eg.,
   // service workers for the same site in the same process). It is sorted in the
   // order of insertion.

content/browser/renderer_host/render_process_host_unittest.cc

@@ -197,14 +197,7 @@ TEST_F(RenderProcessHostUnitTest, ReuseUnmatchedServiceWorkerProcess) {
   // the newest unmatched service worker's process (i.e., sw_host2).
   scoped_refptr<SiteInstanceImpl> site_instance1 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(site_instance1->IsDefaultSiteInstance());
-    // TODO(acolwell): Remove once CreateForServiceWorker() can return a
-    // default SiteInstance.
-    EXPECT_NE(sw_host2, site_instance1->GetProcess());
-  } else {
-    EXPECT_EQ(sw_host2, site_instance1->GetProcess());
-  }
+  EXPECT_EQ(sw_host2, site_instance1->GetProcess());
 
   // Getting a RenderProcessHost for a navigation to the same site must reuse
   // the newest unmatched service worker's process (i.e., sw_host1). sw_host2
@@ -212,25 +205,14 @@ TEST_F(RenderProcessHostUnitTest, ReuseUnmatchedServiceWorkerProcess) {
   // with a corresponding unmatched service worker.
   scoped_refptr<SiteInstanceImpl> site_instance2 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(site_instance2->IsDefaultSiteInstance());
-    // TODO(acolwell): Remove once CreateForServiceWorker() can return a
-    // default SiteInstance.
-    EXPECT_NE(sw_host1, site_instance2->GetProcess());
-  } else {
-    EXPECT_EQ(sw_host1, site_instance2->GetProcess());
-  }
+  EXPECT_EQ(sw_host1, site_instance2->GetProcess());
 
   // Getting a RenderProcessHost for a navigation should return a new process
   // because there is no unmatched service worker's process.
   scoped_refptr<SiteInstanceImpl> site_instance3 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(site_instance3->IsDefaultSiteInstance());
-  } else {
-    EXPECT_NE(sw_host1, site_instance3->GetProcess());
-    EXPECT_NE(sw_host2, site_instance3->GetProcess());
-  }
+  EXPECT_NE(sw_host1, site_instance3->GetProcess());
+  EXPECT_NE(sw_host2, site_instance3->GetProcess());
 }
 
 class UnsuitableHostContentBrowserClient : public ContentBrowserClient {
@@ -322,14 +304,7 @@ TEST_F(RenderProcessHostUnitTest, ReuseServiceWorkerProcessForServiceWorker) {
   // the newest unmatched service worker's process (i.e., sw_host2).
   scoped_refptr<SiteInstanceImpl> site_instance1 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(site_instance1->IsDefaultSiteInstance());
-    // TODO(acolwell): Remove once CreateForServiceWorker() can return
-    // a default SiteInstance.
-    EXPECT_NE(sw_host2, site_instance1->GetProcess());
-  } else {
-    EXPECT_EQ(sw_host2, site_instance1->GetProcess());
-  }
+  EXPECT_EQ(sw_host2, site_instance1->GetProcess());
 
   // Getting a RenderProcessHost for a navigation to the same site must reuse
   // the newest unmatched service worker's process (i.e., sw_host1). sw_host2
@@ -337,14 +312,7 @@ TEST_F(RenderProcessHostUnitTest, ReuseServiceWorkerProcessForServiceWorker) {
   // with a corresponding unmatched service worker.
   scoped_refptr<SiteInstanceImpl> site_instance2 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(site_instance2->IsDefaultSiteInstance());
-    // TODO(acolwell): Remove once CreateForServiceWorker() can return
-    // a default SiteInstance.
-    EXPECT_NE(sw_host1, site_instance2->GetProcess());
-  } else {
-    EXPECT_EQ(sw_host1, site_instance2->GetProcess());
-  }
+  EXPECT_EQ(sw_host1, site_instance2->GetProcess());
 }
 
 TEST_F(RenderProcessHostUnitTest,
@@ -370,28 +338,14 @@ TEST_F(RenderProcessHostUnitTest,
   scoped_refptr<SiteInstanceImpl> sw_site_instance3 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
   RenderProcessHost* sw_host3 = sw_site_instance3->GetProcess();
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(sw_site_instance3->IsDefaultSiteInstance());
-    // TODO(acolwell: Remove once CreateForServiceWorker() can return a
-    // default SiteInstance.
-    EXPECT_NE(sw_host1, sw_host3);
-  } else {
-    EXPECT_EQ(sw_host1, sw_host3);
-  }
+  EXPECT_EQ(sw_host1, sw_host3);
 
   // Getting a RenderProcessHost for a navigation to the same site again with
   // process-per-site flag should reuse the unmatched service worker's process.
   scoped_refptr<SiteInstanceImpl> sw_site_instance4 =
       SiteInstanceImpl::CreateForURL(browser_context(), kUrl);
   RenderProcessHost* sw_host4 = sw_site_instance4->GetProcess();
-  if (AreDefaultSiteInstancesEnabled()) {
-    EXPECT_TRUE(sw_site_instance4->IsDefaultSiteInstance());
-    // TODO(acolwell: Remove once CreateForServiceWorker() can return a
-    // default SiteInstance.
-    EXPECT_NE(sw_host1, sw_host4);
-  } else {
-    EXPECT_EQ(sw_host1, sw_host4);
-  }
+  EXPECT_EQ(sw_host1, sw_host4);
 }
 
 TEST_F(RenderProcessHostUnitTest, DoNotReuseOtherSiteServiceWorkerProcess) {

content/browser/site_instance_impl.cc

@@ -194,10 +194,14 @@ RenderProcessHost* SiteInstanceImpl::GetDefaultProcessIfUsable() {
   return browsing_instance_->default_process();
 }
 
-bool SiteInstanceImpl::IsDefaultSiteInstance() {
+bool SiteInstanceImpl::IsDefaultSiteInstance() const {
   return browsing_instance_->IsDefaultSiteInstance(this);
 }
 
+bool SiteInstanceImpl::IsSiteInDefaultSiteInstance(const GURL& site_url) const {
+  return browsing_instance_->IsSiteInDefaultSiteInstance(site_url);
+}
+
 void SiteInstanceImpl::MaybeSetBrowsingInstanceDefaultProcess() {
   if (!base::FeatureList::IsEnabled(
           features::kProcessSharingWithStrictSiteInstances)) {

content/browser/site_instance_impl.h

@@ -307,7 +307,11 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance,
   RenderProcessHost* GetDefaultProcessIfUsable();
 
   // Returns true if this object was constructed as a default site instance.
-  bool IsDefaultSiteInstance();
+  bool IsDefaultSiteInstance() const;
+
+  // Returns true if |site_url| is a site URL that the BrowsingInstance has
+  // associated with its default SiteInstance.
+  bool IsSiteInDefaultSiteInstance(const GURL& site_url) const;
 
   // Returns true if the the site URL for |url| matches the site URL
   // for this instance (i.e. GetSiteURL()). Otherwise returns false.