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 <joerg.bornemann@theqtcompany.com>

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<QWebEngineUrlRequestInfo::ResourceType>(resourceType)
                                                                                            , static_cast<QWebEngineUrlRequestInfo::NavigationType>(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<BrowserContextAdapter> 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<BrowserContextAdapter> 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<const net::URLRequest*>* 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<net::ChannelIDService>(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<net::URLRequestJobFactoryImpl> 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<QByteArray> 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 <QtCore/qglobal.h>
 #include <QtCore/qatomic.h>
+#include <QtCore/qmutex.h>
 #include <QtCore/qsharedpointer.h>
 
 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<BrowserContextAdapter> 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<QByteArray> customSchemes);
+    void regenerateJobFactory();
     void cancelAllUrlRequests();
     net::HttpNetworkSession::Params generateNetworkSessionParams();
 
+    void setFullConfiguration(QSharedPointer<BrowserContextAdapter> browserContext);
+
     bool m_ignoreCertificateErrors;
-    QAtomicInt m_updateCookieStore;
-    QAtomicInt m_updateHttpCache;
-    QAtomicInt m_updateJobFactory;
-    QSharedPointer<BrowserContextAdapter> m_browserContext;
+
+    QMutex m_mutex;
+    bool m_contextInitialized;
+    bool m_updateAllStorage;
+    bool m_updateCookieStore;
+    bool m_updateHttpCache;
+    bool m_updateJobFactory;
+    bool m_updateUserAgent;
+
+    QWeakPointer<BrowserContextAdapter> m_browserContext;
     content::ProtocolHandlerMap m_protocolHandlers;
 
     QAtomicPointer<net::ProxyConfigService> m_proxyConfigService;
@@ -109,7 +119,20 @@ private:
     scoped_ptr<net::DhcpProxyScriptFetcherFactory> m_dhcpProxyScriptFetcherFactory;
     scoped_refptr<CookieMonsterDelegateQt> m_cookieDelegate;
     content::URLRequestInterceptorScopedVector m_requestInterceptors;
+
     QList<QByteArray> 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<QByteArray> m_customUrlSchemes;
 
     friend class NetworkDelegateQt;
 };