Fix access after free on shutdown

After we keep around the browser-context after the profile is deleted
it was keeping pointers to deleted objects and would sometimes use them
on shutdown.

Change-Id: Ib67d0ee0b27cb1a1b64d9b8b4c348ed418b9bbc3
Reviewed-by: Peter Varga <pvarga@inf.u-szeged.hu>
Reviewed-by: Michal Klocek <michal.klocek@qt.io>

6 files changed, 44 insertions, 9 deletions

diff --git a/src/core/browser_context_adapter.cpp b/src/core/browser_context_adapter.cpp
index bec76ad81..41b5b1932 100644
--- a/src/core/browser_context_adapter.cpp
+++ b/src/core/browser_context_adapter.cpp
@@ -41,6 +41,7 @@
 
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_manager.h"
 
 #include "browser_context_qt.h"
 #include "content_client_qt.h"
@@ -101,9 +102,16 @@ BrowserContextAdapter::BrowserContextAdapter(const QString &storageName)
 
 BrowserContextAdapter::~BrowserContextAdapter()
 {
+    Q_ASSERT(!m_downloadManagerDelegate);
+}
+
+void BrowserContextAdapter::shutdown()
+{
     m_browserContext->ShutdownStoragePartitions();
-    if (m_downloadManagerDelegate)
-        content::BrowserThread::DeleteSoon(content::BrowserThread::UI, FROM_HERE, m_downloadManagerDelegate.take());
+    if (m_downloadManagerDelegate) {
+        m_browserContext->GetDownloadManager(m_browserContext.data())->Shutdown();
+        m_downloadManagerDelegate.reset();
+    }
     BrowserContextDependencyManager::GetInstance()->DestroyBrowserContextServices(m_browserContext.data());
 }
 
@@ -177,6 +185,8 @@ void BrowserContextAdapter::addClient(BrowserContextAdapterClient *adapterClient
 void BrowserContextAdapter::removeClient(BrowserContextAdapterClient *adapterClient)
 {
     m_clients.removeOne(adapterClient);
+    if (m_clients.isEmpty() && this != WebEngineContext::current()->m_defaultBrowserContext.data())
+        shutdown();
 }
 
 void BrowserContextAdapter::cancelDownload(quint32 downloadId)
diff --git a/src/core/browser_context_adapter.h b/src/core/browser_context_adapter.h
index f6ebfd51c..2e7b66a7e 100644
--- a/src/core/browser_context_adapter.h
+++ b/src/core/browser_context_adapter.h
@@ -73,6 +73,8 @@ public:
     static QSharedPointer<BrowserContextAdapter> defaultContext();
     static QObject* globalQObjectRoot();
 
+    void shutdown();
+
     VisitedLinksManagerQt *visitedLinksManager();
     DownloadManagerDelegateQt *downloadManagerDelegate();
 
diff --git a/src/core/web_engine_context.cpp b/src/core/web_engine_context.cpp
index 900b82eb9..6a8c8ae73 100644
--- a/src/core/web_engine_context.cpp
+++ b/src/core/web_engine_context.cpp
@@ -117,7 +117,11 @@ void destroyContext()
     // Before destroying MessageLoop via destroying BrowserMainRunner destructor
     // WebEngineContext's pointer is used.
     sContext->destroy();
-    sContext = 0;
+#if !defined(NDEBUG)
+    if (!sContext->HasOneRef())
+        qWarning("WebEngineContext leaked on exit, likely due to leaked WebEngine View or Page");
+#endif
+    sContext = nullptr;
     s_destroyed = true;
 }
 
