Fix recreation of cookie store and channel id service

As more things gets mojofy and become services,
we can not simply delete parts of storage context.
Therefore regenerate whole storage in case of cookie
store, cache, agent settings updates. URLRequestContextStorage
guards correct order of destruction.

This change aims to fix some crashes during cookie store and
http cache recreation when ChannelIDService gets deleted.
Depending on timing this ends with different stacktraces like
in cookie store manager or during ssl handshake.

We still keep network delegate outside of storage context to
prevent crashes when url requests end their life due
to (forced) request cancellation in cancelAllUrlRequests().

Please note this is just band-aid patch, and introduction of
'profile builder' will be a proper fix.

Fixes: QTBUG-71895
Change-Id: If33a7af3ebba2632ea33f32d913e1c21a4534817
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

2 files changed, 57 insertions, 91 deletions

diff --git a/src/core/profile_io_data_qt.cpp b/src/core/profile_io_data_qt.cpp
index 7783f1ae7..5603d19b9 100644
--- a/src/core/profile_io_data_qt.cpp
+++ b/src/core/profile_io_data_qt.cpp
@@ -210,9 +210,9 @@ void ProfileIODataQt::initializeOnIOThread()
     // this binds factory to io thread
     m_weakPtr = m_weakPtrFactory.GetWeakPtr();
     QMutexLocker lock(&m_mutex);
-    m_initialized = true;
     generateAllStorage();
     generateJobFactory();
+    m_initialized = true;
 }
 
 void ProfileIODataQt::initializeOnUIThread()
@@ -227,6 +227,7 @@ void ProfileIODataQt::initializeOnUIThread()
         protocolHandlerRegistry->CreateJobInterceptorFactory();
     m_cookieDelegate = new CookieMonsterDelegateQt();
     m_cookieDelegate->setClient(m_profile->profileAdapter()->cookieStore());
+    createProxyConfig();
 }
 
 void ProfileIODataQt::cancelAllUrlRequests()
@@ -262,11 +263,11 @@ void ProfileIODataQt::generateStorage()
 
     // We must stop all requests before deleting their backends.
     if (m_storage) {
-        m_cookieDelegate->setCookieMonster(0);
-        m_storage->set_cookie_store(0);
+        m_cookieDelegate->setCookieMonster(nullptr);
+        m_storage->set_cookie_store(nullptr);
         cancelAllUrlRequests();
         // we need to get rid of dangling pointer due to coming storage deletion
-        m_urlRequestContext->set_http_transaction_factory(0);
+        m_urlRequestContext->set_http_transaction_factory(nullptr);
         m_httpNetworkSession.reset();
         m_transportSecurityPersister.reset();
     }
@@ -282,25 +283,10 @@ void ProfileIODataQt::generateStorage()
 //    ct_verifier->AddLogs(net::ct::CreateLogVerifiersForKnownLogs());
     m_storage->set_cert_transparency_verifier(std::move(ct_verifier));
     m_storage->set_ct_policy_enforcer(base::WrapUnique(new net::DefaultCTPolicyEnforcer()));
-
-    std::unique_ptr<net::HostResolver> host_resolver(net::HostResolver::CreateDefaultResolver(NULL));
-
-    // The System Proxy Resolver has issues on Windows with unconfigured network cards,
-    // which is why we want to use the v8 one
-    if (!m_dhcpPacFileFetcherFactory)
-        m_dhcpPacFileFetcherFactory.reset(new net::DhcpPacFileFetcherFactory);
-
-    proxy_resolver::mojom::ProxyResolverFactoryPtr proxyResolver(std::move(m_proxyResolverFactoryInterface));
-    m_storage->set_proxy_resolution_service(network::CreateProxyResolutionServiceUsingMojoFactory(
-                                                std::move(proxyResolver),
-                                                std::unique_ptr<net::ProxyConfigService>(proxyConfigService),
-                                                net::PacFileFetcherImpl::CreateWithFileUrlSupport(m_urlRequestContext.get()),
-                                                m_dhcpPacFileFetcherFactory->Create(m_urlRequestContext.get()),
-                                                host_resolver.get(),
-                                                nullptr /* NetLog */,
-                                                m_networkDelegate.get()));
-
+    m_storage->set_host_resolver(net::HostResolver::CreateDefaultResolver(NULL));
     m_storage->set_ssl_config_service(std::make_unique<net::SSLConfigServiceDefaults>());
+    m_storage->set_http_auth_handler_factory(net::HttpAuthHandlerFactory::CreateDefault(
+                                                 m_urlRequestContext->host_resolver()));
     m_storage->set_transport_security_state(std::make_unique<net::TransportSecurityState>());
 
     if (!m_dataPath.isEmpty()) {
@@ -316,20 +302,23 @@ void ProfileIODataQt::generateStorage()
                     background_task_runner);
     };
 
