Commit 5f364bfb authored by Lukasz Anforowicz's avatar Lukasz Anforowicz Committed by Commit Bot

Destroy [Cast]ExtensionURLLoaderFactory in OnBrowserContextDestroyed.

ExtensionURLLoaderFactory's and CastExtensionURLLoaderFactory's lifetime
cannot exceed the lifetime of the associated BrowserContext.

This CL should fix https://crbug.com/1136214 and is likely to also fix
https://crbug.com/1134050.

Fixed: 1136214
Bug: 1134050
Change-Id: I06b0c17ebe308ec86f0341c7fa7b7bbdc1ffde4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461222
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarSean Topping <seantopping@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815871}
parent 6f4a6681
......@@ -54,10 +54,8 @@
#include "content/public/browser/storage_partition.h"
#include "content/public/test/browser_test.h"
#include "content/public/test/test_utils.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/value_builder.h"
#include "extensions/buildflags/buildflags.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "net/base/load_flags.h"
#include "net/base/net_errors.h"
#include "net/dns/mock_host_resolver.h"
......@@ -76,6 +74,14 @@
#include "chromeos/constants/chromeos_switches.h"
#endif
#if BUILDFLAG(ENABLE_EXTENSIONS)
#include "extensions/browser/extension_protocols.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/value_builder.h"
#endif
namespace {
// A helper class which creates a SimpleURLLoader with an expected final status
......@@ -143,17 +149,21 @@ class ProfileDestructionWatcher : public ProfileObserver {
void Watch(Profile* profile) { observed_profiles_.Add(profile); }
bool destroyed() const { return destroyed_; }
void WaitForDestruction() { run_loop_.Run(); }
private:
// ProfileObserver:
void OnProfileWillBeDestroyed(Profile* profile) override {
DCHECK(!destroyed_) << "Double profile destruction";
destroyed_ = true;
run_loop_.Quit();
observed_profiles_.Remove(profile);
}
bool destroyed() const { return destroyed_; }
private:
bool destroyed_ = false;
base::RunLoop run_loop_;
ScopedObserver<Profile, ProfileObserver> observed_profiles_{this};
DISALLOW_COPY_AND_ASSIGN(ProfileDestructionWatcher);
......@@ -583,7 +593,6 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
// The following tests make sure that it's safe to shut down while one of the
// Profile's URLLoaderFactories is in use by a SimpleURLLoader.
IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
SimpleURLLoaderUsingMainContextDuringShutdown) {
ASSERT_TRUE(embedded_test_server()->Start());
......@@ -595,7 +604,6 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
// The following tests make sure that it's safe to destroy an incognito profile
// while one of the its URLLoaderFactory is in use by a SimpleURLLoader.
IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
SimpleURLLoaderUsingMainContextDuringIncognitoTeardown) {
ASSERT_TRUE(embedded_test_server()->Start());
......@@ -609,7 +617,64 @@ IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
.get());
}
// Verifies the cache directory supports multiple profiles when it's overriden
#if BUILDFLAG(ENABLE_EXTENSIONS)
// Regression test for https://crbug.com/1136214 - verification that
// ExtensionURLLoaderFactory won't hit a use-after-free bug when used after
// a Profile has been torn down already.
IN_PROC_BROWSER_TEST_F(ProfileBrowserTest,
ExtensionURLLoaderFactoryAfterIncognitoTeardown) {
// Create a mojo::Remote to ExtensionURLLoaderFactory for the incognito
// profile.
Browser* incognito_browser =
OpenURLOffTheRecord(browser()->profile(), GURL("about:blank"));
Profile* incognito_profile = incognito_browser->profile();
mojo::Remote<network::mojom::URLLoaderFactory> url_loader_factory;
url_loader_factory.Bind(extensions::CreateExtensionNavigationURLLoaderFactory(
incognito_profile, base::kInvalidUkmSourceId,
false /* is_web_view_request */));
// Verify that the factory works fine while the profile is still alive.
// We don't need to test with a real extension URL - it is sufficient to
// verify that the factory responds with ERR_BLOCKED_BY_CLIENT that indicates
// a missing extension.
GURL missing_extension_url("chrome-extension://no-such-extension/blah");
{
SimpleURLLoaderHelper simple_loader_helper(url_loader_factory.get(),
missing_extension_url,
net::ERR_BLOCKED_BY_CLIENT);
simple_loader_helper.WaitForCompletion();
}
{
// Start monitoring |incognito_profile| for shutdown.
ProfileManager* profile_manager = g_browser_process->profile_manager();
EXPECT_TRUE(profile_manager->IsValidProfile(incognito_profile));
ProfileDestructionWatcher watcher;
watcher.Watch(incognito_profile);
// Close all incognito tabs, starting profile shutdown.
incognito_browser->tab_strip_model()->CloseAllTabs();
// ProfileDestructionWatcher waits for
// BrowserContext::NotifyWillBeDestroyed, but after the RunLoop unwinds, the
// profile should already be gone - let's assert this below (since this
// ensures that |simple_loader_helper2| really tests what needs to be
// tested).
watcher.WaitForDestruction();
EXPECT_FALSE(profile_manager->IsValidProfile(incognito_profile));
}
// Verify that the factory doesn't crash (https://crbug.com/1136214), but
// instead SimpleURLLoaderImpl::OnMojoDisconnect reports net::ERR_FAILED.
{
SimpleURLLoaderHelper simple_loader_helper2(
url_loader_factory.get(), missing_extension_url, net::ERR_FAILED);
simple_loader_helper2.WaitForCompletion();
}
}
#endif
// Verifies the cache directory supports multiple profiles when it's overridden
// by group policy or command line switches.
IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, DiskCacheDirOverride) {
base::ScopedAllowBlockingForTesting allow_blocking;
......
......@@ -37,6 +37,7 @@
#include "chromecast/browser/cast_browser_context.h"
#include "chromecast/browser/cast_browser_process.h"
#include "chromecast/browser/cast_content_browser_client.h"
#include "chromecast/browser/cast_extension_url_loader_factory.h"
#include "chromecast/browser/cast_feature_list_creator.h"
#include "chromecast/browser/cast_system_memory_pressure_evaluator.h"
#include "chromecast/browser/cast_system_memory_pressure_evaluator_adjuster.h"
......@@ -384,6 +385,7 @@ void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
extensions::EnsureBrowserContextKeyedServiceFactoriesBuilt();
extensions::CastExtensionSystemFactory::GetInstance();
CastExtensionURLLoaderFactory::EnsureShutdownNotifierFactoryBuilt();
}
#endif
......
......@@ -10,11 +10,13 @@
#include <vector>
#include "base/strings/strcat.h"
#include "chromecast/browser/extensions/cast_extension_system_factory.h"
#include "chromecast/common/cast_redirect_manifest_handler.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/storage_partition.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_registry_factory.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "services/network/public/mojom/url_loader.mojom.h"
......@@ -181,6 +183,16 @@ CastExtensionURLLoaderFactory::CastExtensionURLLoaderFactory(
content::BrowserContext::GetDefaultStoragePartition(browser_context)
->GetURLLoaderFactoryForBrowserProcess()) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
// base::Unretained is safe below, because lifetime of
// |browser_context_shutdown_subscription_| guarantees that
// OnBrowserContextDestroyed won't be called after |this| is destroyed.
browser_context_shutdown_subscription_ =
BrowserContextShutdownNotifierFactory::GetInstance()
->Get(browser_context)
->Subscribe(base::BindRepeating(
&CastExtensionURLLoaderFactory::OnBrowserContextDestroyed,
base::Unretained(this)));
}
CastExtensionURLLoaderFactory::~CastExtensionURLLoaderFactory() = default;
......@@ -234,13 +246,43 @@ void CastExtensionURLLoaderFactory::CreateLoaderAndStart(
network_factory_);
}
void CastExtensionURLLoaderFactory::OnBrowserContextDestroyed() {
// When the BrowserContext gets destroyed, |this| factory is not able to serve
// any more requests.
DisconnectReceiversAndDestroy();
}
// static
CastExtensionURLLoaderFactory::BrowserContextShutdownNotifierFactory*
CastExtensionURLLoaderFactory::BrowserContextShutdownNotifierFactory::
GetInstance() {
static base::NoDestructor<BrowserContextShutdownNotifierFactory> s_factory;
return s_factory.get();
}
CastExtensionURLLoaderFactory::BrowserContextShutdownNotifierFactory::
BrowserContextShutdownNotifierFactory()
: BrowserContextKeyedServiceShutdownNotifierFactory(
"CastExtensionURLLoaderFactory::"
"BrowserContextShutdownNotifierFactory") {
DependsOn(extensions::ExtensionRegistryFactory::GetInstance());
DependsOn(extensions::CastExtensionSystemFactory::GetInstance());
}
// static
mojo::PendingRemote<network::mojom::URLLoaderFactory>
CastExtensionURLLoaderFactory::Create(
content::BrowserContext* browser_context,
mojo::PendingRemote<network::mojom::URLLoaderFactory> extension_factory) {
DCHECK(browser_context);
mojo::PendingRemote<network::mojom::URLLoaderFactory> pending_remote;
// Return an unbound |pending_remote| if the |browser_context| has already
// started shutting down.
if (browser_context->ShutdownStarted())
return pending_remote;
// The CastExtensionURLLoaderFactory will delete itself when there are no more
// receivers - see the NonNetworkURLLoaderFactoryBase::OnDisconnect method.
new CastExtensionURLLoaderFactory(
......@@ -250,5 +292,10 @@ CastExtensionURLLoaderFactory::Create(
return pending_remote;
}
// static
void CastExtensionURLLoaderFactory::EnsureShutdownNotifierFactoryBuilt() {
BrowserContextShutdownNotifierFactory::GetInstance();
}
} // namespace shell
} // namespace chromecast
......@@ -5,7 +5,12 @@
#ifndef CHROMECAST_BROWSER_CAST_EXTENSION_URL_LOADER_FACTORY_H_
#define CHROMECAST_BROWSER_CAST_EXTENSION_URL_LOADER_FACTORY_H_
#include <memory>
#include "base/macros.h"
#include "base/no_destructor.h"
#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
#include "content/public/browser/non_network_url_loader_factory_base.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/pending_remote.h"
......@@ -44,6 +49,8 @@ class CastExtensionURLLoaderFactory
content::BrowserContext* browser_context,
mojo::PendingRemote<network::mojom::URLLoaderFactory> extension_factory);
static void EnsureShutdownNotifierFactoryBuilt();
private:
~CastExtensionURLLoaderFactory() override;
......@@ -65,10 +72,31 @@ class CastExtensionURLLoaderFactory
const net::MutableNetworkTrafficAnnotationTag& traffic_annotation)
override;
void OnBrowserContextDestroyed();
class BrowserContextShutdownNotifierFactory
: public BrowserContextKeyedServiceShutdownNotifierFactory {
public:
static BrowserContextShutdownNotifierFactory* GetInstance();
// No copying.
BrowserContextShutdownNotifierFactory(
const BrowserContextShutdownNotifierFactory&) = delete;
BrowserContextShutdownNotifierFactory& operator=(
const BrowserContextShutdownNotifierFactory&) = delete;
private:
friend class base::NoDestructor<BrowserContextShutdownNotifierFactory>;
BrowserContextShutdownNotifierFactory();
};
extensions::ExtensionRegistry* extension_registry_;
mojo::Remote<network::mojom::URLLoaderFactory> extension_factory_;
scoped_refptr<network::SharedURLLoaderFactory> network_factory_;
std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
browser_context_shutdown_subscription_;
DISALLOW_COPY_AND_ASSIGN(CastExtensionURLLoaderFactory);
};
......
......@@ -17,6 +17,19 @@ NonNetworkURLLoaderFactoryBase::NonNetworkURLLoaderFactoryBase(
NonNetworkURLLoaderFactoryBase::~NonNetworkURLLoaderFactoryBase() = default;
void NonNetworkURLLoaderFactoryBase::DisconnectReceiversAndDestroy() {
// Clear |receivers_| to explicitly make sure that no further method
// invocations or disconnection notifications will happen. (per the
// comment of mojo::ReceiverSet::Clear)
receivers_.Clear();
// Similarly to OnDisconnect, if there are no more |receivers_|, then no
// instance methods of |this| can be called in the future (mojo methods Clone
// and CreateLoaderAndStart should be the only public entrypoints).
// Therefore, it is safe to delete |this| at this point.
delete this;
}
void NonNetworkURLLoaderFactoryBase::Clone(
mojo::PendingReceiver<network::mojom::URLLoaderFactory> loader) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
......@@ -27,8 +40,13 @@ void NonNetworkURLLoaderFactoryBase::Clone(
void NonNetworkURLLoaderFactoryBase::OnDisconnect() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
if (receivers_.empty())
if (receivers_.empty()) {
// If there are no more |receivers_|, then no instance methods of |this| can
// be called in the future (mojo methods Clone and CreateLoaderAndStart
// should be the only public entrypoints). Therefore, it is safe to delete
// |this| at this point.
delete this;
}
}
} // namespace content
......@@ -39,6 +39,18 @@ class CONTENT_EXPORT NonNetworkURLLoaderFactoryBase
~NonNetworkURLLoaderFactoryBase() override;
// Sometimes a derived class can no longer function, even when the set of
// |receivers_| is still non-empty. This should be rare (typically the
// lifetime of users of mojo::Remote<network::mojom::URLLoaderFactory> should
// be shorter than whatever the factory depends on), but may happen in some
// corner cases (e.g. in a race between 1) BrowserContext destruction and 2)
// CreateLoaderAndStart mojo call).
//
// When a derived class gets notified that its dependencies got destroyed, it
// should call DisconnectReceiversAndDestroy to prevent any future calls to
// CreateLoaderAndStart.
void DisconnectReceiversAndDestroy();
THREAD_CHECKER(thread_checker_);
private:
......
......@@ -39,6 +39,7 @@
#include "extensions/browser/event_router_factory.h"
#include "extensions/browser/extension_message_filter.h"
#include "extensions/browser/extension_prefs_factory.h"
#include "extensions/browser/extension_protocols.h"
#include "extensions/browser/guest_view/extensions_guest_view_message_filter.h"
#include "extensions/browser/process_manager_factory.h"
#include "extensions/browser/renderer_startup_helper.h"
......@@ -83,6 +84,7 @@ void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
declarative_net_request::RulesMonitorService::GetFactoryInstance();
DeclarativeUserScriptManagerFactory::GetInstance();
DisplaySourceEventRouterFactory::GetInstance();
EnsureExtensionURLLoaderFactoryShutdownNotifierFactoryBuilt();
EventRouterFactory::GetInstance();
ExtensionMessageFilter::EnsureShutdownNotifierFactoryBuilt();
ExtensionsGuestViewMessageFilter::EnsureShutdownNotifierFactoryBuilt();
......
......@@ -29,6 +29,7 @@
#include "base/metrics/histogram_functions.h"
#include "base/metrics/histogram_macros.h"
#include "base/metrics/user_metrics.h"
#include "base/no_destructor.h"
#include "base/numerics/safe_conversions.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
......@@ -38,6 +39,8 @@
#include "base/threading/thread_restrictions.h"
#include "base/timer/elapsed_timer.h"
#include "build/build_config.h"
#include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
#include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
......@@ -53,6 +56,7 @@
#include "extensions/browser/content_verify_job.h"
#include "extensions/browser/extension_navigation_ui_data.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_registry_factory.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/extension_util.h"
#include "extensions/browser/extensions_browser_client.h"
......@@ -61,6 +65,7 @@
#include "extensions/browser/info_map.h"
#include "extensions/browser/media_router_extension_access_logger.h"
#include "extensions/browser/process_map.h"
#include "extensions/browser/process_map_factory.h"
#include "extensions/browser/url_request_util.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
......@@ -459,8 +464,15 @@ class ExtensionURLLoaderFactory
base::UkmSourceId ukm_source_id,
bool is_web_view_request,
int render_process_id) {
DCHECK(browser_context);
mojo::PendingRemote<network::mojom::URLLoaderFactory> pending_remote;
// Return an unbound |pending_remote| if the |browser_context| has already
// started shutting down.
if (browser_context->ShutdownStarted())
return pending_remote;
// Manages its own lifetime.
new ExtensionURLLoaderFactory(
browser_context, ukm_source_id, is_web_view_request, render_process_id,
......@@ -469,6 +481,10 @@ class ExtensionURLLoaderFactory
return pending_remote;
}
static void EnsureShutdownNotifierFactoryBuilt() {
BrowserContextShutdownNotifierFactory::GetInstance();
}
private:
// Constructs ExtensionURLLoaderFactory bound to the |factory_receiver|.
//
......@@ -490,6 +506,16 @@ class ExtensionURLLoaderFactory
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
extension_info_map_ =
extensions::ExtensionSystem::Get(browser_context_)->info_map();
// base::Unretained is safe below, because lifetime of
// |browser_context_shutdown_subscription_| guarantees that
// OnBrowserContextDestroyed won't be called after |this| is destroyed.
browser_context_shutdown_subscription_ =
BrowserContextShutdownNotifierFactory::GetInstance()
->Get(browser_context)
->Subscribe(base::BindRepeating(
&ExtensionURLLoaderFactory::OnBrowserContextDestroyed,
base::Unretained(this)));
}
~ExtensionURLLoaderFactory() override = default;
......@@ -506,6 +532,12 @@ class ExtensionURLLoaderFactory
override {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (browser_context_->ShutdownStarted()) {
mojo::Remote<network::mojom::URLLoaderClient>(std::move(client))
->OnComplete(network::URLLoaderCompletionStatus(net::ERR_FAILED));
return;
}
client =
WrapWithMetricsIfNeeded(request.url, ukm_source_id_, std::move(client));
......@@ -723,6 +755,38 @@ class ExtensionURLLoaderFactory
/* allow_directory_listing */ false, std::move(response_headers));
}
void OnBrowserContextDestroyed() {
// When |browser_context_| gets destroyed, |this| factory is not able to
// serve any more requests.
DisconnectReceiversAndDestroy();
}
class BrowserContextShutdownNotifierFactory
: public BrowserContextKeyedServiceShutdownNotifierFactory {
public:
static BrowserContextShutdownNotifierFactory* GetInstance() {
static base::NoDestructor<BrowserContextShutdownNotifierFactory>
s_factory;
return s_factory.get();
}
// No copying.
BrowserContextShutdownNotifierFactory(
const BrowserContextShutdownNotifierFactory&) = delete;
BrowserContextShutdownNotifierFactory& operator=(
const BrowserContextShutdownNotifierFactory&) = delete;
private:
friend class base::NoDestructor<BrowserContextShutdownNotifierFactory>;
BrowserContextShutdownNotifierFactory()
: BrowserContextKeyedServiceShutdownNotifierFactory(
"ExtensionURLLoaderFactory::"
"BrowserContextShutdownNotifierFactory") {
DependsOn(ExtensionRegistryFactory::GetInstance());
DependsOn(ProcessMapFactory::GetInstance());
}
};
content::BrowserContext* browser_context_;
bool is_web_view_request_;
base::UkmSourceId ukm_source_id_;
......@@ -733,6 +797,9 @@ class ExtensionURLLoaderFactory
const int render_process_id_;
scoped_refptr<extensions::InfoMap> extension_info_map_;
std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
browser_context_shutdown_subscription_;
DISALLOW_COPY_AND_ASSIGN(ExtensionURLLoaderFactory);
};
......@@ -828,4 +895,8 @@ CreateExtensionURLLoaderFactory(int render_process_id, int render_frame_id) {
browser_context, ukm_source_id, is_web_view_request, render_process_id);
}
void EnsureExtensionURLLoaderFactoryShutdownNotifierFactoryBuilt() {
ExtensionURLLoaderFactory::EnsureShutdownNotifierFactoryBuilt();
}
} // namespace extensions
......@@ -77,6 +77,8 @@ CreateExtensionServiceWorkerScriptURLLoaderFactory(
mojo::PendingRemote<network::mojom::URLLoaderFactory>
CreateExtensionURLLoaderFactory(int render_process_id, int render_frame_id);
void EnsureExtensionURLLoaderFactoryShutdownNotifierFactoryBuilt();
} // namespace extensions
#endif // EXTENSIONS_BROWSER_EXTENSION_PROTOCOLS_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment