From ff0e5c0348d763e525e44b99e1885a9623b6ad8f Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Mon, 10 Jan 2022 16:11:16 -0500 Subject: [PATCH] Fix possible use after shutdown of BrowserContext (fixes issue #3193) --- libcef/browser/browser_context.cc | 3 + .../browser/media_router/media_route_impl.cc | 2 +- .../browser/media_router/media_router_impl.cc | 2 +- libcef/browser/net_service/cookie_helper.cc | 63 +++++++++++++------ libcef/browser/net_service/cookie_helper.h | 10 ++- .../net_service/cookie_manager_impl.cc | 2 +- .../resource_request_handler_wrapper.cc | 30 +++++---- 7 files changed, 72 insertions(+), 40 deletions(-) diff --git a/libcef/browser/browser_context.cc b/libcef/browser/browser_context.cc index ca13d7e28..305c6ad02 100644 --- a/libcef/browser/browser_context.cc +++ b/libcef/browser/browser_context.cc @@ -213,6 +213,9 @@ void CefBrowserContext::Shutdown() { // Destroy objects that may hold references to the MediaRouter. media_router_manager_.reset(); + + // Invalidate any Getter references to this object. + weak_ptr_factory_.InvalidateWeakPtrs(); } void CefBrowserContext::AddCefRequestContext(CefRequestContextImpl* context) { diff --git a/libcef/browser/media_router/media_route_impl.cc b/libcef/browser/media_router/media_route_impl.cc index a4f2c356a..1a3be8f60 100644 --- a/libcef/browser/media_router/media_route_impl.cc +++ b/libcef/browser/media_router/media_route_impl.cc @@ -16,7 +16,7 @@ CefBrowserContext* GetBrowserContext(const CefBrowserContext::Getter& getter) { CEF_REQUIRE_UIT(); DCHECK(!getter.is_null()); - // Will return nullptr if the BrowserContext has been destroyed. + // Will return nullptr if the BrowserContext has been shut down. return getter.Run(); } diff --git a/libcef/browser/media_router/media_router_impl.cc b/libcef/browser/media_router/media_router_impl.cc index dcf5a553a..8e874e041 100644 --- a/libcef/browser/media_router/media_router_impl.cc +++ b/libcef/browser/media_router/media_router_impl.cc @@ -17,7 +17,7 @@ CefBrowserContext* GetBrowserContext(const CefBrowserContext::Getter& getter) { CEF_REQUIRE_UIT(); DCHECK(!getter.is_null()); - // Will return nullptr if the BrowserContext has been destroyed. + // Will return nullptr if the BrowserContext has been shut down. return getter.Run(); } diff --git a/libcef/browser/net_service/cookie_helper.cc b/libcef/browser/net_service/cookie_helper.cc index aab96ec5d..0b9603c20 100644 --- a/libcef/browser/net_service/cookie_helper.cc +++ b/libcef/browser/net_service/cookie_helper.cc @@ -22,7 +22,16 @@ namespace cookie_helper { namespace { -// Do not keep a reference to the CookieManager returned by this method. +// Do not keep a reference to the object returned by this method. +CefBrowserContext* GetBrowserContext(const CefBrowserContext::Getter& getter) { + CEF_REQUIRE_UIT(); + DCHECK(!getter.is_null()); + + // Will return nullptr if the BrowserContext has been shut down. + return getter.Run(); +} + +// Do not keep a reference to the object returned by this method. network::mojom::CookieManager* GetCookieManager( content::BrowserContext* browser_context) { CEF_REQUIRE_UIT(); @@ -82,13 +91,22 @@ void GetCookieListCallback(const AllowCookieCallback& allow_cookie_callback, } void LoadCookiesOnUIThread( - content::BrowserContext* browser_context, + const CefBrowserContext::Getter& browser_context_getter, const GURL& url, const net::CookieOptions& options, net::CookiePartitionKeyCollection cookie_partition_key_collection, const AllowCookieCallback& allow_cookie_callback, DoneCookieCallback done_callback) { - CEF_REQUIRE_UIT(); + auto cef_browser_context = GetBrowserContext(browser_context_getter); + auto browser_context = + cef_browser_context ? cef_browser_context->AsBrowserContext() : nullptr; + if (!browser_context) { + GetCookieListCallback(allow_cookie_callback, std::move(done_callback), + net::CookieAccessResultList(), + net::CookieAccessResultList()); + return; + } + GetCookieManager(browser_context) ->GetCookieList( url, options, cookie_partition_key_collection, @@ -126,15 +144,23 @@ void SetCanonicalCookieCallback(SaveCookiesProgress* progress, } } -void SaveCookiesOnUIThread(content::BrowserContext* browser_context, - const GURL& url, - const net::CookieOptions& options, - int total_count, - net::CookieList cookies, - DoneCookieCallback done_callback) { - CEF_REQUIRE_UIT(); +void SaveCookiesOnUIThread( + const CefBrowserContext::Getter& browser_context_getter, + const GURL& url, + const net::CookieOptions& options, + int total_count, + net::CookieList cookies, + DoneCookieCallback done_callback) { DCHECK(!cookies.empty()); + auto cef_browser_context = GetBrowserContext(browser_context_getter); + auto browser_context = + cef_browser_context ? cef_browser_context->AsBrowserContext() : nullptr; + if (!browser_context) { + std::move(done_callback).Run(0, net::CookieList()); + return; + } + network::mojom::CookieManager* cookie_manager = GetCookieManager(browser_context); @@ -186,7 +212,7 @@ bool IsCookieableScheme( return url.SchemeIsHTTPOrHTTPS() || url.SchemeIsWSOrWSS(); } -void LoadCookies(content::BrowserContext* browser_context, +void LoadCookies(const CefBrowserContext::Getter& browser_context_getter, const network::ResourceRequest& request, const AllowCookieCallback& allow_cookie_callback, DoneCookieCallback done_callback) { @@ -201,14 +227,13 @@ void LoadCookies(content::BrowserContext* browser_context, } CEF_POST_TASK( - CEF_UIT, - base::BindOnce(LoadCookiesOnUIThread, browser_context, request.url, - GetCookieOptions(request), - net::CookiePartitionKeyCollection(), - allow_cookie_callback, std::move(done_callback))); + CEF_UIT, base::BindOnce(LoadCookiesOnUIThread, browser_context_getter, + request.url, GetCookieOptions(request), + net::CookiePartitionKeyCollection(), + allow_cookie_callback, std::move(done_callback))); } -void SaveCookies(content::BrowserContext* browser_context, +void SaveCookies(const CefBrowserContext::Getter& browser_context_getter, const network::ResourceRequest& request, net::HttpResponseHeaders* headers, const AllowCookieCallback& allow_cookie_callback, @@ -256,8 +281,8 @@ void SaveCookies(content::BrowserContext* browser_context, if (!allowed_cookies.empty()) { CEF_POST_TASK( CEF_UIT, - base::BindOnce(SaveCookiesOnUIThread, browser_context, request.url, - GetCookieOptions(request), total_count, + base::BindOnce(SaveCookiesOnUIThread, browser_context_getter, + request.url, GetCookieOptions(request), total_count, std::move(allowed_cookies), std::move(done_callback))); } else { diff --git a/libcef/browser/net_service/cookie_helper.h b/libcef/browser/net_service/cookie_helper.h index 06459a5a9..b1fe26108 100644 --- a/libcef/browser/net_service/cookie_helper.h +++ b/libcef/browser/net_service/cookie_helper.h @@ -5,13 +5,11 @@ #ifndef CEF_LIBCEF_BROWSER_NET_SERVICE_COOKIE_HELPER_H_ #define CEF_LIBCEF_BROWSER_NET_SERVICE_COOKIE_HELPER_H_ +#include "libcef/browser/browser_context.h" + #include "base/callback_forward.h" #include "net/cookies/canonical_cookie.h" -namespace content { -class BrowserContext; -} - namespace net { class HttpResponseHeaders; } @@ -44,7 +42,7 @@ using DoneCookieCallback = // both retrieved and allowed by |allow_cookie_callback|. The loaded cookies // will not be set on |request|; that should be done in |done_callback|. Must be // called on the IO thread. -void LoadCookies(content::BrowserContext* browser_context, +void LoadCookies(const CefBrowserContext::Getter& browser_context_getter, const network::ResourceRequest& request, const AllowCookieCallback& allow_cookie_callback, DoneCookieCallback done_callback); @@ -55,7 +53,7 @@ void LoadCookies(content::BrowserContext* browser_context, // retrieved, and |allowed_cookies| representing the list of cookies that were // both allowed by |allow_cookie_callback| an successfully saved. Must be called // on the IO thread. -void SaveCookies(content::BrowserContext* browser_context, +void SaveCookies(const CefBrowserContext::Getter& browser_context_getter, const network::ResourceRequest& request, net::HttpResponseHeaders* headers, const AllowCookieCallback& allow_cookie_callback, diff --git a/libcef/browser/net_service/cookie_manager_impl.cc b/libcef/browser/net_service/cookie_manager_impl.cc index c67dc8cf2..21f801462 100644 --- a/libcef/browser/net_service/cookie_manager_impl.cc +++ b/libcef/browser/net_service/cookie_manager_impl.cc @@ -23,7 +23,7 @@ CefBrowserContext* GetBrowserContext(const CefBrowserContext::Getter& getter) { CEF_REQUIRE_UIT(); DCHECK(!getter.is_null()); - // Will return nullptr if the BrowserContext has been destroyed. + // Will return nullptr if the BrowserContext has been shut down. return getter.Run(); } diff --git a/libcef/browser/net_service/resource_request_handler_wrapper.cc b/libcef/browser/net_service/resource_request_handler_wrapper.cc index 718d14080..7c71015ce 100644 --- a/libcef/browser/net_service/resource_request_handler_wrapper.cc +++ b/libcef/browser/net_service/resource_request_handler_wrapper.cc @@ -244,10 +244,9 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { const base::RepeatingClosure& unhandled_request_callback) { CEF_REQUIRE_UIT(); - browser_context_ = browser_context; - auto profile = Profile::FromBrowserContext(browser_context); auto cef_browser_context = CefBrowserContext::FromProfile(profile); + browser_context_getter_ = cef_browser_context->getter(); iothread_state_ = cef_browser_context->iothread_state(); CHECK(iothread_state_); cookieable_schemes_ = cef_browser_context->GetCookieableSchemes(); @@ -291,7 +290,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { std::unique_ptr observer) {} // Only accessed on the UI thread. - content::BrowserContext* browser_context_ = nullptr; + CefBrowserContext::Getter browser_context_getter_; bool initialized_ = false; @@ -410,7 +409,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { static void TryCreateURLLoaderNetworkObserver( std::unique_ptr pending_request, CefRefPtr frame, - content::BrowserContext* browser_context, + const CefBrowserContext::Getter& browser_context_getter, base::WeakPtr self) { CEF_REQUIRE_UIT(); @@ -428,10 +427,16 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { ->CreateURLLoaderNetworkObserver(); } } else { - url_loader_network_observer = - static_cast( - browser_context->GetDefaultStoragePartition()) - ->CreateAuthCertObserverForServiceWorker(); + auto cef_browser_context = browser_context_getter.Run(); + auto browser_context = cef_browser_context + ? cef_browser_context->AsBrowserContext() + : nullptr; + if (browser_context) { + url_loader_network_observer = + static_cast( + browser_context->GetDefaultStoragePartition()) + ->CreateAuthCertObserverForServiceWorker(); + } } CEF_POST_TASK(CEF_IOT, @@ -488,7 +493,8 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { std::make_unique( request_id, request, request_was_redirected, std::move(callback), std::move(cancel_callback)), - init_state_->frame_, init_state_->browser_context_, + init_state_->frame_, + init_state_->browser_context_getter_, weak_ptr_factory_.GetWeakPtr())); return; } @@ -582,7 +588,7 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { &InterceptedRequestHandlerWrapper::ContinueWithLoadedCookies, weak_ptr_factory_.GetWeakPtr(), request_id, request, std::move(callback)); - cookie_helper::LoadCookies(init_state_->browser_context_, *request, + cookie_helper::LoadCookies(init_state_->browser_context_getter_, *request, allow_cookie_callback, std::move(done_cookie_callback)); } @@ -960,8 +966,8 @@ class InterceptedRequestHandlerWrapper : public InterceptedRequestHandler { auto done_cookie_callback = base::BindOnce( &InterceptedRequestHandlerWrapper::ContinueWithSavedCookies, weak_ptr_factory_.GetWeakPtr(), request_id, std::move(callback)); - cookie_helper::SaveCookies(init_state_->browser_context_, *request, headers, - allow_cookie_callback, + cookie_helper::SaveCookies(init_state_->browser_context_getter_, *request, + headers, allow_cookie_callback, std::move(done_cookie_callback)); }