-    if (!m_httpAuthPreferences)
-        m_httpAuthPreferences.reset(new net::HttpAuthPreferences());
-
-    m_storage->set_http_auth_handler_factory(net::HttpAuthHandlerFactory::CreateDefault(
-                                                 host_resolver.get(),
-                                                 m_httpAuthPreferences.get()
-#if (defined(OS_POSIX) && !defined(OS_ANDROID)) || defined(OS_FUCHSIA)
-                                                 , std::string() /* gssapi library name */
-#endif
-                                            ));
     m_storage->set_http_server_properties(std::unique_ptr<net::HttpServerProperties>(
                                               new net::HttpServerPropertiesImpl));
-     // Give |m_storage| ownership at the end in case it's |mapped_host_resolver|.
-    m_storage->set_host_resolver(std::move(host_resolver));
+
+    // The System Proxy Resolver has issues on Windows with unconfigured network cards,
+    // which is why we want to use the v8 one
+    if (!m_dhcpPacFileFetcherFactory)
+        m_dhcpPacFileFetcherFactory.reset(new net::DhcpPacFileFetcherFactory);
+
+    proxy_resolver::mojom::ProxyResolverFactoryPtr proxyResolver(std::move(m_proxyResolverFactoryInterface));
+    m_storage->set_proxy_resolution_service(network::CreateProxyResolutionServiceUsingMojoFactory(
+                                                std::move(proxyResolver),
+                                                std::unique_ptr<net::ProxyConfigService>(proxyConfigService),
+                                                net::PacFileFetcherImpl::CreateWithFileUrlSupport(m_urlRequestContext.get()),
+                                                m_dhcpPacFileFetcherFactory->Create(m_urlRequestContext.get()),
+                                                m_urlRequestContext->host_resolver(),
+                                                nullptr /* NetLog */,
+                                                m_urlRequestContext->network_delegate()));
 }
 
 
@@ -337,10 +326,8 @@ void ProfileIODataQt::generateCookieStore()
 {
     Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
     Q_ASSERT(m_urlRequestContext);
-    Q_ASSERT(m_storage);
 
     QMutexLocker lock(&m_mutex);
-    m_updateCookieStore = false;
 
     scoped_refptr<net::SQLiteChannelIDStore> channel_id_db;
     if (!m_channelIdPath.isEmpty() && m_persistentCookiesPolicy != ProfileAdapter::NoPersistentCookies) {
@@ -354,11 +341,6 @@ void ProfileIODataQt::generateCookieStore()
             base::WrapUnique(new net::ChannelIDService(
                     new net::DefaultChannelIDStore(channel_id_db.get()))));
 
-    // 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_cookieDelegate->setCookieMonster(0);
-    m_storage->set_cookie_store(0);
-
     std::unique_ptr<net::CookieStore> cookieStore;
     switch (m_persistentCookiesPolicy) {
     case ProfileAdapter::NoPersistentCookies:
@@ -398,11 +380,6 @@ void ProfileIODataQt::generateCookieStore()
     const std::vector<std::string> cookieableSchemes(kCookieableSchemes,
                                                      kCookieableSchemes + arraysize(kCookieableSchemes));
     cookieMonster->SetCookieableSchemes(cookieableSchemes);
-
-    if (!m_updateAllStorage && m_updateHttpCache) {
-        // HttpCache needs to be regenerated when we generate a new channel id service
-        generateHttpCache();
-    }
 }
 
 void ProfileIODataQt::generateUserAgent()
@@ -412,8 +389,6 @@ void ProfileIODataQt::generateUserAgent()
     Q_ASSERT(m_storage);
 
     QMutexLocker lock(&m_mutex);
-    m_updateUserAgent = false;
-
     m_storage->set_http_user_agent_settings(std::unique_ptr<net::HttpUserAgentSettings>(
         new net::StaticHttpUserAgentSettings(m_httpAcceptLanguage.toStdString(),
                                              m_httpUserAgent.toStdString())));
@@ -426,10 +401,6 @@ void ProfileIODataQt::generateHttpCache()
     Q_ASSERT(m_storage);
 
     QMutexLocker lock(&m_mutex);
-    m_updateHttpCache = false;
-
-    if (m_updateCookieStore)
-        generateCookieStore();
 
     net::HttpCache::DefaultBackend* main_backend = 0;
     switch (m_httpCacheType) {
@@ -583,6 +554,34 @@ void ProfileIODataQt::setFullConfiguration()
     m_dataPath = m_profileAdapter->dataPath();
 }
 
+void ProfileIODataQt::requestStorageGeneration() {
+    Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    QMutexLocker lock(&m_mutex);
+    if (m_initialized && !m_updateAllStorage) {
+        m_updateAllStorage = true;
+        createProxyConfig();
+        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                         base::Bind(&ProfileIODataQt::generateAllStorage, m_weakPtr));
+    }
+}
+
+// TODO(miklocek): mojofy ProxyConfigServiceQt
+void ProfileIODataQt::createProxyConfig()
+{
+    Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+    QMutexLocker lock(&m_mutex);
+    // 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().
+    Q_ASSERT(m_proxyConfigService == 0);
+    m_proxyConfigService =
+            new ProxyConfigServiceQt(
+                net::ProxyResolutionService::CreateSystemProxyConfigService(
+                    content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::IO)));
+    //pass interface to io thread
+    m_proxyResolverFactoryInterface = ChromeMojoProxyResolverFactory::CreateWithStrongBinding().PassInterface();
+}
+
 void ProfileIODataQt::updateStorageSettings()
 {
     Q_ASSERT(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ -595,23 +594,7 @@ void ProfileIODataQt::updateStorageSettings()
         file::ForgetServiceUserIdUserDirAssociation(userId);
         file::AssociateServiceUserIdWithUserDir(userId, toFilePath(m_profileAdapter->dataPath()));
     }
-
-    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().
-        Q_ASSERT(m_proxyConfigService == 0);
-        m_proxyConfigService =
-                new ProxyConfigServiceQt(
-                    net::ProxyResolutionService::CreateSystemProxyConfigService(
-                        content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::IO)));
-        //pass interface to io thread
-        m_proxyResolverFactoryInterface = ChromeMojoProxyResolverFactory::CreateWithStrongBinding().PassInterface();
-        if (m_initialized)
-            content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                             base::Bind(&ProfileIODataQt::generateAllStorage, m_weakPtr));
-    }
+    requestStorageGeneration();
 }
 
 void ProfileIODataQt::updateCookieStore()
