From fbc59483b9fa5fbcf349daa6cc36f3a2b3ca8849 Mon Sep 17 00:00:00 2001 From: Marshall Greenblatt Date: Mon, 8 Oct 2018 18:57:14 +0300 Subject: [PATCH] Fix crash using CefCookieManager::SetStoragePath (issue #2522) --- BUILD.gn | 2 + libcef/browser/cookie_manager_impl.cc | 57 ++------- libcef/browser/cookie_manager_impl.h | 6 +- libcef/browser/net/cookie_store_proxy.cc | 35 ++---- libcef/browser/net/cookie_store_proxy.h | 18 +-- libcef/browser/net/cookie_store_source.cc | 111 ++++++++++++++++++ libcef/browser/net/cookie_store_source.h | 75 ++++++++++++ .../net/url_request_context_getter_impl.cc | 63 ++-------- .../net/url_request_context_getter_impl.h | 5 +- .../browser/net/url_request_context_proxy.cc | 4 +- 10 files changed, 234 insertions(+), 142 deletions(-) create mode 100644 libcef/browser/net/cookie_store_source.cc create mode 100644 libcef/browser/net/cookie_store_source.h diff --git a/BUILD.gn b/BUILD.gn index 46f5698af..bc0090767 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -405,6 +405,8 @@ static_library("libcef_static") { "libcef/browser/net/chrome_scheme_handler.h", "libcef/browser/net/cookie_store_proxy.cc", "libcef/browser/net/cookie_store_proxy.h", + "libcef/browser/net/cookie_store_source.cc", + "libcef/browser/net/cookie_store_source.h", "libcef/browser/net/crlset_file_util_impl.cc", "libcef/browser/net/devtools_scheme_handler.cc", "libcef/browser/net/devtools_scheme_handler.h", diff --git a/libcef/browser/cookie_manager_impl.cc b/libcef/browser/cookie_manager_impl.cc index 2d4061a04..c2e67c4c1 100644 --- a/libcef/browser/cookie_manager_impl.cc +++ b/libcef/browser/cookie_manager_impl.cc @@ -10,12 +10,12 @@ #include "libcef/browser/content_browser_client.h" #include "libcef/browser/context.h" +#include "libcef/browser/net/cookie_store_source.h" #include "libcef/browser/net/network_delegate.h" #include "libcef/common/task_runner_impl.h" #include "libcef/common/time_util.h" #include "base/bind.h" -#include "base/files/file_util.h" #include "base/format_macros.h" #include "base/logging.h" #include "base/threading/thread_restrictions.h" @@ -24,7 +24,6 @@ #include "content/browser/storage_partition_impl.h" #include "net/cookies/cookie_util.h" #include "net/cookies/parsed_cookie.h" -#include "net/extras/sqlite/sqlite_persistent_cookie_store.h" #include "net/url_request/url_request_context.h" #include "url/gurl.h" @@ -164,7 +163,7 @@ void CefCookieManagerImpl::GetCookieStore( return; } - DCHECK(is_blocking_ || cookie_store_.get()); + DCHECK(is_blocking_ || cookie_source_); // Binding ref-counted |this| to CookieStoreGetter may result in // heap-use-after-free if (a) the CookieStoreGetter contains the last @@ -187,8 +186,8 @@ void CefCookieManagerImpl::GetCookieStore( net::CookieStore* CefCookieManagerImpl::GetExistingCookieStore() { CEF_REQUIRE_IOT(); - if (cookie_store_.get()) { - return cookie_store_.get(); + if (cookie_source_) { + return cookie_source_->GetCookieStore(); } else if (request_context_impl_.get()) { net::CookieStore* cookie_store = request_context_impl_->GetExistingCookieStore(); @@ -289,45 +288,14 @@ bool CefCookieManagerImpl::SetStoragePath( if (!path.empty()) new_path = base::FilePath(path); - if (cookie_store_.get() && - ((storage_path_.empty() && path.empty()) || storage_path_ == new_path)) { - // The path has not changed so don't do anything. - RunAsyncCompletionOnIOThread(callback); - return true; + if (!cookie_source_) { + cookie_source_.reset(new CefCookieStoreOwnerSource()); } - scoped_refptr persistent_store; - if (!new_path.empty()) { - // TODO(cef): Move directory creation to the blocking pool instead of - // allowing file IO on this thread. - base::ThreadRestrictions::ScopedAllowIO allow_io; - if (base::DirectoryExists(new_path) || base::CreateDirectory(new_path)) { - const base::FilePath& cookie_path = new_path.AppendASCII("Cookies"); - persistent_store = new net::SQLitePersistentCookieStore( - cookie_path, BrowserThread::GetTaskRunnerForThread(BrowserThread::IO), - // Intentionally using the background task runner exposed by CEF to - // facilitate unit test expectations. This task runner MUST be - // configured with BLOCK_SHUTDOWN. - CefContentBrowserClient::Get()->background_task_runner(), - persist_session_cookies, nullptr); - } else { - NOTREACHED() << "The cookie storage directory could not be created"; - storage_path_.clear(); - } - } - - // Set the new cookie store that will be used for all new requests. The old - // cookie store, if any, will be automatically flushed and closed when no - // longer referenced. - cookie_store_.reset(new net::CookieMonster(persistent_store.get(), nullptr, - g_browser_process->net_log())); - if (persistent_store.get() && persist_session_cookies) - cookie_store_->SetPersistSessionCookies(true); - storage_path_ = new_path; - - // Restore the previously supported schemes. - SetSupportedSchemesInternal(supported_schemes_, callback); + cookie_source_->SetCookieStoragePath(new_path, persist_session_cookies, + g_browser_process->net_log()); + RunAsyncCompletionOnIOThread(callback); return true; } @@ -505,10 +473,9 @@ void CefCookieManagerImpl::SetSupportedSchemesInternal( return; } - DCHECK(is_blocking_ || cookie_store_.get()); - if (cookie_store_) { - supported_schemes_ = schemes; - SetCookieMonsterSchemes(cookie_store_.get(), supported_schemes_); + DCHECK(is_blocking_ || cookie_source_); + if (cookie_source_) { + cookie_source_->SetCookieSupportedSchemes(schemes); } RunAsyncCompletionOnIOThread(callback); diff --git a/libcef/browser/cookie_manager_impl.h b/libcef/browser/cookie_manager_impl.h index e9459d05b..31ffb4c9a 100644 --- a/libcef/browser/cookie_manager_impl.h +++ b/libcef/browser/cookie_manager_impl.h @@ -15,6 +15,8 @@ #include "base/memory/weak_ptr.h" #include "net/cookies/cookie_monster.h" +class CefCookieStoreOwnerSource; + // Implementation of the CefCookieManager interface. class CefCookieManagerImpl : public CefCookieManager { public: @@ -122,9 +124,7 @@ class CefCookieManagerImpl : public CefCookieManager { scoped_refptr request_context_impl_; // Used for cookie monsters owned by this object. - base::FilePath storage_path_; - std::vector supported_schemes_; - std::unique_ptr cookie_store_; + std::unique_ptr cookie_source_; // Must be the last member. base::WeakPtrFactory weak_ptr_factory_; diff --git a/libcef/browser/net/cookie_store_proxy.cc b/libcef/browser/net/cookie_store_proxy.cc index 654f07bfa..cee8e6147 100644 --- a/libcef/browser/net/cookie_store_proxy.cc +++ b/libcef/browser/net/cookie_store_proxy.cc @@ -4,12 +4,12 @@ #include "libcef/browser/net/cookie_store_proxy.h" -#include "libcef/browser/cookie_manager_impl.h" -#include "libcef/browser/net/url_request_context_impl.h" +#include "include/cef_request_context.h" +#include "libcef/browser/net/cookie_store_source.h" #include "libcef/browser/thread_util.h" #include "base/logging.h" -#include "net/url_request/url_request_context.h" +#include "net/cookies/cookie_change_dispatcher.h" namespace { @@ -44,12 +44,10 @@ class NullCookieChangeDispatcher : public net::CookieChangeDispatcher { } // namespace CefCookieStoreProxy::CefCookieStoreProxy( - CefURLRequestContextImpl* parent, - CefRefPtr handler) - : parent_(parent), handler_(handler) { + std::unique_ptr source) + : source_(std::move(source)) { CEF_REQUIRE_IOT(); - DCHECK(parent_); - DCHECK(handler_.get()); + DCHECK(source_); } CefCookieStoreProxy::~CefCookieStoreProxy() { @@ -189,24 +187,5 @@ bool CefCookieStoreProxy::IsEphemeral() { net::CookieStore* CefCookieStoreProxy::GetCookieStore() { CEF_REQUIRE_IOT(); - - CefRefPtr manager = handler_->GetCookieManager(); - if (manager.get()) { - // Use the cookie store provided by the manager. May be nullptr if the - // cookie manager is blocking. - return reinterpret_cast(manager.get()) - ->GetExistingCookieStore(); - } - - DCHECK(parent_); - if (parent_) { - // Use the cookie store from the parent. - net::CookieStore* cookie_store = parent_->cookie_store(); - DCHECK(cookie_store); - if (!cookie_store) - LOG(ERROR) << "Cookie store does not exist"; - return cookie_store; - } - - return nullptr; + return source_->GetCookieStore(); } diff --git a/libcef/browser/net/cookie_store_proxy.h b/libcef/browser/net/cookie_store_proxy.h index c1f1e4b0e..4567f9b91 100644 --- a/libcef/browser/net/cookie_store_proxy.h +++ b/libcef/browser/net/cookie_store_proxy.h @@ -6,19 +6,15 @@ #define CEF_LIBCEF_BROWSER_COOKIE_STORE_PROXY_H_ #pragma once -#include "include/cef_request_context_handler.h" - #include "net/cookies/cookie_store.h" -class CefURLRequestContextImpl; +class CefCookieStoreSource; -// Proxies cookie requests to the CefRequestContextHandler or global cookie -// store. Life span is controlled by CefURLRequestContextProxy. Only accessed on -// the IO thread. See browser_context.h for an object relationship diagram. +// Proxies cookie requests to a CefCookieStoreSource (see comments on the +// implementation classes for details). Only accessed on the IO thread. class CefCookieStoreProxy : public net::CookieStore { public: - CefCookieStoreProxy(CefURLRequestContextImpl* parent, - CefRefPtr handler); + explicit CefCookieStoreProxy(std::unique_ptr source); ~CefCookieStoreProxy() override; // net::CookieStore methods. @@ -52,11 +48,7 @@ class CefCookieStoreProxy : public net::CookieStore { private: net::CookieStore* GetCookieStore(); - // The |parent_| pointer is kept alive by CefURLRequestContextGetterProxy - // which has a ref to the owning CefURLRequestContextGetterImpl. - CefURLRequestContextImpl* parent_; - CefRefPtr handler_; - + std::unique_ptr const source_; std::unique_ptr null_dispatcher_; DISALLOW_COPY_AND_ASSIGN(CefCookieStoreProxy); diff --git a/libcef/browser/net/cookie_store_source.cc b/libcef/browser/net/cookie_store_source.cc new file mode 100644 index 000000000..5755418f4 --- /dev/null +++ b/libcef/browser/net/cookie_store_source.cc @@ -0,0 +1,111 @@ +// Copyright (c) 2018 The Chromium Embedded Framework Authors. All rights +// reserved. Use of this source code is governed by a BSD-style license that can +// be found in the LICENSE file. + +#include "libcef/browser/net/cookie_store_source.h" + +#include "libcef/browser/content_browser_client.h" +#include "libcef/browser/cookie_manager_impl.h" +#include "libcef/browser/net/url_request_context_impl.h" +#include "libcef/browser/thread_util.h" + +#include "base/files/file_util.h" +#include "base/logging.h" +#include "net/extras/sqlite/sqlite_persistent_cookie_store.h" + +CefCookieStoreHandlerSource::CefCookieStoreHandlerSource( + CefURLRequestContextImpl* parent, + CefRefPtr handler) + : parent_(parent), handler_(handler) { + DCHECK(parent_); + DCHECK(handler_); +} + +net::CookieStore* CefCookieStoreHandlerSource::GetCookieStore() { + CEF_REQUIRE_IOT(); + + CefRefPtr manager = handler_->GetCookieManager(); + if (manager) { + // Use the cookie store provided by the manager. May be nullptr if the + // cookie manager is blocking. + return reinterpret_cast(manager.get()) + ->GetExistingCookieStore(); + } + + DCHECK(parent_); + if (parent_) { + // Use the cookie store from the parent. + net::CookieStore* cookie_store = parent_->cookie_store(); + DCHECK(cookie_store); + if (!cookie_store) + LOG(ERROR) << "Cookie store does not exist"; + return cookie_store; + } + + return nullptr; +} + +CefCookieStoreOwnerSource::CefCookieStoreOwnerSource() {} + +void CefCookieStoreOwnerSource::SetCookieStoragePath( + const base::FilePath& path, + bool persist_session_cookies, + net::NetLog* net_log) { + CEF_REQUIRE_IOT(); + + if (cookie_store_ && ((path_.empty() && path.empty()) || path_ == path)) { + // The path has not changed so don't do anything. + return; + } + + scoped_refptr persistent_store; + if (!path.empty()) { + // TODO(cef): Move directory creation to the blocking pool instead of + // allowing file IO on this thread. + base::ThreadRestrictions::ScopedAllowIO allow_io; + if (base::DirectoryExists(path) || base::CreateDirectory(path)) { + const base::FilePath& cookie_path = path.AppendASCII("Cookies"); + persistent_store = new net::SQLitePersistentCookieStore( + cookie_path, + content::BrowserThread::GetTaskRunnerForThread( + content::BrowserThread::IO), + // Intentionally using the background task runner exposed by CEF to + // facilitate unit test expectations. This task runner MUST be + // configured with BLOCK_SHUTDOWN. + CefContentBrowserClient::Get()->background_task_runner(), + persist_session_cookies, NULL); + } else { + NOTREACHED() << "The cookie storage directory could not be created"; + } + } + + // Set the new cookie store that will be used for all new requests. The old + // cookie store, if any, will be automatically flushed and closed when no + // longer referenced. + std::unique_ptr cookie_monster( + new net::CookieMonster(persistent_store.get(), nullptr, net_log)); + if (persistent_store.get() && persist_session_cookies) + cookie_monster->SetPersistSessionCookies(true); + path_ = path; + + // Restore the previously supported schemes. + CefCookieManagerImpl::SetCookieMonsterSchemes(cookie_monster.get(), + supported_schemes_); + + cookie_store_ = std::move(cookie_monster); +} + +void CefCookieStoreOwnerSource::SetCookieSupportedSchemes( + const std::vector& schemes) { + CEF_REQUIRE_IOT(); + + supported_schemes_ = schemes; + CefCookieManagerImpl::SetCookieMonsterSchemes( + static_cast(cookie_store_.get()), + supported_schemes_); +} + +net::CookieStore* CefCookieStoreOwnerSource::GetCookieStore() { + CEF_REQUIRE_IOT(); + return cookie_store_.get(); +} diff --git a/libcef/browser/net/cookie_store_source.h b/libcef/browser/net/cookie_store_source.h new file mode 100644 index 000000000..a094b5c2c --- /dev/null +++ b/libcef/browser/net/cookie_store_source.h @@ -0,0 +1,75 @@ +// Copyright (c) 2018 The Chromium Embedded Framework Authors. All rights +// reserved. Use of this source code is governed by a BSD-style license that can +// be found in the LICENSE file. + +#ifndef CEF_LIBCEF_BROWSER_COOKIE_STORE_SOURCE_H_ +#define CEF_LIBCEF_BROWSER_COOKIE_STORE_SOURCE_H_ +#pragma once + +#include +#include +#include + +#include "include/cef_request_context_handler.h" + +#include "base/macros.h" + +namespace base { +class FilePath; +} + +namespace net { +class CookieStore; +class NetLog; +} // namespace net + +class CefURLRequestContextImpl; + +// Abstract base class for CookieStore sources. Only accessed on the IO thread. +class CefCookieStoreSource { + public: + virtual net::CookieStore* GetCookieStore() = 0; + virtual ~CefCookieStoreSource() {} +}; + +// Sources a cookie store that is created/owned by a CefCookieManager or the +// parent context. Life span is controlled by CefURLRequestContextProxy. See +// browser_context.h for an object relationship diagram. +class CefCookieStoreHandlerSource : public CefCookieStoreSource { + public: + CefCookieStoreHandlerSource(CefURLRequestContextImpl* parent, + CefRefPtr handler); + + net::CookieStore* GetCookieStore() override; + + private: + // The |parent_| pointer is kept alive by CefURLRequestContextGetterProxy + // which has a ref to the owning CefURLRequestContextGetterImpl. + CefURLRequestContextImpl* parent_; + CefRefPtr handler_; + + DISALLOW_COPY_AND_ASSIGN(CefCookieStoreHandlerSource); +}; + +// Sources a cookie store that is created/owned by this object. Life span is +// controlled by the owning URLRequestContext. +class CefCookieStoreOwnerSource : public CefCookieStoreSource { + public: + CefCookieStoreOwnerSource(); + + void SetCookieStoragePath(const base::FilePath& path, + bool persist_session_cookies, + net::NetLog* net_log); + void SetCookieSupportedSchemes(const std::vector& schemes); + + net::CookieStore* GetCookieStore() override; + + private: + std::unique_ptr cookie_store_; + base::FilePath path_; + std::vector supported_schemes_; + + DISALLOW_COPY_AND_ASSIGN(CefCookieStoreOwnerSource); +}; + +#endif // CEF_LIBCEF_BROWSER_COOKIE_STORE_SOURCE_H_ diff --git a/libcef/browser/net/url_request_context_getter_impl.cc b/libcef/browser/net/url_request_context_getter_impl.cc index 9ad59b984..a2a25aea4 100644 --- a/libcef/browser/net/url_request_context_getter_impl.cc +++ b/libcef/browser/net/url_request_context_getter_impl.cc @@ -10,6 +10,8 @@ #include "libcef/browser/content_browser_client.h" #include "libcef/browser/cookie_manager_impl.h" +#include "libcef/browser/net/cookie_store_proxy.h" +#include "libcef/browser/net/cookie_store_source.h" #include "libcef/browser/net/network_delegate.h" #include "libcef/browser/net/scheme_handler.h" #include "libcef/browser/net/url_request_interceptor.h" @@ -18,7 +20,6 @@ #include "libcef/common/content_client.h" #include "base/command_line.h" -#include "base/files/file_util.h" #include "base/logging.h" #include "base/memory/ptr_util.h" #include "base/stl_util.h" @@ -43,7 +44,6 @@ #include "net/cert/multi_log_ct_verifier.h" #include "net/cookies/cookie_monster.h" #include "net/dns/host_resolver.h" -#include "net/extras/sqlite/sqlite_persistent_cookie_store.h" #include "net/ftp/ftp_network_layer.h" #include "net/http/http_auth_handler_factory.h" #include "net/http/http_auth_preferences.h" @@ -473,57 +473,21 @@ void CefURLRequestContextGetterImpl::SetCookieStoragePath( const base::FilePath& path, bool persist_session_cookies) { CEF_REQUIRE_IOT(); - - if (io_state_->url_request_context_->cookie_store() && - ((io_state_->cookie_store_path_.empty() && path.empty()) || - io_state_->cookie_store_path_ == path)) { - // The path has not changed so don't do anything. - return; + if (!io_state_->cookie_source_) { + // Use a proxy because we can't change the URLRequestContext's CookieStore + // during runtime. + io_state_->cookie_source_ = new CefCookieStoreOwnerSource(); + io_state_->storage_->set_cookie_store(std::make_unique( + base::WrapUnique(io_state_->cookie_source_))); } - - scoped_refptr persistent_store; - if (!path.empty()) { - // TODO(cef): Move directory creation to the blocking pool instead of - // allowing file IO on this thread. - base::ThreadRestrictions::ScopedAllowIO allow_io; - if (base::DirectoryExists(path) || base::CreateDirectory(path)) { - const base::FilePath& cookie_path = path.AppendASCII("Cookies"); - persistent_store = new net::SQLitePersistentCookieStore( - cookie_path, BrowserThread::GetTaskRunnerForThread(BrowserThread::IO), - // Intentionally using the background task runner exposed by CEF to - // facilitate unit test expectations. This task runner MUST be - // configured with BLOCK_SHUTDOWN. - CefContentBrowserClient::Get()->background_task_runner(), - persist_session_cookies, NULL); - } else { - NOTREACHED() << "The cookie storage directory could not be created"; - } - } - - // Set the new cookie store that will be used for all new requests. The old - // cookie store, if any, will be automatically flushed and closed when no - // longer referenced. - std::unique_ptr cookie_monster(new net::CookieMonster( - persistent_store.get(), nullptr, io_state_->net_log_)); - if (persistent_store.get() && persist_session_cookies) - cookie_monster->SetPersistSessionCookies(true); - io_state_->cookie_store_path_ = path; - - // Restore the previously supported schemes. - CefCookieManagerImpl::SetCookieMonsterSchemes( - cookie_monster.get(), io_state_->cookie_supported_schemes_); - - io_state_->storage_->set_cookie_store(std::move(cookie_monster)); + io_state_->cookie_source_->SetCookieStoragePath(path, persist_session_cookies, + io_state_->net_log_); } void CefURLRequestContextGetterImpl::SetCookieSupportedSchemes( const std::vector& schemes) { CEF_REQUIRE_IOT(); - - io_state_->cookie_supported_schemes_ = schemes; - CefCookieManagerImpl::SetCookieMonsterSchemes( - static_cast(GetExistingCookieStore()), - io_state_->cookie_supported_schemes_); + io_state_->cookie_source_->SetCookieSupportedSchemes(schemes); } void CefURLRequestContextGetterImpl::AddHandler( @@ -540,9 +504,8 @@ void CefURLRequestContextGetterImpl::AddHandler( net::CookieStore* CefURLRequestContextGetterImpl::GetExistingCookieStore() const { CEF_REQUIRE_IOT(); - if (io_state_->url_request_context_ && - io_state_->url_request_context_->cookie_store()) { - return io_state_->url_request_context_->cookie_store(); + if (io_state_->cookie_source_) { + return io_state_->cookie_source_->GetCookieStore(); } LOG(ERROR) << "Cookie store does not exist"; diff --git a/libcef/browser/net/url_request_context_getter_impl.h b/libcef/browser/net/url_request_context_getter_impl.h index 791ab96a9..038d494e5 100644 --- a/libcef/browser/net/url_request_context_getter_impl.h +++ b/libcef/browser/net/url_request_context_getter_impl.h @@ -22,6 +22,7 @@ #include "content/public/browser/browser_context.h" #include "net/url_request/url_request_job_factory.h" +class CefCookieStoreOwnerSource; class PrefRegistrySimple; class PrefService; @@ -113,8 +114,8 @@ class CefURLRequestContextGetterImpl : public CefURLRequestContextGetter { content::ProtocolHandlerMap protocol_handlers_; content::URLRequestInterceptorScopedVector request_interceptors_; - base::FilePath cookie_store_path_; - std::vector cookie_supported_schemes_; + // Owned by the URLRequestContextStorage. + CefCookieStoreOwnerSource* cookie_source_ = nullptr; std::vector> handler_list_; diff --git a/libcef/browser/net/url_request_context_proxy.cc b/libcef/browser/net/url_request_context_proxy.cc index d1b6b64b8..93731f013 100644 --- a/libcef/browser/net/url_request_context_proxy.cc +++ b/libcef/browser/net/url_request_context_proxy.cc @@ -5,6 +5,7 @@ #include "libcef/browser/net/url_request_context_proxy.h" #include "libcef/browser/net/cookie_store_proxy.h" +#include "libcef/browser/net/cookie_store_source.h" #include "libcef/browser/net/url_request_context_impl.h" #include "libcef/browser/thread_util.h" @@ -16,7 +17,8 @@ CefURLRequestContextProxy::CefURLRequestContextProxy( DCHECK(handler.get()); // Cookie store that proxies to the browser implementation. - cookie_store_proxy_.reset(new CefCookieStoreProxy(parent, handler)); + cookie_store_proxy_.reset(new CefCookieStoreProxy( + std::make_unique(parent, handler))); set_cookie_store(cookie_store_proxy_.get()); // All other values refer to the parent request context.