[M115 merge] Remove SiteInstanceDeleting from content/public API.

Currently, two unit tests are relying on
ContentBrowserClient::SiteInstanceDeleting() to ensure that
SiteInstance lifetimes are managed properly.  However, it is
undesirable to allow //content embedders to run arbitrary code during
SiteInstance destruction, per the linked bug.  Hence, this CL converts
SiteInstanceDeleting() usage to a test-specific callback instead and
removes it from ContentBrowserClient.

(cherry picked from commit f149f77)

Bug: 1444438
Change-Id: I2affcb4a40ccfbf3bd715d7b225976a687405616
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4564316
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149171}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4610071
Cr-Commit-Position: refs/branch-heads/5790@{#710}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

content/browser/site_instance_impl.cc

@@ -104,7 +104,9 @@ SiteInstanceImpl::SiteInstanceImpl(BrowsingInstance* browsing_instance)
 }
 
 SiteInstanceImpl::~SiteInstanceImpl() {
-  GetContentClient()->browser()->SiteInstanceDeleting(this);
+  if (destruction_callback_for_testing_) {
+    std::move(destruction_callback_for_testing_).Run();
+  }
 
   // Now that no one is referencing us, we can safely remove ourselves from
   // the BrowsingInstance.  Any future visits to a page from this site

content/browser/site_instance_impl.h

@@ -512,6 +512,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
     active_document_count_--;
   }
 
+  // Set a callback to be run from this SiteInstance's destructor. Used only in
+  // tests.
+  void set_destruction_callback_for_testing(base::OnceClosure callback) {
+    destruction_callback_for_testing_ = std::move(callback);
+  }
+
  private:
   friend class BrowsingInstance;
   friend class SiteInstanceGroupManager;
@@ -658,6 +664,9 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
 
   // Tracks the number of active documents currently in this SiteInstance.
   size_t active_document_count_ = 0;
+
+  // Test-only callback to run when this SiteInstance is destroyed.
+  base::OnceClosure destruction_callback_for_testing_;
 };
 
 }  // namespace content

content/browser/site_instance_impl_unittest.cc

@@ -94,32 +94,8 @@ class SiteInstanceTestBrowserClient : public TestContentBrowserClient {
     privileged_process_id_ = process_id;
   }
 
-  void SiteInstanceDeleting(content::SiteInstance* site_instance) override {
-    site_instance_delete_count_++;
-    // Infer deletion of the browsing instance.
-    if (static_cast<SiteInstanceImpl*>(site_instance)
-            ->browsing_instance_->HasOneRef()) {
-      browsing_instance_delete_count_++;
-    }
-  }
-
-  int GetAndClearSiteInstanceDeleteCount() {
-    int result = site_instance_delete_count_;
-    site_instance_delete_count_ = 0;
-    return result;
-  }
-
-  int GetAndClearBrowsingInstanceDeleteCount() {
-    int result = browsing_instance_delete_count_;
-    browsing_instance_delete_count_ = 0;
-    return result;
-  }
-
  private:
   int privileged_process_id_ = -1;
-
-  int site_instance_delete_count_ = 0;
-  int browsing_instance_delete_count_ = 0;
 };
 
 class SiteInstanceTest : public testing::Test {
@@ -208,6 +184,45 @@ class SiteInstanceTest : public testing::Test {
                                         /*should_compare_effective_urls=*/true);
   }
 
+  // Helper class to watch whether a particular SiteInstance has been
+  // destroyed.
+  class SiteInstanceDestructionObserver {
+   public:
+    SiteInstanceDestructionObserver() = default;
+
+    explicit SiteInstanceDestructionObserver(SiteInstanceImpl* site_instance) {
+      SetSiteInstance(site_instance);
+    }
+
+    void SetSiteInstance(SiteInstanceImpl* site_instance) {
+      site_instance_ = site_instance;
+      site_instance_->set_destruction_callback_for_testing(
+          base::BindOnce(&SiteInstanceDestructionObserver::SiteInstanceDeleting,
+                         weak_factory_.GetWeakPtr()));
+    }
+
+    void SiteInstanceDeleting() {
+      ASSERT_FALSE(site_instance_deleted_);
+      ASSERT_FALSE(browsing_instance_deleted_);
+
+      site_instance_deleted_ = true;
+      // Infer deletion of the BrowsingInstance.
+      if (site_instance_->browsing_instance_->HasOneRef()) {
+        browsing_instance_deleted_ = true;
+      }
+      site_instance_ = nullptr;
+    }
+
+    bool site_instance_deleted() { return site_instance_deleted_; }
+    bool browsing_instance_deleted() { return browsing_instance_deleted_; }
+
+   private:
+    raw_ptr<SiteInstanceImpl> site_instance_ = nullptr;
+    bool site_instance_deleted_ = false;
+    bool browsing_instance_deleted_ = false;
+    base::WeakPtrFactory<SiteInstanceDestructionObserver> weak_factory_{this};
+  };
+
  private:
   BrowserTaskEnvironment task_environment_;
   TestBrowserContext context_;
