Fix default SiteInstance memory leak.

Removing reference cycle between BrowsingInstance and default
SiteInstance that was accidentally introduced in
http://crrev.com/c/1452714

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

content/browser/browsing_instance.cc

@@ -26,7 +26,8 @@ BrowsingInstance::BrowsingInstance(BrowserContext* browser_context)
           BrowsingInstanceId::FromUnsafeValue(next_browsing_instance_id_++),
           BrowserOrResourceContext(browser_context)),
       active_contents_count_(0u),
-      default_process_(nullptr) {
+      default_process_(nullptr),
+      default_site_instance_(nullptr) {
   DCHECK(browser_context);
 }
 
@@ -51,8 +52,7 @@ void BrowsingInstance::SetDefaultProcess(RenderProcessHost* default_process) {
 
 bool BrowsingInstance::IsDefaultSiteInstance(
     const SiteInstanceImpl* site_instance) const {
-  return site_instance != nullptr &&
-         site_instance == default_site_instance_.get();
+  return site_instance != nullptr && site_instance == default_site_instance_;
 }
 
 bool BrowsingInstance::HasSiteInstance(const GURL& url) {
@@ -117,11 +117,18 @@ scoped_refptr<SiteInstanceImpl> BrowsingInstance::GetSiteInstanceForURLHelper(
       !SiteInstanceImpl::DoesSiteRequireDedicatedProcess(isolation_context_,
                                                          url)) {
     DCHECK(!default_process_);
-    if (!default_site_instance_) {
-      default_site_instance_ = new SiteInstanceImpl(this);
-      default_site_instance_->SetSite(SiteInstanceImpl::GetDefaultSiteURL());
+    scoped_refptr<SiteInstanceImpl> site_instance = default_site_instance_;
+    if (!site_instance) {
+      site_instance = new SiteInstanceImpl(this);
+      site_instance->SetSite(SiteInstanceImpl::GetDefaultSiteURL());
+
+      // Keep a copy of the pointer so it can be used for other URLs. This is
+      // safe because the SiteInstanceImpl destructor will call
+      // UnregisterSiteInstance() to clear this copy when the last
+      // reference to |site_instance| is destroyed.
+      default_site_instance_ = site_instance.get();
     }
-    return default_site_instance_;
+    return site_instance;
   }
 
   return nullptr;
@@ -133,7 +140,7 @@ void BrowsingInstance::RegisterSiteInstance(SiteInstanceImpl* site_instance) {
 
   // Explicitly prevent the |default_site_instance_| from being added since
   // the map is only supposed to contain instances that map to a single site.
-  if (site_instance == default_site_instance_.get())
+  if (site_instance == default_site_instance_)
     return;
 
   std::string site = site_instance->GetSiteURL().possibly_invalid_spec();
@@ -153,6 +160,12 @@ void BrowsingInstance::RegisterSiteInstance(SiteInstanceImpl* site_instance) {
 void BrowsingInstance::UnregisterSiteInstance(SiteInstanceImpl* site_instance) {
   DCHECK(site_instance->browsing_instance_.get() == this);
   DCHECK(site_instance->HasSite());
+
+  if (site_instance == default_site_instance_) {
+    // The last reference to the default SiteInstance is being destroyed.
+    default_site_instance_ = nullptr;
+  }
+
   std::string site = site_instance->GetSiteURL().possibly_invalid_spec();
 
   // Only unregister the SiteInstance if it is the same one that is registered
@@ -175,6 +188,7 @@ BrowsingInstance::~BrowsingInstance() {
   // us are gone.
   DCHECK(site_instance_map_.empty());
   DCHECK_EQ(0u, active_contents_count_);
+  DCHECK(!default_site_instance_);
   if (default_process_)
     default_process_->RemoveObserver(this);
 }

content/browser/browsing_instance.h

@@ -203,8 +203,9 @@ class CONTENT_EXPORT BrowsingInstance final
   // |site_instance_map_| and it does not require a dedicated process.
   // This field and |default_process_| are mutually exclusive and this field
   // should only be set if kProcessSharingWithStrictSiteInstances is not
-  // enabled.
-  scoped_refptr<SiteInstanceImpl> default_site_instance_;
+  // enabled. This is a raw pointer to avoid a reference cycle between the
+  // BrowsingInstance and the SiteInstanceImpl.
+  SiteInstanceImpl* default_site_instance_;
 
   DISALLOW_COPY_AND_ASSIGN(BrowsingInstance);
 };

content/browser/site_instance_impl_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
 #include "base/test/mock_log.h"
+#include "base/test/scoped_command_line.h"
 #include "base/test/scoped_feature_list.h"
 #include "content/browser/browsing_instance.h"
 #include "content/browser/child_process_security_policy_impl.h"
@@ -238,6 +239,33 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
   EXPECT_EQ(1, browser_client()->GetAndClearSiteInstanceDeleteCount());
   EXPECT_EQ(1, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
   // contents is now deleted, along with instance and browsing_instance
+
+  // Ensure that default SiteInstances are deleted when all references to them
+  // are gone.
+  {
+    base::test::ScopedCommandLine scoped_command_line;
+
+    // Disable site isolation so we can get default SiteInstances on all
+    // platforms.
+    scoped_command_line.GetProcessCommandLine()->AppendSwitch(
+        switches::kDisableSiteIsolation);
+
+    auto site_instance1 = SiteInstanceImpl::CreateForURL(
+        browser_context.get(), GURL("http://foo.com"));
+    // TODO(958060): Remove the creation of this second instance and update
+    // the deletion count below once CreateForURL() starts returning a default
+    // SiteInstance for sites that don't require a dedicated process.
+    auto site_instance2 =
+        site_instance1->GetRelatedSiteInstance(GURL("http://bar.com"));
+    EXPECT_FALSE(site_instance1->IsDefaultSiteInstance());
+    EXPECT_TRUE(static_cast<SiteInstanceImpl*>(site_instance2.get())
+                    ->IsDefaultSiteInstance());
+    site_instance1.reset();
+    site_instance2.reset();
+
+    EXPECT_EQ(2, browser_client()->GetAndClearSiteInstanceDeleteCount());
+    EXPECT_EQ(1, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  }
 }
 
 // Test to ensure GetProcess returns and creates processes correctly.