From 8ec38f87990cd5355a4cf0cf0533a6459fa2f6e3 Mon Sep 17 00:00:00 2001
From: Kitsune Ral <Kitsune-Ral@users.sf.net>
Date: Tue, 21 Nov 2017 17:12:20 +0900
Subject: Eliminate race condition in Avatar::get()

The race occurred because _ongoingRequest wasn't properly reinitialized if another request on a bigger size arrives while a request for a smaller size is in the air. The old request is now abandoned and continuations are collected inside the Avatar object rather than in the lambda connected to the job. Closes #124.
Along the way, _scaledPixmaps is now std::vector instead of QHash since usually it only contains very few (1 or 2) entries and QHash is a waste of memory for that.
---
 avatar.cpp | 24 ++++++++++++++----------
 avatar.h   |  9 +++++----
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/avatar.cpp b/avatar.cpp
index 0ce9210b..f5101ddb 100644
--- a/avatar.cpp
+++ b/avatar.cpp
@@ -24,7 +24,7 @@
 
 using namespace QMatrixClient;
 
-QPixmap Avatar::get(int width, int height, std::function<void()> continuation)
+QPixmap Avatar::get(int width, int height, Avatar::notifier_t notifier)
 {
     QSize size(width, height);
 
@@ -32,12 +32,15 @@ QPixmap Avatar::get(int width, int height, std::function<void()> continuation)
     // is a sure way to trick the below code into constantly getting another
     // image from the server because the existing one is alleged unsatisfactory.
     // This is plain abuse by the client, though; so not critical for now.
-    if( (!_valid && _url.isValid() && !_ongoingRequest)
-        || width > _requestedSize.width()
-        || height > _requestedSize.height() )
+    if( ( !(_valid || _ongoingRequest)
+          || width > _requestedSize.width()
+          || height > _requestedSize.height() ) && _url.isValid() )
     {
         qCDebug(MAIN) << "Getting avatar from" << _url.toString();
         _requestedSize = size;
+        if (_ongoingRequest)
+            _ongoingRequest->abandon();
+        notifiers.emplace_back(std::move(notifier));
         _ongoingRequest = _connection->callApi<MediaThumbnailJob>(_url, size);
         _ongoingRequest->connect( _ongoingRequest, &MediaThumbnailJob::finished,
                                  _connection, [=]() {
@@ -46,7 +49,8 @@ QPixmap Avatar::get(int width, int height, std::function<void()> continuation)
                 _valid = true;
                 _originalPixmap = _ongoingRequest->scaledThumbnail(_requestedSize);
                 _scaledPixmaps.clear();
-                continuation();
+                for (auto n: notifiers)
+                    n();
             }
             _ongoingRequest = nullptr;
         });
@@ -60,12 +64,12 @@ QPixmap Avatar::get(int width, int height, std::function<void()> continuation)
         _originalPixmap = _defaultIcon.pixmap(size);
     }
 
-    auto& pixmap = _scaledPixmaps[{width, height}]; // Create if needed
-    if (pixmap.isNull())
-    {
-        pixmap = _originalPixmap.scaled(size,
+    for (auto p: _scaledPixmaps)
+        if (p.first == size)
+            return p.second;
+    auto pixmap = _originalPixmap.scaled(size,
                     Qt::KeepAspectRatio, Qt::SmoothTransformation);
-    }
+    _scaledPixmaps.emplace_back(size, pixmap);
     return pixmap;
 }
 
diff --git a/avatar.h b/avatar.h
index 6889e839..60cf3779 100644
--- a/avatar.h
+++ b/avatar.h
@@ -35,7 +35,9 @@ namespace QMatrixClient
                 : _defaultIcon(std::move(defaultIcon)), _connection(connection)
             { }
 
-            QPixmap get(int w, int h, std::function<void()> continuation);
+            using notifier_t = std::function<void()>;
+
+            QPixmap get(int w, int h, notifier_t notifier);
 
             QUrl url() const { return _url; }
             bool updateUrl(const QUrl& newUrl);
@@ -45,13 +47,12 @@ namespace QMatrixClient
             QPixmap _originalPixmap;
             QIcon _defaultIcon;
 
-            /// Map of requested size to the actual pixmap used for it
-            /// (it's a shame that QSize has no predefined qHash()).
-            QHash<QPair<int,int>, QPixmap> _scaledPixmaps;
+            std::vector<QPair<QSize, QPixmap>> _scaledPixmaps;
 
             QSize _requestedSize;
             bool _valid = false;
             Connection* _connection;
             MediaThumbnailJob* _ongoingRequest = nullptr;
+            std::vector<notifier_t> notifiers;
     };
 }  // namespace QMatrixClient
-- 
cgit v1.2.3