diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc index 110ad0fb82e376c93a9a5364eb080b539ce6a6e2..666022635ccb5f79dfd8c7f39a60227bf0bb5406 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..79c2ffdc371c6b7a819ce5f66239430c11be8e6f 100644 --- a/content/browser/site_instance_impl.h +++ b/content/browser/site_instance_impl.h @@ -482,6 +482,12 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance { // Sets the process for `this`, creating a SiteInstanceGroup if necessary. void SetProcessForTesting(RenderProcessHost* process); + // 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 +631,9 @@ 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; + + // 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(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 site_instance_ = nullptr; + bool site_instance_deleted_ = false; + bool browsing_instance_deleted_ = false; + base::WeakPtrFactory 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 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 browser_context(new TestBrowserContext()); + SiteInstanceDestructionObserver observer2; { std::unique_ptr 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( + 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/BUILD.gn b/content/public/browser/BUILD.gn index 81bc334b42db76287feeec3f9edf7f6f12009b7b..4272f1c5389caab61be128bb005d3179f414b863 100644 --- a/content/public/browser/BUILD.gn +++ b/content/public/browser/BUILD.gn @@ -82,6 +82,7 @@ source_set("browser_sources") { "browser_main_runner.h", "browser_message_filter.cc", "browser_message_filter.h", + "browser_or_resource_context.cc", "browser_or_resource_context.h", "browser_plugin_guest_delegate.cc", "browser_plugin_guest_delegate.h", diff --git a/content/public/browser/browser_or_resource_context.cc b/content/public/browser/browser_or_resource_context.cc new file mode 100644 index 0000000000000000000000000000000000000000..1ab5b126a05758ef70dbc955d5efccb05fc773cd --- /dev/null +++ b/content/public/browser/browser_or_resource_context.cc @@ -0,0 +1,33 @@ +// Copyright 2023 The Chromium Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "content/public/browser/browser_or_resource_context.h" + +#include "base/check_deref.h" +#include "content/public/browser/browser_context.h" +#include "content/public/browser/resource_context.h" + +namespace content { + +BrowserOrResourceContext::BrowserOrResourceContext() = default; + +BrowserOrResourceContext::BrowserOrResourceContext( + BrowserContext* browser_context) + : storage_(raw_ref(CHECK_DEREF(browser_context))) { + DCHECK_CURRENTLY_ON(BrowserThread::UI); +} + +BrowserOrResourceContext::BrowserOrResourceContext( + ResourceContext* resource_context) + : storage_(raw_ref(CHECK_DEREF(resource_context))) { + DCHECK_CURRENTLY_ON(BrowserThread::IO); +} + +BrowserOrResourceContext::~BrowserOrResourceContext() = default; +BrowserOrResourceContext::BrowserOrResourceContext( + const BrowserOrResourceContext&) = default; +BrowserOrResourceContext& BrowserOrResourceContext::operator=( + const BrowserOrResourceContext&) = default; + +} // namespace content diff --git a/content/public/browser/browser_or_resource_context.h b/content/public/browser/browser_or_resource_context.h index b4510741dff278ad5413c31b607e2d21d8a3cf1b..04a9d031cbc6f310020dc9e1bc37b970701b77ca 100644 --- a/content/public/browser/browser_or_resource_context.h +++ b/content/public/browser/browser_or_resource_context.h @@ -6,99 +6,69 @@ #define CONTENT_PUBLIC_BROWSER_BROWSER_OR_RESOURCE_CONTEXT_H_ #include -#include -#include "base/check_op.h" -#include "base/memory/raw_ptr_exclusion.h" +#include "base/memory/raw_ref.h" +#include "content/common/content_export.h" #include "content/public/browser/browser_thread.h" +#include "third_party/abseil-cpp/absl/types/variant.h" namespace content { class BrowserContext; class ResourceContext; -// A class holding either a BrowserContext* or a ResourceContext*. -// This class should hold a BrowserContext* when constructed on the UI thread -// and a ResourceContext* when constructed on the IO thread. This object must -// only be accessed on the thread it was constructed and does not allow -// converting between the two pointer types. -class BrowserOrResourceContext final { +// A class holding either BrowserContext*, a ResourceContext*, or nothing. This +// class should hold a BrowserContext* when constructed on the UI thread and a +// ResourceContext* when constructed on the IO thread. This object must only be +// accessed on the thread it was constructed and does not allow converting +// between the two pointer types. +class CONTENT_EXPORT BrowserOrResourceContext final { public: - BrowserOrResourceContext() { - union_.browser_context_ = nullptr; - flavour_ = kNullFlavour; - } + BrowserOrResourceContext(); + ~BrowserOrResourceContext(); - // BrowserOrResourceContext is implicitly constructible from either - // BrowserContext* or ResourceContext*. Neither of the constructor arguments - // can be null (enforced by DCHECKs and in some cases at compile time). - explicit BrowserOrResourceContext(BrowserContext* browser_context) { - DCHECK(browser_context); - DCHECK_CURRENTLY_ON(BrowserThread::UI); - union_.browser_context_ = browser_context; - flavour_ = kBrowserContextFlavour; - } + BrowserOrResourceContext(const BrowserOrResourceContext&); + BrowserOrResourceContext& operator=(const BrowserOrResourceContext&); - explicit BrowserOrResourceContext(ResourceContext* resource_context) { - DCHECK(resource_context); - DCHECK_CURRENTLY_ON(BrowserThread::IO); - union_.resource_context_ = resource_context; - flavour_ = kResourceContextFlavour; - } - BrowserOrResourceContext(std::nullptr_t) = delete; + // BrowserOrResourceContext is constructible from either BrowserContext* or + // ResourceContext*. + // TODO(dcheng): Change this to take a ref. + explicit BrowserOrResourceContext(BrowserContext* browser_context); - // BrowserOrResourceContext has a trivial, default destructor. - ~BrowserOrResourceContext() = default; + // TODO(dcheng): Change this to take a ref. + explicit BrowserOrResourceContext(ResourceContext* resource_context); - // BrowserOrResourceContext is trivially copyable. - BrowserOrResourceContext(const BrowserOrResourceContext& other) = default; - BrowserOrResourceContext& operator=(const BrowserOrResourceContext& other) = - default; + BrowserOrResourceContext(std::nullptr_t) = delete; + // Returns true if `this` is not null. explicit operator bool() const { - return (union_.resource_context_ != nullptr && - union_.browser_context_ != nullptr); + return !absl::holds_alternative(storage_); } - // To be called only on the UI thread. In DCHECK-enabled builds will verify - // that this object has kBrowserContextFlavour (implying that the returned - // BrowserContext* is valid and non-null. + // To be called only on the UI thread. Will CHECK() if `this` does not hold a + // `BrowserContext*`. + // TODO(dcheng): Change this to return a ref. BrowserContext* ToBrowserContext() const { DCHECK_CURRENTLY_ON(BrowserThread::UI); - CHECK_EQ(kBrowserContextFlavour, flavour_); - return union_.browser_context_; + return &*absl::get>(storage_); } - // To be called only on the IO thread. In DCHECK-enabled builds will verify - // that this object has kResourceContextFlavour (implying that the returned - // ResourceContext* is valid and non-null. + // To be called only on the UI thread. Will CHECK() if `this` does not hold a + // `ResourceContext*`. + // TODO(dcheng): Change this to return a ref. ResourceContext* ToResourceContext() const { DCHECK_CURRENTLY_ON(BrowserThread::IO); - CHECK_EQ(kResourceContextFlavour, flavour_); - return union_.resource_context_; + return &*absl::get>(storage_); } private: - union Union { - // This field is not a raw_ptr<> because it was filtered by the rewriter - // for: #union - RAW_PTR_EXCLUSION BrowserContext* browser_context_; - // This field is not a raw_ptr<> because it was filtered by the rewriter - // for: #union - RAW_PTR_EXCLUSION ResourceContext* resource_context_; - } union_; - - enum Flavour { - kNullFlavour, - kBrowserContextFlavour, - kResourceContextFlavour, - } flavour_; + // `absl::monostate` corresponds to the null state. + absl::variant, + raw_ref> + storage_; }; -static_assert( - std::is_trivially_copyable::value, - "BrowserOrResourceContext should be trivially copyable in release builds."); - } // namespace content #endif // CONTENT_PUBLIC_BROWSER_BROWSER_OR_RESOURCE_CONTEXT_H_ diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h index ea78cafbd90b8dcf5b40e6dc360de39edd8dd7f9..8d637ccae19d48c9532995ac0a0860b571a94ded 100644 --- a/content/public/browser/content_browser_client.h +++ b/content/public/browser/content_browser_client.h @@ -611,9 +611,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 diff --git a/extensions/browser/api/web_request/web_request_permissions_unittest.cc b/extensions/browser/api/web_request/web_request_permissions_unittest.cc index bf9846abc9357b0376f69c53f643ab80128baf4d..49fa9e2aab7b808e84c92f031f5bc83a63a16069 100644 --- a/extensions/browser/api/web_request/web_request_permissions_unittest.cc +++ b/extensions/browser/api/web_request/web_request_permissions_unittest.cc @@ -205,10 +205,8 @@ TEST_F(ExtensionWebRequestPermissionsTest, TestHideRequestForURL) { // If the origin is labeled by the WebStoreAppId, it becomes protected. { const int kWebstoreProcessId = 42; - const content::SiteInstanceId kSiteInstanceId(23); ProcessMap::Get(browser_context()) - ->Insert(extensions::kWebStoreAppId, kWebstoreProcessId, - kSiteInstanceId); + ->Insert(extensions::kWebStoreAppId, kWebstoreProcessId); WebRequestInfo sensitive_request_info(create_request_params( non_sensitive_url, WebRequestResourceType::SCRIPT, kWebstoreProcessId)); EXPECT_TRUE(WebRequestPermissions::HideRequest(permission_helper, diff --git a/extensions/browser/process_map.cc b/extensions/browser/process_map.cc index 472bad1be5824b03f24403b7b3f32571d574dabb..71571c8eb1fc2d6bdebe757dde9aad77dc7a5676 100644 --- a/extensions/browser/process_map.cc +++ b/extensions/browser/process_map.cc @@ -46,12 +46,8 @@ bool IsWebViewProcessForExtension(int process_id, // Item struct ProcessMap::Item { - Item(const std::string& extension_id, - int process_id, - content::SiteInstanceId site_instance_id) - : extension_id(extension_id), - process_id(process_id), - site_instance_id(site_instance_id) {} + Item(const std::string& extension_id, int process_id) + : extension_id(extension_id), process_id(process_id) {} Item(const Item&) = delete; Item& operator=(const Item&) = delete; @@ -62,14 +58,12 @@ struct ProcessMap::Item { Item& operator=(ProcessMap::Item&&) = default; bool operator<(const ProcessMap::Item& other) const { - return std::tie(extension_id, process_id, site_instance_id) < - std::tie(other.extension_id, other.process_id, - other.site_instance_id); + return std::tie(extension_id, process_id) < + std::tie(other.extension_id, other.process_id); } std::string extension_id; int process_id = 0; - content::SiteInstanceId site_instance_id; }; @@ -83,16 +77,8 @@ ProcessMap* ProcessMap::Get(content::BrowserContext* browser_context) { return ProcessMapFactory::GetForBrowserContext(browser_context); } -bool ProcessMap::Insert(const std::string& extension_id, - int process_id, - content::SiteInstanceId site_instance_id) { - return items_.insert(Item(extension_id, process_id, site_instance_id)).second; -} - -bool ProcessMap::Remove(const std::string& extension_id, - int process_id, - content::SiteInstanceId site_instance_id) { - return items_.erase(Item(extension_id, process_id, site_instance_id)) > 0; +bool ProcessMap::Insert(const std::string& extension_id, int process_id) { + return items_.insert(Item(extension_id, process_id)).second; } int ProcessMap::RemoveAllFromProcess(int process_id) { diff --git a/extensions/browser/process_map.h b/extensions/browser/process_map.h index 4e4074dc15d26b42d52ead9086f64a8a2edee7eb..40d565a16ae0f8cbea5fc8632810530359d8905c 100644 --- a/extensions/browser/process_map.h +++ b/extensions/browser/process_map.h @@ -90,13 +90,8 @@ class ProcessMap : public KeyedService { size_t size() const { return items_.size(); } - bool Insert(const std::string& extension_id, - int process_id, - content::SiteInstanceId site_instance_id); + bool Insert(const std::string& extension_id, int process_id); - bool Remove(const std::string& extension_id, - int process_id, - content::SiteInstanceId site_instance_id); int RemoveAllFromProcess(int process_id); bool Contains(const std::string& extension_id, int process_id) const; diff --git a/extensions/browser/process_map_unittest.cc b/extensions/browser/process_map_unittest.cc index b2a68705c8922690dfb875fe9d8642fd32761fee..bdeeaa63a66deb8324f8a3ceaae5d4f2931f0ab5 100644 --- a/extensions/browser/process_map_unittest.cc +++ b/extensions/browser/process_map_unittest.cc @@ -62,55 +62,56 @@ TEST(ExtensionProcessMapTest, Test) { // Test behavior when empty. EXPECT_FALSE(map.Contains("a", 1)); - EXPECT_FALSE(map.Remove("a", 1, content::SiteInstanceId(1))); + EXPECT_FALSE(map.RemoveAllFromProcess(1)); EXPECT_EQ(0u, map.size()); // Test insertion and behavior with one item. - EXPECT_TRUE(map.Insert("a", 1, content::SiteInstanceId(1))); + EXPECT_TRUE(map.Insert("a", 1)); EXPECT_TRUE(map.Contains("a", 1)); EXPECT_FALSE(map.Contains("a", 2)); EXPECT_FALSE(map.Contains("b", 1)); EXPECT_EQ(1u, map.size()); // Test inserting a duplicate item. - EXPECT_FALSE(map.Insert("a", 1, content::SiteInstanceId(1))); + EXPECT_FALSE(map.Insert("a", 1)); EXPECT_TRUE(map.Contains("a", 1)); EXPECT_EQ(1u, map.size()); // Insert some more items. - EXPECT_TRUE(map.Insert("a", 2, content::SiteInstanceId(2))); - EXPECT_TRUE(map.Insert("b", 1, content::SiteInstanceId(3))); - EXPECT_TRUE(map.Insert("b", 2, content::SiteInstanceId(4))); + EXPECT_TRUE(map.Insert("a", 2)); + EXPECT_TRUE(map.Insert("b", 3)); + EXPECT_TRUE(map.Insert("b", 4)); EXPECT_EQ(4u, map.size()); EXPECT_TRUE(map.Contains("a", 1)); EXPECT_TRUE(map.Contains("a", 2)); - EXPECT_TRUE(map.Contains("b", 1)); - EXPECT_TRUE(map.Contains("b", 2)); - EXPECT_FALSE(map.Contains("a", 3)); - - // Note that this only differs from an existing item because of the site - // instance id. - EXPECT_TRUE(map.Insert("a", 1, content::SiteInstanceId(5))); - EXPECT_TRUE(map.Contains("a", 1)); + EXPECT_TRUE(map.Contains("b", 3)); + EXPECT_TRUE(map.Contains("b", 4)); - // Test removal. - EXPECT_TRUE(map.Remove("a", 1, content::SiteInstanceId(1))); - EXPECT_FALSE(map.Remove("a", 1, content::SiteInstanceId(1))); - EXPECT_EQ(4u, map.size()); - - // Should still return true because there were two site instances for this - // extension/process pair. - EXPECT_TRUE(map.Contains("a", 1)); + EXPECT_FALSE(map.Contains("a", 3)); + EXPECT_FALSE(map.Contains("b", 2)); + EXPECT_FALSE(map.Contains("a", 5)); + EXPECT_FALSE(map.Contains("c", 3)); - EXPECT_TRUE(map.Remove("a", 1, content::SiteInstanceId(5))); + // At this point we have {a,1}, {a,2}, {b,3}, and {b,4} in the map. Test + // removal of these processes. + EXPECT_EQ(1, map.RemoveAllFromProcess(1)); EXPECT_EQ(3u, map.size()); EXPECT_FALSE(map.Contains("a", 1)); + EXPECT_TRUE(map.Contains("a", 2)); - EXPECT_EQ(2, map.RemoveAllFromProcess(2)); - EXPECT_EQ(1u, map.size()); + EXPECT_EQ(1, map.RemoveAllFromProcess(2)); + EXPECT_EQ(2u, map.size()); EXPECT_EQ(0, map.RemoveAllFromProcess(2)); + EXPECT_EQ(2u, map.size()); + EXPECT_EQ(1, map.RemoveAllFromProcess(3)); EXPECT_EQ(1u, map.size()); + EXPECT_EQ(0, map.RemoveAllFromProcess(3)); + EXPECT_EQ(1u, map.size()); + EXPECT_EQ(1, map.RemoveAllFromProcess(4)); + EXPECT_EQ(0u, map.size()); + EXPECT_EQ(0, map.RemoveAllFromProcess(4)); + EXPECT_EQ(0u, map.size()); } TEST(ExtensionProcessMapTest, GetMostLikelyContextType) { @@ -135,13 +136,13 @@ TEST(ExtensionProcessMapTest, GetMostLikelyContextType) { extensions::Feature::CONTENT_SCRIPT_CONTEXT, map.GetMostLikelyContextType(extension.get(), 2, &untrusted_webui_url)); - map.Insert("b", 3, content::SiteInstanceId(1)); + map.Insert("b", 3); extension = CreateExtensionWithFlags(extensions::TypeToCreate::kExtension, "b"); EXPECT_EQ(extensions::Feature::BLESSED_EXTENSION_CONTEXT, map.GetMostLikelyContextType(extension.get(), 3, &extension_url)); - map.Insert("c", 4, content::SiteInstanceId(2)); + map.Insert("c", 4); extension = CreateExtensionWithFlags(extensions::TypeToCreate::kPlatformApp, "c"); EXPECT_EQ(extensions::Feature::BLESSED_EXTENSION_CONTEXT, @@ -149,30 +150,30 @@ TEST(ExtensionProcessMapTest, GetMostLikelyContextType) { map.set_is_lock_screen_context(true); - map.Insert("d", 5, content::SiteInstanceId(3)); + map.Insert("d", 5); extension = CreateExtensionWithFlags(extensions::TypeToCreate::kPlatformApp, "d"); EXPECT_EQ(extensions::Feature::LOCK_SCREEN_EXTENSION_CONTEXT, map.GetMostLikelyContextType(extension.get(), 5, &extension_url)); - map.Insert("e", 6, content::SiteInstanceId(4)); + map.Insert("e", 6); extension = CreateExtensionWithFlags(extensions::TypeToCreate::kExtension, "e"); EXPECT_EQ(extensions::Feature::LOCK_SCREEN_EXTENSION_CONTEXT, map.GetMostLikelyContextType(extension.get(), 6, &extension_url)); - map.Insert("f", 7, content::SiteInstanceId(5)); + map.Insert("f", 7); extension = CreateExtensionWithFlags(extensions::TypeToCreate::kHostedApp, "f"); EXPECT_EQ(extensions::Feature::BLESSED_WEB_PAGE_CONTEXT, map.GetMostLikelyContextType(extension.get(), 7, &web_url)); - map.Insert("g", 8, content::SiteInstanceId(6)); + map.Insert("g", 8); EXPECT_EQ(extensions::Feature::WEBUI_UNTRUSTED_CONTEXT, map.GetMostLikelyContextType(/*extension=*/nullptr, 8, &untrusted_webui_url)); - map.Insert("h", 9, content::SiteInstanceId(7)); + map.Insert("h", 9); EXPECT_EQ(extensions::Feature::WEB_PAGE_CONTEXT, map.GetMostLikelyContextType(/*extension=*/nullptr, 9, &web_url)); } diff --git a/extensions/shell/browser/shell_content_browser_client.cc b/extensions/shell/browser/shell_content_browser_client.cc index 3e7111150b0f9f1217073b53da72917af503cfb9..25e946d08800c518819ec1776e40e77853cbbb48 100644 --- a/extensions/shell/browser/shell_content_browser_client.cc +++ b/extensions/shell/browser/shell_content_browser_client.cc @@ -163,26 +163,7 @@ void ShellContentBrowserClient::SiteInstanceGotProcess( return; ProcessMap::Get(browser_main_parts_->browser_context()) - ->Insert(extension->id(), - site_instance->GetProcess()->GetID(), - site_instance->GetId()); -} - -void ShellContentBrowserClient::SiteInstanceDeleting( - content::SiteInstance* site_instance) { - // Don't do anything if we're shutting down. - if (content::BrowserMainRunner::ExitedMainMessageLoop()) - return; - - // If this isn't an extension renderer there's nothing to do. - const Extension* extension = GetExtension(site_instance); - if (!extension) - return; - - ProcessMap::Get(browser_main_parts_->browser_context()) - ->Remove(extension->id(), - site_instance->GetProcess()->GetID(), - site_instance->GetId()); + ->Insert(extension->id(), site_instance->GetProcess()->GetID()); } void ShellContentBrowserClient::AppendExtraCommandLineSwitches( diff --git a/extensions/shell/browser/shell_content_browser_client.h b/extensions/shell/browser/shell_content_browser_client.h index 79830e44c9fa719db173d2928b79e782acd82c49..2777d0f6c0aab2bc99883fb39c8a3b406cdad85b 100644 --- a/extensions/shell/browser/shell_content_browser_client.h +++ b/extensions/shell/browser/shell_content_browser_client.h @@ -66,7 +66,6 @@ class ShellContentBrowserClient : public content::ContentBrowserClient { const GURL& site_url) override; bool IsHandledURL(const GURL& url) override; void SiteInstanceGotProcess(content::SiteInstance* site_instance) override; - void SiteInstanceDeleting(content::SiteInstance* site_instance) override; void AppendExtraCommandLineSwitches(base::CommandLine* command_line, int child_process_id) override; content::SpeechRecognitionManagerDelegate*