mirror
/
electron
mirror of https://github.com/electron/electron
1
0
Fork 0
Code Issues Releases Activity
electron/patches/chromium/cherry-pick-aa23556ff213.patch

283 lines
12 KiB
Diff
Raw Permalink Blame History

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Alex Moshchuk <alexmos@chromium.org>
Date: Tue, 13 Jun 2023 20:27:29 +0000
Subject: 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 f149f77b2a4f3416f8f7ea54a8fd972763c4b383)
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: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114}
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index 4404b3099182ff3513dbc61ee5d37272b8abe771..857462259a56f5cccd56210db40d742c49ae46d7 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -105,7 +105,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
diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h
index ea2d35e9b35acb94c6626c8ee24cf323ad0774ad..f7cc8a11c1be26b239a825e43f9ec6b703cff064 100644
--- a/content/browser/site_instance_impl.h
+++ b/content/browser/site_instance_impl.h
@@ -482,6 +482,28 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
// Sets the process for `this`, creating a SiteInstanceGroup if necessary.
void SetProcessForTesting(RenderProcessHost* process);
+ // Returns the number of active documents using `this`, potentially spanning
+ // multiple pages/WebContents, but still within the same BrowsingInstance.
+ size_t active_document_count() const { return active_document_count_; }
+
+ // Increments the active document count after a new document that uses `this`
+ // finishes committing and becomes active.
+ void IncrementActiveDocumentCount() { active_document_count_++; }
+
+ // Decrement the active document count after a previously document that uses
+ // `this` got swapped out and becomes inactive due to another document
+ // committing in the same frame.
+ void DecrementActiveDocumentCount() {
+ CHECK_GT(active_document_count_, 0u);
+ 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;
@@ -625,6 +647,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
// Keeps track of whether we need to verify that the StoragePartition
// information does not change when `site_info_` is set.
bool verify_storage_partition_info_ = false;
+
+ // 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
diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc
index c74e6897cc89ab76ee80b88c79cd401d931ae62a..cf6a1102e95f41d4f3ac130493d43c57e62df974 100644
--- a/content/browser/site_instance_impl_unittest.cc
+++ b/content/browser/site_instance_impl_unittest.cc
@@ -91,32 +91,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 {
@@ -193,6 +169,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_;
@@ -440,7 +455,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,
@@ -450,8 +466,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(
@@ -461,36 +477,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
}
@@ -521,7 +541,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(),
@@ -547,14 +566,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.
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 43890ccf69e8b0b7318a93f78b734f27fca17d89..6b978e7642c4e8c5f8aec2536f9bd02114d6279f 100644
--- a/content/public/browser/content_browser_client.h
+++ b/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
View Git Blame Copy Permalink