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

267 lines
11 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 3fd0545c6a9a49e064d54d3292187e78a4900040..8ba13311bdd825d6b07b941769af94f6e9002091 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 bea6b211ead2190960a3bf5a52ca888daa7c5797..28d03536c16aad7d4021e6a9efddd1bc06324ace 100644
--- a/content/browser/site_instance_impl.h
+++ b/content/browser/site_instance_impl.h
@@ -446,6 +446,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance {
// BrowsingInstance.
RenderProcessHost* GetDefaultProcessForBrowsingInstance();
+ // 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 SiteInstanceTestBrowserClient;
@@ -585,6 +591,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 73a689bb2680bd7d76cf580c1a085923c0e1e464..e89c0866d23d8a6353eed4a5d424113112f3cad7 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 543b1712a9b744598ec6d851bc658e5b7e86d39e..757b20c73826e291c8958cc2bfaded2a8f8cd576 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -582,9 +582,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