diff options
author | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2017-11-16 14:36:33 +0100 |
---|---|---|
committer | Allan Sandfeld Jensen <allan.jensen@qt.io> | 2018-01-05 13:59:35 +0000 |
commit | 9ef3a8263098c6a32db8b824aabf85587d1f1140 (patch) | |
tree | 9ef2a62d51287dd676ebada6d63058687144bc2c | |
parent | 196ae04aa7c9b274880409fb38a050db99197900 (diff) |
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>
-rw-r--r-- | src/core/browser_context_adapter.cpp | 14 | ||||
-rw-r--r-- | src/core/browser_context_adapter.h | 2 | ||||
-rw-r--r-- | src/core/web_engine_context.cpp | 21 | ||||
-rw-r--r-- | src/core/web_engine_context.h | 5 | ||||
-rw-r--r-- | src/webenginewidgets/api/qwebengineprofile.cpp | 9 | ||||
-rw-r--r-- | src/webenginewidgets/api/qwebengineprofile_p.h | 2 |
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: |