@@ -193,19 +197,26 @@ void WebEngineContext::destroy()
 {
     if (m_devtoolsServer)
         m_devtoolsServer->stop();
-    delete m_globalQObject;
-    m_globalQObject = 0;
     base::MessagePump::Delegate *delegate = m_runLoop->loop_;
     // Flush the UI message loop before quitting.
     while (delegate->DoWork()) { }
+
+    if (m_defaultBrowserContext)
+        m_defaultBrowserContext->shutdown();
+    // Delete the global object and thus custom profiles
+    delete m_globalQObject;
+    m_globalQObject = nullptr;
+    // Handle any events posted by browser-context shutdown.
+    while (delegate->DoWork()) { }
+
     GLContextHelper::destroy();
-    m_devtoolsServer.reset(0);
+    m_devtoolsServer.reset();
     m_runLoop->AfterRun();
 
     // Force to destroy RenderProcessHostImpl by destroying BrowserMainRunner.
     // RenderProcessHostImpl should be destroyed before WebEngineContext since
     // default BrowserContext might be used by the RenderprocessHostImpl's destructor.
-    m_browserRunner.reset(0);
+    m_browserRunner.reset();
 
     // Drop the false reference.
     sContext->Release();
diff --git a/src/core/web_engine_context.h b/src/core/web_engine_context.h
index 92c45e260..1b4be48b1 100644
--- a/src/core/web_engine_context.h
+++ b/src/core/web_engine_context.h
@@ -80,7 +80,7 @@ class WebEngineContext : public base::RefCounted<WebEngineContext> {
 public:
     static scoped_refptr<WebEngineContext> current();
 
-    QSharedPointer<QtWebEngineCore::BrowserContextAdapter> defaultBrowserContext();
+    QSharedPointer<BrowserContextAdapter> defaultBrowserContext();
     QObject *globalQObject();
 #if BUILDFLAG(ENABLE_BASIC_PRINTING)
     printing::PrintJobManager* getPrintJobManager();
@@ -90,6 +90,7 @@ public:
 
 private:
     friend class base::RefCounted<WebEngineContext>;
+    friend class BrowserContextAdapter;
     WebEngineContext();
     ~WebEngineContext();
 
@@ -98,7 +99,7 @@ private:
     std::unique_ptr<content::ContentMainRunner> m_contentRunner;
     std::unique_ptr<content::BrowserMainRunner> m_browserRunner;
     QObject* m_globalQObject;
-    QSharedPointer<QtWebEngineCore::BrowserContextAdapter> m_defaultBrowserContext;
+    QSharedPointer<BrowserContextAdapter> m_defaultBrowserContext;
     std::unique_ptr<DevToolsServerQt> m_devtoolsServer;
 #if BUILDFLAG(ENABLE_BASIC_PRINTING)
     std::unique_ptr<printing::PrintJobManager> m_printJobManager;
diff --git a/src/webenginewidgets/api/qwebengineprofile.cpp b/src/webenginewidgets/api/qwebengineprofile.cpp
index f120ae51e..e1e72aa45 100644
--- a/src/webenginewidgets/api/qwebengineprofile.cpp
+++ b/src/webenginewidgets/api/qwebengineprofile.cpp
@@ -155,11 +155,18 @@ QWebEngineBrowserContext::QWebEngineBrowserContext(QSharedPointer<QtWebEngineCor
 
 QWebEngineBrowserContext::~QWebEngineBrowserContext()
 {
+    if (m_profile)
+        shutdown();
+}
+
+void QWebEngineBrowserContext::shutdown()
+{
     Q_ASSERT(m_profile);
     // In the case the user sets this profile as the parent of the interceptor
     // it can be deleted before the browser-context still referencing it is.
     browserContextRef->setRequestInterceptor(nullptr);
     browserContextRef->removeClient(m_profile);
+    m_profile = 0;
 }
 
 QWebEngineProfilePrivate::QWebEngineProfilePrivate(QSharedPointer<QtWebEngineCore::BrowserContextAdapter> browserContext)
@@ -181,6 +188,8 @@ QWebEngineProfilePrivate::~QWebEngineProfilePrivate()
     }
 
     m_ongoingDownloads.clear();
+    if (m_browserContext)
+        m_browserContext->shutdown();
 }
 
 QSharedPointer<QtWebEngineCore::BrowserContextAdapter> QWebEngineProfilePrivate::browserContext() const
diff --git a/src/webenginewidgets/api/qwebengineprofile_p.h b/src/webenginewidgets/api/qwebengineprofile_p.h
index 067a5bd7e..61ad4f81c 100644
--- a/src/webenginewidgets/api/qwebengineprofile_p.h
+++ b/src/webenginewidgets/api/qwebengineprofile_p.h
@@ -76,6 +76,8 @@ public:
     QWebEngineBrowserContext(QSharedPointer<QtWebEngineCore::BrowserContextAdapter> browserContext, QWebEngineProfilePrivate *profile);
     ~QWebEngineBrowserContext();
 
+    void shutdown();
+
     QSharedPointer<QtWebEngineCore::BrowserContextAdapter> browserContextRef;
 
 private: