From 7537526093c92e89672d1e952a9baceecaa91730 Mon Sep 17 00:00:00 2001
From: Kirill Burtsev <kirill.burtsev@qt.io>
Date: Mon, 11 Feb 2019 19:21:03 +0100
Subject: Remove download properly on profile destruction to avoid use after
 free
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

In the Widgets API, download items are children of the profile and are
destroyed when the parent profile destroys its children. The download
item's destructor can therefore not access the profile, as it would
cause a heap-use-after-free crashes. On quick side turn ongoing downloads
cleanup to match widgets one.

Fixes: QTBUG-73839
Change-Id: Iabb379e91187e3e68ebcd4693fec35883b72b1f2
Reviewed-by: Michael Brüning <michael.bruning@qt.io>
Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>
---
 .../api/qwebenginedownloaditem.cpp                 |  4 +--
 src/webenginewidgets/api/qwebengineprofile.cpp     | 30 ++++++++++++++--------
 src/webenginewidgets/api/qwebengineprofile_p.h     |  2 ++
 3 files changed, 23 insertions(+), 13 deletions(-)

(limited to 'src/webenginewidgets')

diff --git a/src/webenginewidgets/api/qwebenginedownloaditem.cpp b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
index f0f0958a5..deb92bfd3 100644
--- a/src/webenginewidgets/api/qwebenginedownloaditem.cpp
+++ b/src/webenginewidgets/api/qwebenginedownloaditem.cpp
@@ -660,10 +660,10 @@ QWebEngineDownloadItem::QWebEngineDownloadItem(QWebEngineDownloadItemPrivate *p,
 */
 QWebEngineDownloadItem::~QWebEngineDownloadItem()
 {
+    // MEMO Items are owned by profile by default and will be destroyed on profile's destruction
+    //      It's not safe to access profile in that case, so we rely on profile to clean up items
     if (!isFinished())
         cancel();
-    if (auto profileAdapter = d_ptr->profile->profileAdapter())
-        profileAdapter->removeDownload(d_ptr->downloadId);
 }
 
 QT_END_NAMESPACE
diff --git a/src/webenginewidgets/api/qwebengineprofile.cpp b/src/webenginewidgets/api/qwebengineprofile.cpp
index 03ce5e0bc..0d12fdae1 100644
--- a/src/webenginewidgets/api/qwebengineprofile.cpp
+++ b/src/webenginewidgets/api/qwebengineprofile.cpp
@@ -175,13 +175,6 @@ QWebEngineProfilePrivate::~QWebEngineProfilePrivate()
         m_profileAdapter->removeClient(this);
     }
 
-    for (QWebEngineDownloadItem *download : qAsConst(m_ongoingDownloads)) {
-        if (download)
-            download->cancel();
-    }
-
-    m_ongoingDownloads.clear();
-
     if (m_profileAdapter != QtWebEngineCore::ProfileAdapter::defaultProfileAdapter())
         delete m_profileAdapter;
 
@@ -196,6 +189,23 @@ ProfileAdapter* QWebEngineProfilePrivate::profileAdapter() const
 void QWebEngineProfilePrivate::downloadDestroyed(quint32 downloadId)
 {
     m_ongoingDownloads.remove(downloadId);
+    if (m_profileAdapter)
+        m_profileAdapter->removeDownload(downloadId);
+}
+
+void QWebEngineProfilePrivate::cleanDownloads()
+{
+    for (auto download : m_ongoingDownloads.values()) {
+        if (!download)
+            continue;
+
+        if (!download->isFinished())
+            download->cancel();
+
+        if (m_profileAdapter)
+            m_profileAdapter->removeDownload(download->id());
+    }
+    m_ongoingDownloads.clear();
 }
 
 void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
@@ -219,6 +229,7 @@ void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
     QWebEngineDownloadItem *download = new QWebEngineDownloadItem(itemPrivate, q);
 
     m_ongoingDownloads.insert(info.id, download);
+    QObject::connect(download, &QWebEngineDownloadItem::destroyed, q, [id = info.id, this] () { downloadDestroyed(id); });
 
     Q_EMIT q->downloadRequested(download);
 
@@ -232,7 +243,6 @@ void QWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
     if (state == QWebEngineDownloadItem::DownloadRequested) {
         // Delete unaccepted downloads.
         info.accepted = false;
-        m_ongoingDownloads.remove(info.id);
         delete download;
     }
 }
@@ -250,9 +260,6 @@ void QWebEngineProfilePrivate::downloadUpdated(const DownloadItemInfo &info)
     }
 
     download->d_func()->update(info);
-
-    if (download->isFinished())
-        m_ongoingDownloads.remove(info.id);
 }
 
 /*!
@@ -301,6 +308,7 @@ QWebEngineProfile::QWebEngineProfile(QWebEngineProfilePrivate *privatePtr, QObje
 */
 QWebEngineProfile::~QWebEngineProfile()
 {
+    d_ptr->cleanDownloads();
 }
 
 /*!
diff --git a/src/webenginewidgets/api/qwebengineprofile_p.h b/src/webenginewidgets/api/qwebengineprofile_p.h
index 9ff8df849..4a76f457f 100644
--- a/src/webenginewidgets/api/qwebengineprofile_p.h
+++ b/src/webenginewidgets/api/qwebengineprofile_p.h
@@ -81,6 +81,8 @@ public:
 
     void downloadDestroyed(quint32 downloadId);
 
+    void cleanDownloads();
+
     void downloadRequested(DownloadItemInfo &info) override;
     void downloadUpdated(const DownloadItemInfo &info) override;
 
-- 
cgit v1.2.3