@@ -621,13 +604,7 @@ void ProfileIODataQt::updateCookieStore()
     m_persistentCookiesPolicy = m_profileAdapter->persistentCookiesPolicy();
     m_cookiesPath = m_profileAdapter->cookiesPath();
     m_channelIdPath = m_profileAdapter->channelIdPath();
-
-    if (m_initialized && !m_updateAllStorage && !m_updateCookieStore) {
-        m_updateCookieStore = true;
-        m_updateHttpCache = true;
-        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                         base::Bind(&ProfileIODataQt::generateCookieStore, m_weakPtr));
-    }
+    requestStorageGeneration();
 }
 
 void ProfileIODataQt::updateUserAgent()
@@ -636,12 +613,7 @@ void ProfileIODataQt::updateUserAgent()
     QMutexLocker lock(&m_mutex);
     m_httpAcceptLanguage = m_profileAdapter->httpAcceptLanguage();
     m_httpUserAgent = m_profileAdapter->httpUserAgent();
-
-    if (m_initialized && !m_updateAllStorage && !m_updateUserAgent) {
-        m_updateUserAgent = true;
-        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                         base::Bind(&ProfileIODataQt::generateUserAgent, m_weakPtr));
-    }
+    requestStorageGeneration();
 }
 
 void ProfileIODataQt::updateHttpCache()
@@ -660,12 +632,7 @@ void ProfileIODataQt::updateHttpCache()
             content::BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB |
                         content::BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB);
     }
-
-    if (m_initialized && !m_updateAllStorage && !m_updateHttpCache) {
-        m_updateHttpCache = true;
-        content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
-                                         base::Bind(&ProfileIODataQt::generateHttpCache, m_weakPtr));
-    }
+    requestStorageGeneration();
 }
 
 void ProfileIODataQt::updateJobFactory()
diff --git a/src/core/profile_io_data_qt.h b/src/core/profile_io_data_qt.h
index 5b416861c..2d4706bf4 100644
--- a/src/core/profile_io_data_qt.h
+++ b/src/core/profile_io_data_qt.h
@@ -105,6 +105,8 @@ public:
     void updateHttpCache(); // runs on ui thread
     void updateJobFactory(); // runs on ui thread
     void updateRequestInterceptor(); // runs on ui thread
+    void requestStorageGeneration(); //runs on ui thread
+    void createProxyConfig(); //runs on ui thread
 
 private:
     ProfileQt *m_profile;
@@ -140,10 +142,7 @@ private:
     int m_httpCacheMaxSize = 0;
     bool m_initialized = false;
     bool m_updateAllStorage = false;
-    bool m_updateCookieStore = false;
-    bool m_updateHttpCache = false;
     bool m_updateJobFactory = false;
-    bool m_updateUserAgent = false;
     bool m_ignoreCertificateErrors = false;
     base::WeakPtrFactory<ProfileIODataQt> m_weakPtrFactory; // this should be always the last member
     QString m_dataPath;