From d107266cd403416d622948186968106a9cf78590 Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Thu, 28 Apr 2016 12:59:40 +0200 Subject: Fix threading issues with URLRequestContext URLRequestContextGetterQt contains data which is shared between UI and IO thread. Make the class more thread friendly by making copies of the data that can be accessed from the IO thread, and protect synchronization with a full mutex instead of atomics. Also fixes circular reference between URLRequestContextGetterQt and BrowserContextAdapter. Task-number: QTBUG-50160 Task-number: QTBUG-52509 Change-Id: Idaba211533cfad229e1d1872cdfdf4e7dffeb3d8 Reviewed-by: Joerg Bornemann --- src/core/browser_context_adapter.cpp | 2 + src/core/network_delegate_qt.cpp | 3 +- src/core/url_request_context_getter_qt.cpp | 171 +++++++++++++++++++++-------- src/core/url_request_context_getter_qt.h | 39 +++++-- 4 files changed, 162 insertions(+), 53 deletions(-) diff --git a/src/core/browser_context_adapter.cpp b/src/core/browser_context_adapter.cpp index c7118b4f7..481197eed 100644 --- a/src/core/browser_context_adapter.cpp +++ b/src/core/browser_context_adapter.cpp @@ -148,6 +148,8 @@ QWebEngineUrlRequestInterceptor *BrowserContextAdapter::requestInterceptor() void BrowserContextAdapter::setRequestInterceptor(QWebEngineUrlRequestInterceptor *interceptor) { m_requestInterceptor = interceptor; + if (m_browserContext->url_request_getter_.get()) + m_browserContext->url_request_getter_->updateRequestInterceptor(); } void BrowserContextAdapter::addClient(BrowserContextAdapterClient *adapterClient) diff --git a/src/core/network_delegate_qt.cpp b/src/core/network_delegate_qt.cpp index 8420d7f99..fd79917f4 100644 --- a/src/core/network_delegate_qt.cpp +++ b/src/core/network_delegate_qt.cpp @@ -87,7 +87,6 @@ int NetworkDelegateQt::OnBeforeURLRequest(net::URLRequest *request, const net::C { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); Q_ASSERT(m_requestContextGetter); - Q_ASSERT(m_requestContextGetter->m_browserContext); const content::ResourceRequestInfo *resourceInfo = content::ResourceRequestInfo::ForRequest(request); @@ -101,7 +100,7 @@ int NetworkDelegateQt::OnBeforeURLRequest(net::URLRequest *request, const net::C const QUrl qUrl = toQt(request->url()); - QWebEngineUrlRequestInterceptor* interceptor = m_requestContextGetter->m_browserContext->requestInterceptor(); + QWebEngineUrlRequestInterceptor* interceptor = m_requestContextGetter->m_requestInterceptor; if (interceptor) { QWebEngineUrlRequestInfoPrivate *infoPrivate = new QWebEngineUrlRequestInfoPrivate(static_cast(resourceType) , static_cast(navigationType) diff --git a/src/core/url_request_context_getter_qt.cpp b/src/core/url_request_context_getter_qt.cpp index aa35485ae..f7c9e4600 100644 --- a/src/core/url_request_context_getter_qt.cpp +++ b/src/core/url_request_context_getter_qt.cpp @@ -87,14 +87,23 @@ using content::BrowserThread; URLRequestContextGetterQt::URLRequestContextGetterQt(QSharedPointer browserContext, content::ProtocolHandlerMap *protocolHandlers, content::URLRequestInterceptorScopedVector request_interceptors) : m_ignoreCertificateErrors(false) + , m_mutex(QMutex::Recursive) + , m_contextInitialized(false) + , m_updateAllStorage(false) + , m_updateCookieStore(false) + , m_updateHttpCache(false) + , m_updateJobFactory(true) + , m_updateUserAgent(false) , m_browserContext(browserContext) , m_baseJobFactory(0) , m_cookieDelegate(new CookieMonsterDelegateQt()) , m_requestInterceptors(request_interceptors.Pass()) { std::swap(m_protocolHandlers, *protocolHandlers); - m_cookieDelegate->setClient(m_browserContext->cookieStore()); + QMutexLocker lock(&m_mutex); + m_cookieDelegate->setClient(browserContext->cookieStore()); + setFullConfiguration(browserContext); updateStorageSettings(); } @@ -104,6 +113,23 @@ URLRequestContextGetterQt::~URLRequestContextGetterQt() delete m_proxyConfigService.fetchAndStoreAcquire(0); } + +void URLRequestContextGetterQt::setFullConfiguration(QSharedPointer browserContext) +{ + if (!browserContext) + return; + + m_requestInterceptor = browserContext->requestInterceptor(); + m_persistentCookiesPolicy = browserContext->persistentCookiesPolicy(); + m_cookiesPath = browserContext->cookiesPath(); + m_httpAcceptLanguage = browserContext->httpAcceptLanguage(); + m_httpUserAgent = browserContext->httpUserAgent(); + m_httpCacheType = browserContext->httpCacheType(); + m_httpCachePath = browserContext->httpCachePath(); + m_httpCacheMaxSize = browserContext->httpCacheMaxSize(); + m_customUrlSchemes = browserContext->customUrlSchemes(); +} + net::URLRequestContext *URLRequestContextGetterQt::GetURLRequestContext() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); @@ -113,8 +139,10 @@ net::URLRequestContext *URLRequestContextGetterQt::GetURLRequestContext() m_networkDelegate.reset(new NetworkDelegateQt(this)); m_urlRequestContext->set_network_delegate(m_networkDelegate.get()); - generateStorage(); + QMutexLocker lock(&m_mutex); + generateAllStorage(); generateJobFactory(); + m_contextInitialized = true; } return m_urlRequestContext.get(); @@ -123,24 +151,31 @@ net::URLRequestContext *URLRequestContextGetterQt::GetURLRequestContext() void URLRequestContextGetterQt::updateStorageSettings() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - // m_proxyConfigService having a non-null value is used to indicate - // storage is already being updated. - if (!m_proxyConfigService.loadAcquire()) { + + QMutexLocker lock(&m_mutex); + setFullConfiguration(m_browserContext.toStrongRef()); + + if (!m_updateAllStorage) { + m_updateAllStorage = true; // We must create the proxy config service on the UI loop on Linux because it // must synchronously run on the glib message loop. This will be passed to // the URLRequestContextStorage on the IO thread in GetURLRequestContext(). - m_proxyConfigService.storeRelease(new ProxyConfigServiceQt(net::ProxyService::CreateSystemProxyConfigService( - content::BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), - content::BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE) - ))); - if (m_storage) { - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestContextGetterQt::generateStorage, this)); - } + Q_ASSERT(m_proxyConfigService == 0); + m_proxyConfigService = + new ProxyConfigServiceQt( + net::ProxyService::CreateSystemProxyConfigService( + content::BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), + content::BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE) + )); + if (m_contextInitialized) + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, + base::Bind(&URLRequestContextGetterQt::generateAllStorage, this)); } } void URLRequestContextGetterQt::cancelAllUrlRequests() { + Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); Q_ASSERT(m_urlRequestContext); std::set* url_requests = m_urlRequestContext->url_requests(); @@ -154,6 +189,17 @@ void URLRequestContextGetterQt::cancelAllUrlRequests() } +void URLRequestContextGetterQt::generateAllStorage() +{ + Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); + QMutexLocker lock(&m_mutex); + generateStorage(); + generateCookieStore(); + generateUserAgent(); + generateHttpCache(); + m_updateAllStorage = false; +} + void URLRequestContextGetterQt::generateStorage() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); @@ -171,8 +217,6 @@ void URLRequestContextGetterQt::generateStorage() net::ProxyConfigService *proxyConfigService = m_proxyConfigService.fetchAndStoreAcquire(0); Q_ASSERT(proxyConfigService); - generateCookieStore(); - generateUserAgent(); m_storage->set_channel_id_service(scoped_ptr(new net::ChannelIDService( new net::DefaultChannelIDStore(NULL), @@ -203,16 +247,20 @@ void URLRequestContextGetterQt::generateStorage() // Give |m_storage| ownership at the end in case it's |mapped_host_resolver|. m_storage->set_host_resolver(host_resolver.Pass()); - - generateHttpCache(); } void URLRequestContextGetterQt::updateCookieStore() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - // Do not trigger an update if another is already triggered, or we are not yet initialized. - if (m_urlRequestContext && !m_proxyConfigService && !m_updateCookieStore.fetchAndStoreRelaxed(1)) - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestContextGetterQt::generateCookieStore, this)); + QMutexLocker lock(&m_mutex); + m_httpAcceptLanguage = m_browserContext.data()->httpAcceptLanguage(); + m_httpUserAgent = m_browserContext.data()->httpUserAgent(); + + if (m_contextInitialized && !m_updateAllStorage && !m_updateCookieStore) { + m_updateCookieStore = true; + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, + base::Bind(&URLRequestContextGetterQt::generateCookieStore, this)); + } } void URLRequestContextGetterQt::generateCookieStore() @@ -220,14 +268,16 @@ void URLRequestContextGetterQt::generateCookieStore() Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); Q_ASSERT(m_urlRequestContext); Q_ASSERT(m_storage); - m_updateCookieStore = 0; + + QMutexLocker lock(&m_mutex); + m_updateCookieStore = false; // Unset it first to get a chance to destroy and flush the old cookie store before opening a new on possibly the same file. m_storage->set_cookie_store(0); m_cookieDelegate->setCookieMonster(0); net::CookieStore* cookieStore = 0; - switch (m_browserContext->persistentCookiesPolicy()) { + switch (m_persistentCookiesPolicy) { case BrowserContextAdapter::NoPersistentCookies: cookieStore = content::CreateCookieStore(content::CookieStoreConfig( @@ -240,7 +290,7 @@ void URLRequestContextGetterQt::generateCookieStore() case BrowserContextAdapter::AllowPersistentCookies: cookieStore = content::CreateCookieStore(content::CookieStoreConfig( - toFilePath(m_browserContext->cookiesPath()), + toFilePath(m_cookiesPath), content::CookieStoreConfig::PERSISTANT_SESSION_COOKIES, NULL, m_cookieDelegate.get()) @@ -249,7 +299,7 @@ void URLRequestContextGetterQt::generateCookieStore() case BrowserContextAdapter::ForcePersistentCookies: cookieStore = content::CreateCookieStore(content::CookieStoreConfig( - toFilePath(m_browserContext->cookiesPath()), + toFilePath(m_cookiesPath), content::CookieStoreConfig::RESTORED_SESSION_COOKIES, NULL, m_cookieDelegate.get()) @@ -266,9 +316,15 @@ void URLRequestContextGetterQt::generateCookieStore() void URLRequestContextGetterQt::updateUserAgent() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - // Do not trigger an update if all storage settings are already being updated - if (m_urlRequestContext && !m_proxyConfigService) - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestContextGetterQt::generateUserAgent, this)); + QMutexLocker lock(&m_mutex); + m_httpAcceptLanguage = m_browserContext.data()->httpAcceptLanguage(); + m_httpUserAgent = m_browserContext.data()->httpUserAgent(); + + if (m_contextInitialized && !m_updateAllStorage && !m_updateUserAgent) { + m_updateUserAgent = true; + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, + base::Bind(&URLRequestContextGetterQt::generateUserAgent, this)); + } } void URLRequestContextGetterQt::generateUserAgent() @@ -277,26 +333,48 @@ void URLRequestContextGetterQt::generateUserAgent() Q_ASSERT(m_urlRequestContext); Q_ASSERT(m_storage); + QMutexLocker lock(&m_mutex); + m_updateUserAgent = true; + m_storage->set_http_user_agent_settings( - new net::StaticHttpUserAgentSettings(m_browserContext->httpAcceptLanguage().toStdString(), m_browserContext->httpUserAgent().toStdString())); + new net::StaticHttpUserAgentSettings(m_httpAcceptLanguage.toStdString(), m_httpUserAgent.toStdString())); } void URLRequestContextGetterQt::updateHttpCache() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - // Do not trigger a new update if another is already triggered - if (m_urlRequestContext && !m_proxyConfigService && !m_updateHttpCache.fetchAndStoreRelaxed(1)) - content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, base::Bind(&URLRequestContextGetterQt::generateHttpCache, this)); + QMutexLocker lock(&m_mutex); + m_httpCacheType = m_browserContext.data()->httpCacheType(); + m_httpCachePath = m_browserContext.data()->httpCachePath(); + m_httpCacheMaxSize = m_browserContext.data()->httpCacheMaxSize(); + + if (m_contextInitialized && !m_updateAllStorage && !m_updateHttpCache) { + m_updateHttpCache = true; + content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, + base::Bind(&URLRequestContextGetterQt::generateHttpCache, this)); + } } void URLRequestContextGetterQt::updateJobFactory() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + QMutexLocker lock(&m_mutex); + m_customUrlSchemes = m_browserContext.data()->customUrlSchemes(); - if (m_urlRequestContext && !m_updateJobFactory.fetchAndStoreRelaxed(1)) + if (m_contextInitialized && !m_updateJobFactory) { + m_updateJobFactory = true; content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE, - base::Bind(&URLRequestContextGetterQt::regenerateJobFactory, - this, m_browserContext->customUrlSchemes())); + base::Bind(&URLRequestContextGetterQt::regenerateJobFactory, this)); + } +} + +void URLRequestContextGetterQt::updateRequestInterceptor() +{ + Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + QMutexLocker lock(&m_mutex); + m_requestInterceptor = m_browserContext.data()->requestInterceptor(); + + // We in this case do not need to regenerate any Chromium classes. } static bool doNetworkSessionParamsMatch(const net::HttpNetworkSession::Params &first, const net::HttpNetworkSession::Params &second) @@ -351,15 +429,18 @@ void URLRequestContextGetterQt::generateHttpCache() Q_ASSERT(m_urlRequestContext); Q_ASSERT(m_storage); + QMutexLocker lock(&m_mutex); + m_updateHttpCache = false; + net::HttpCache::DefaultBackend* main_backend = 0; - switch (m_browserContext->httpCacheType()) { + switch (m_httpCacheType) { case BrowserContextAdapter::MemoryHttpCache: main_backend = new net::HttpCache::DefaultBackend( net::MEMORY_CACHE, net::CACHE_BACKEND_DEFAULT, base::FilePath(), - m_browserContext->httpCacheMaxSize(), + m_httpCacheMaxSize, BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE) ); break; @@ -368,8 +449,8 @@ void URLRequestContextGetterQt::generateHttpCache() new net::HttpCache::DefaultBackend( net::DISK_CACHE, net::CACHE_BACKEND_DEFAULT, - toFilePath(m_browserContext->httpCachePath()), - m_browserContext->httpCacheMaxSize(), + toFilePath(m_httpCachePath), + m_httpCacheMaxSize, BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE) ); break; @@ -389,7 +470,6 @@ void URLRequestContextGetterQt::generateHttpCache() cache = new net::HttpCache(network_session, main_backend); m_storage->set_http_transaction_factory(cache); - m_updateHttpCache = 0; } void URLRequestContextGetterQt::generateJobFactory() @@ -398,6 +478,9 @@ void URLRequestContextGetterQt::generateJobFactory() Q_ASSERT(m_urlRequestContext); Q_ASSERT(!m_jobFactory); + QMutexLocker lock(&m_mutex); + m_updateJobFactory = false; + scoped_ptr jobFactory(new net::URLRequestJobFactoryImpl()); { @@ -416,7 +499,7 @@ void URLRequestContextGetterQt::generateJobFactory() jobFactory->SetProtocolHandler(url::kFtpScheme, new net::FtpProtocolHandler(new net::FtpNetworkLayer(m_urlRequestContext->host_resolver()))); - m_installedCustomSchemes = m_browserContext->customUrlSchemes(); + m_installedCustomSchemes = m_customUrlSchemes; Q_FOREACH (const QByteArray &scheme, m_installedCustomSchemes) { jobFactory->SetProtocolHandler(scheme.toStdString(), new CustomProtocolHandler(m_browserContext)); } @@ -436,22 +519,24 @@ void URLRequestContextGetterQt::generateJobFactory() m_urlRequestContext->set_job_factory(m_jobFactory.get()); } -void URLRequestContextGetterQt::regenerateJobFactory(const QList customSchemes) +void URLRequestContextGetterQt::regenerateJobFactory() { Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); Q_ASSERT(m_urlRequestContext); Q_ASSERT(m_jobFactory); Q_ASSERT(m_baseJobFactory); - m_updateJobFactory.storeRelease(0); - if (customSchemes == m_installedCustomSchemes) + QMutexLocker lock(&m_mutex); + m_updateJobFactory = false; + + if (m_customUrlSchemes == m_installedCustomSchemes) return; Q_FOREACH (const QByteArray &scheme, m_installedCustomSchemes) { m_baseJobFactory->SetProtocolHandler(scheme.toStdString(), nullptr); } - m_installedCustomSchemes = customSchemes; + m_installedCustomSchemes = m_customUrlSchemes; Q_FOREACH (const QByteArray &scheme, m_installedCustomSchemes) { m_baseJobFactory->SetProtocolHandler(scheme.toStdString(), new CustomProtocolHandler(m_browserContext)); } diff --git a/src/core/url_request_context_getter_qt.h b/src/core/url_request_context_getter_qt.h index b925232a6..11c3f4e79 100644 --- a/src/core/url_request_context_getter_qt.h +++ b/src/core/url_request_context_getter_qt.h @@ -52,9 +52,10 @@ #include "cookie_monster_delegate_qt.h" #include "network_delegate_qt.h" +#include "browser_context_adapter.h" -#include #include +#include #include namespace net { @@ -64,8 +65,7 @@ class ProxyConfigService; namespace QtWebEngineCore { -class BrowserContextAdapter; - +// FIXME: This class should be split into a URLRequestContextGetter and a ProfileIOData, similar to what chrome does. class URLRequestContextGetterQt : public net::URLRequestContextGetter { public: URLRequestContextGetterQt(QSharedPointer browserContext, content::ProtocolHandlerMap *protocolHandlers, content::URLRequestInterceptorScopedVector request_interceptors); @@ -79,25 +79,35 @@ public: void updateCookieStore(); void updateHttpCache(); void updateJobFactory(); + void updateRequestInterceptor(); private: virtual ~URLRequestContextGetterQt(); // Called on the IO thread: + void generateAllStorage(); void generateStorage(); void generateCookieStore(); void generateHttpCache(); void generateUserAgent(); void generateJobFactory(); - void regenerateJobFactory(const QList customSchemes); + void regenerateJobFactory(); void cancelAllUrlRequests(); net::HttpNetworkSession::Params generateNetworkSessionParams(); + void setFullConfiguration(QSharedPointer browserContext); + bool m_ignoreCertificateErrors; - QAtomicInt m_updateCookieStore; - QAtomicInt m_updateHttpCache; - QAtomicInt m_updateJobFactory; - QSharedPointer m_browserContext; + + QMutex m_mutex; + bool m_contextInitialized; + bool m_updateAllStorage; + bool m_updateCookieStore; + bool m_updateHttpCache; + bool m_updateJobFactory; + bool m_updateUserAgent; + + QWeakPointer m_browserContext; content::ProtocolHandlerMap m_protocolHandlers; QAtomicPointer m_proxyConfigService; @@ -109,7 +119,20 @@ private: scoped_ptr m_dhcpProxyScriptFetcherFactory; scoped_refptr m_cookieDelegate; content::URLRequestInterceptorScopedVector m_requestInterceptors; + QList m_installedCustomSchemes; + QWebEngineUrlRequestInterceptor* m_requestInterceptor; + + // Configuration values to setup URLRequestContext in IO thread, copied from browserContext + // FIXME: Should later be moved to a separate ProfileIOData class. + BrowserContextAdapter::PersistentCookiesPolicy m_persistentCookiesPolicy; + QString m_cookiesPath; + QString m_httpAcceptLanguage; + QString m_httpUserAgent; + BrowserContextAdapter::HttpCacheType m_httpCacheType; + QString m_httpCachePath; + int m_httpCacheMaxSize; + QList m_customUrlSchemes; friend class NetworkDelegateQt; }; -- cgit v1.2.3