@@ -459,7 +474,8 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
 
   // Ensure that instances are deleted when their NavigationEntries are gone.
   scoped_refptr<SiteInstanceImpl> instance = SiteInstanceImpl::Create(&context);
-  EXPECT_EQ(0, browser_client()->GetAndClearSiteInstanceDeleteCount());
+  SiteInstanceDestructionObserver observer(instance.get());
+  EXPECT_FALSE(observer.site_instance_deleted());
 
   NavigationEntryImpl* e1 = new NavigationEntryImpl(
       instance, url, Referrer(), /* initiator_origin= */ absl::nullopt,
@@ -469,8 +485,8 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
 
   // Redundantly setting e1's SiteInstance shouldn't affect the ref count.
   e1->set_site_instance(instance);
-  EXPECT_EQ(0, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(0, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  EXPECT_FALSE(observer.site_instance_deleted());
+  EXPECT_FALSE(observer.browsing_instance_deleted());
 
   // Add a second reference
   NavigationEntryImpl* e2 = new NavigationEntryImpl(
@@ -480,36 +496,40 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
       false /* is_initial_entry */);
 
   instance = nullptr;
-  EXPECT_EQ(0, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(0, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+
+  EXPECT_FALSE(observer.site_instance_deleted());
+  EXPECT_FALSE(observer.browsing_instance_deleted());
 
   // Now delete both entries and be sure the SiteInstance goes away.
   delete e1;
-  EXPECT_EQ(0, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(0, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  EXPECT_FALSE(observer.site_instance_deleted());
+  EXPECT_FALSE(observer.browsing_instance_deleted());
   delete e2;
   // instance is now deleted
-  EXPECT_EQ(1, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(1, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  EXPECT_TRUE(observer.site_instance_deleted());
+  EXPECT_TRUE(observer.browsing_instance_deleted());
   // browsing_instance is now deleted
 
-  // Ensure that instances are deleted when their RenderViewHosts are gone.
+  // Ensure that instances are deleted when their RenderFrameHosts are gone.
   std::unique_ptr<TestBrowserContext> browser_context(new TestBrowserContext());
+  SiteInstanceDestructionObserver observer2;
   {
     std::unique_ptr<WebContents> web_contents(
         WebContents::Create(WebContents::CreateParams(
             browser_context.get(),
             SiteInstance::Create(browser_context.get()))));
-    EXPECT_EQ(0, browser_client()->GetAndClearSiteInstanceDeleteCount());
-    EXPECT_EQ(0, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+    observer2.SetSiteInstance(static_cast<SiteInstanceImpl*>(
+        web_contents->GetPrimaryMainFrame()->GetSiteInstance()));
+    EXPECT_FALSE(observer2.site_instance_deleted());
+    EXPECT_FALSE(observer2.browsing_instance_deleted());
   }
 
   // Make sure that we flush any messages related to the above WebContentsImpl
   // destruction.
   DrainMessageLoop();
 
-  EXPECT_EQ(1, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(1, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  EXPECT_TRUE(observer2.site_instance_deleted());
+  EXPECT_TRUE(observer2.browsing_instance_deleted());
   // contents is now deleted, along with instance and browsing_instance
 }
 
@@ -540,7 +560,6 @@ TEST_F(SiteInstanceTest, DefaultSiteInstanceProperties) {
 
   auto site_instance = SiteInstanceImpl::CreateForTesting(
       &browser_context, GURL("http://foo.com"));
-
   EXPECT_TRUE(site_instance->IsDefaultSiteInstance());
   EXPECT_TRUE(site_instance->HasSite());
   EXPECT_EQ(site_instance->GetSiteInfo(),
@@ -566,14 +585,15 @@ TEST_F(SiteInstanceTest, DefaultSiteInstanceDestruction) {
   // are gone.
   auto site_instance = SiteInstanceImpl::CreateForTesting(
       &browser_context, GURL("http://foo.com"));
+  SiteInstanceDestructionObserver observer(site_instance.get());
 
   EXPECT_EQ(AreDefaultSiteInstancesEnabled(),
             site_instance->IsDefaultSiteInstance());
 
   site_instance.reset();
 
-  EXPECT_EQ(1, browser_client()->GetAndClearSiteInstanceDeleteCount());
-  EXPECT_EQ(1, browser_client()->GetAndClearBrowsingInstanceDeleteCount());
+  EXPECT_TRUE(observer.site_instance_deleted());
+  EXPECT_TRUE(observer.browsing_instance_deleted());
 }
 
 // Test to ensure GetProcess returns and creates processes correctly.

content/public/browser/content_browser_client.h

@@ -608,9 +608,6 @@ class CONTENT_EXPORT ContentBrowserClient {
   // Called when a site instance is first associated with a process.
   virtual void SiteInstanceGotProcess(SiteInstance* site_instance) {}
 
-  // Called from a site instance's destructor.
-  virtual void SiteInstanceDeleting(SiteInstance* site_instance) {}
-
   // Returns true if for the navigation from |current_effective_url| to
   // |destination_effective_url| in |site_instance|, a new SiteInstance and
   // BrowsingInstance should be created (even if we are in a process model that