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>
---
 src/webengine/api/qquickwebenginedownloaditem.cpp |  2 --
 src/webengine/api/qquickwebengineprofile.cpp      | 28 +++++++++++++++--------
 src/webengine/api/qquickwebengineprofile_p.h      |  4 +++-
 3 files changed, 22 insertions(+), 12 deletions(-)

(limited to 'src/webengine')

diff --git a/src/webengine/api/qquickwebenginedownloaditem.cpp b/src/webengine/api/qquickwebenginedownloaditem.cpp
index 7d1382876..981d11633 100644
--- a/src/webengine/api/qquickwebenginedownloaditem.cpp
+++ b/src/webengine/api/qquickwebenginedownloaditem.cpp
@@ -629,8 +629,6 @@ QQuickWebEngineDownloadItem::~QQuickWebEngineDownloadItem()
 {
     if (!isFinished())
         cancel();
-    if (d_ptr->profile)
-        d_ptr->profile->d_ptr->profileAdapter()->removeDownload(d_ptr->downloadId);
 }
 
 QT_END_NAMESPACE
diff --git a/src/webengine/api/qquickwebengineprofile.cpp b/src/webengine/api/qquickwebengineprofile.cpp
index ddc71602b..26fcf28f7 100644
--- a/src/webengine/api/qquickwebengineprofile.cpp
+++ b/src/webengine/api/qquickwebengineprofile.cpp
@@ -175,13 +175,6 @@ QQuickWebEngineProfilePrivate::~QQuickWebEngineProfilePrivate()
         m_profileAdapter->removeClient(this);
     }
 
-    for (QQuickWebEngineDownloadItem *download : qAsConst(m_ongoingDownloads)) {
-        if (download)
-            download->cancel();
-    }
-
-    m_ongoingDownloads.clear();
-
     if (m_profileAdapter != QtWebEngineCore::ProfileAdapter::defaultProfileAdapter())
         delete m_profileAdapter;
 }
@@ -215,6 +208,23 @@ void QQuickWebEngineProfilePrivate::cancelDownload(quint32 downloadId)
 void QQuickWebEngineProfilePrivate::downloadDestroyed(quint32 downloadId)
 {
     m_ongoingDownloads.remove(downloadId);
+    if (m_profileAdapter)
+        m_profileAdapter->removeDownload(downloadId);
+}
+
+void QQuickWebEngineProfilePrivate::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 QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
@@ -239,6 +249,7 @@ void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
     QQuickWebEngineDownloadItem *download = new QQuickWebEngineDownloadItem(itemPrivate, q);
 
     m_ongoingDownloads.insert(info.id, download);
+    QObject::connect(download, &QQuickWebEngineDownloadItem::destroyed, q, [id = info.id, this] () { downloadDestroyed(id); });
 
     QQmlEngine::setObjectOwnership(download, QQmlEngine::JavaScriptOwnership);
     Q_EMIT q->downloadRequested(download);
@@ -252,7 +263,6 @@ void QQuickWebEngineProfilePrivate::downloadRequested(DownloadItemInfo &info)
     if (state == QQuickWebEngineDownloadItem::DownloadRequested) {
         // Delete unaccepted downloads.
         info.accepted = false;
-        m_ongoingDownloads.remove(info.id);
         delete download;
     }
 }
@@ -275,7 +285,6 @@ void QQuickWebEngineProfilePrivate::downloadUpdated(const DownloadItemInfo &info
 
     if (info.state != ProfileAdapterClient::DownloadInProgress) {
         Q_EMIT q->downloadFinished(download);
-        m_ongoingDownloads.remove(info.id);
     }
 }
 
@@ -380,6 +389,7 @@ QQuickWebEngineProfile::QQuickWebEngineProfile(QQuickWebEngineProfilePrivate *pr
 */
 QQuickWebEngineProfile::~QQuickWebEngineProfile()
 {
+    d_ptr->cleanDownloads();
 }
 
 /*!
diff --git a/src/webengine/api/qquickwebengineprofile_p.h b/src/webengine/api/qquickwebengineprofile_p.h
index d31ded0ec..2b1a5b134 100644
--- a/src/webengine/api/qquickwebengineprofile_p.h
+++ b/src/webengine/api/qquickwebengineprofile_p.h
@@ -53,7 +53,7 @@
 
 #include "profile_adapter_client.h"
 #include "profile_adapter.h"
-#include "qquickwebengineprofile_p.h"
+#include "qquickwebengineprofile.h"
 
 #include <QExplicitlySharedDataPointer>
 #include <QMap>
@@ -80,6 +80,8 @@ public:
     void cancelDownload(quint32 downloadId);
     void downloadDestroyed(quint32 downloadId);
 
+    void cleanDownloads();
+
     void downloadRequested(DownloadItemInfo &info) override;
     void downloadUpdated(const DownloadItemInfo &info) override;
 
-- 
cgit v1.2.3