Avoid dangling pointers in QQuickTextEdit::resourceRequestFinished()

If we encounter an error while trying to download a network resource
for a document (for example an inline image URL has an invalid protocol,
or the web server returns 404), we call QTextDocument::resource() one
more time, in case a placeholder image is to be returned. That will
call back to QQuickTextEdit::loadResource(), which will delete the
QQuickPixmap "job". Break out of the loop, don't keep iterating on
the same pixmapsInProgress list that we're deleting it from.
Likewise, break out if resourceRequestFinished() deletes the job itself.
So now we expect resourceRequestFinished() to run its loop more often
rather than continuing the first run until pixmapsInProgress is empty;
but as before, don't invalidate() until all resource-loading jobs are
done (pixmapsInProgress is empty).

Also, as a hypothetical improvement to thread safety, erase() from
the QList before deleting; and don't dereference the iterator again
when we've already done that once at the top.

Amends 7fb39a7accba014063e32ac41a58b77905bbd95b

turtle.svg for the autotest is from
https://openclipart.org/detail/235021/silhouette-turtle

Fixes: QTBUG-122108
Pick-to: 6.7
Change-Id: Icfcaba7b42c68b572efda15b1ddc791010701bfa
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>

1 files changed, 23 insertions, 19 deletions

diff --git a/src/quick/items/qquicktextedit.cpp b/src/quick/items/qquicktextedit.cpp
index 352c325108..938f75e963 100644
--- a/src/quick/items/qquicktextedit.cpp
+++ b/src/quick/items/qquicktextedit.cpp
@@ -2191,21 +2191,26 @@ QVariant QQuickTextEdit::loadResource(int type, const QUrl &source)
         // let QTextDocument::loadResource() handle local file loading
         return {};
     }
+
     // see if we already started a load job
-    for (auto it = d->pixmapsInProgress.cbegin(); it != d->pixmapsInProgress.cend(); ++it) {
-        const auto *job = *it;
-        if (job->url() == url) {
-            if (job->isError()) {
-                qmlWarning(this) << job->error();
-                delete *it;
-                it = d->pixmapsInProgress.erase(it);
-                return QImage();
-            }
+    auto existingJobIter = std::find_if(
+            d->pixmapsInProgress.cbegin(), d->pixmapsInProgress.cend(),
+            [&url](const auto *job) { return job->url() == url; } );
+    if (existingJobIter != d->pixmapsInProgress.cend()) {
+        const QQuickPixmap *job = *existingJobIter;
+        if (job->isError()) {
+            qmlWarning(this) << job->error();
+            d->pixmapsInProgress.erase(existingJobIter);
+            delete job;
+            return QImage();
+        } else {
             qCDebug(lcTextEdit) << "already downloading" << url;
             // existing job: return a null variant if it's not done yet
             return job->isReady() ? job->image() : QVariant();
         }
     }
+
+    // not found: start a new load job
     qCDebug(lcTextEdit) << "loading" << source << "resolved" << url
                         << "type" << static_cast<QTextDocument::ResourceType>(type);
     QQmlContext *context = qmlContext(this);
@@ -2224,27 +2229,26 @@ QVariant QQuickTextEdit::loadResource(int type, const QUrl &source)
 void QQuickTextEdit::resourceRequestFinished()
 {
     Q_D(QQuickTextEdit);
-    bool allDone = true;
-    for (auto it = d->pixmapsInProgress.cbegin(); it != d->pixmapsInProgress.cend();) {
+    for (auto it = d->pixmapsInProgress.cbegin(); it != d->pixmapsInProgress.cend(); ++it) {
         auto *job = *it;
         if (job->isError()) {
             // get QTextDocument::loadResource() to call QQuickTextEdit::loadResource() again, to return the placeholder
-            qCDebug(lcTextEdit) << "failed to load" << job->url();
+            qCDebug(lcTextEdit) << "failed to load (error)" << job->url();
             d->document->resource(QTextDocument::ImageResource, job->url());
+            // that will call QQuickTextEdit::loadResource() which will delete the job;
+            // so leave it in pixmapsInProgress for now, and stop this loop
+            break;
         } else if (job->isReady()) {
             // get QTextDocument::loadResource() to call QQuickTextEdit::loadResource() again, and cache the result
             auto res = d->document->resource(QTextDocument::ImageResource, job->url());
             // If QTextDocument::resource() returned a valid variant, it's been cached too. Either way, the job is done.
             qCDebug(lcTextEdit) << (res.isValid() ? "done downloading" : "failed to load") << job->url() << job->rect();
-            delete *it;
-            it = d->pixmapsInProgress.erase(it);
-        } else {
-            allDone = false;
-            ++it;
+            d->pixmapsInProgress.erase(it);
+            delete job;
+            break;
         }
     }
-    if (allDone) {
-        Q_ASSERT(d->pixmapsInProgress.isEmpty());
+    if (d->pixmapsInProgress.isEmpty()) {
         invalidate();
         updateSize();
         q_invalidate();