Fix race condition in consumeRoomData()

QCoreApplication::processEvents() is well-known to be a _wrong_ solution
to the unresponsive UI problem; despite that, connection.cpp has long
had that call to let UI update itself while processing bulky room
updates (mainly from the initial sync). This commit finally fixes this,
after an (admittedly rare) race condition has been hit, as follows:
0. Pre-requisite: quotest runs all the tests and is about to leave
   the room; there's an ongoing sync request.
1. Quotest calls /leave
2. Sync returns, with the batch of _several_ rooms (that's important)
3. The above code handles the first room in the batch
4. processEvents() is called, just in time for the /leave response.
5. The /leave response handler in quotest ends up calling
   Connection::logout() (processEvents() still hasn't returned).
6. Connection::logout() calls abandon() on the ongoing SyncJob,
   pulling the rug from under onSyncSuccess()/consumeRoomData().
7. processEvents() returns and the above code proceeds to the next
   room - only to find that the roomDataList (that is a ref to
   a structure owned by SyncJob), is now pointing to garbage.

Morals of the story:
1. processEvents() effectively makes code multi-threaded: one flow is
   suspended and another one may run _on the same data_. After the first
   flow is resumed, it cannot make any assumptions regarding which data
   the second flow touched and/or changed.
2. The library had quite a few cases of using &&-refs, avoiding even
   move operations but also leaving ownership of the data with the
   original producer (SyncJob). If the lifetime of that producer ends
   too soon, those refs become dangling.

The fix makes two important things, respectively:
2. Ownership of room data is now transfered to the processing side,
   the moment it is scheduled (see below), in the form of moving
   into a lambda capture.
1. Instead of processEvents(), processing of room data is scheduled
   via QMetaObject::invokeMethod(), uncoupling the moment when the
   data was received in SyncJob from the moment they are processed
   in Room::updateData() (and all the numerous  signal-slots it calls).

Also: Room::baseStateLoaded now causes Connection::loadedRoomState, not
the other way round - this is more natural and doesn't need Connection
to keep firstTimeRooms map around.

5 files changed, 30 insertions, 27 deletions

diff --git a/lib/connection.cpp b/lib/connection.cpp
index 1ea394a1..4418958e 100644
--- a/lib/connection.cpp
+++ b/lib/connection.cpp
@@ -96,7 +96,6 @@ public:
     /// as of the last sync
     QHash<QString, QString> roomAliasMap;
     QVector<QString> roomIdsToForget;
-    QVector<Room*> firstTimeRooms;
     QVector<QString> pendingStateRoomIds;
     QMap<QString, User*> userMap;
     DirectChatsMap directChats;
@@ -833,16 +832,14 @@ void Connection::Private::consumeRoomData(SyncDataList&& roomDataList,
         }
         if (auto* r = q->provideRoom(roomData.roomId, roomData.joinState)) {
             pendingStateRoomIds.removeOne(roomData.roomId);
-            r->updateData(std::move(roomData), fromCache);
-            if (firstTimeRooms.removeOne(r)) {
-                emit q->loadedRoomState(r);
-                if (capabilities.roomVersions)
-                    r->checkVersion();
-                // Otherwise, the version will be checked in reloadCapabilities()
-            }
+            // Update rooms one by one, giving time to update the UI.
+            QMetaObject::invokeMethod(
+                r,
+                [r, rd = std::move(roomData), fromCache] () mutable {
+                    r->updateData(std::move(rd), fromCache);
+                },
+                Qt::QueuedConnection);
         }
-        // Let UI update itself after updating each room
-        QCoreApplication::processEvents();
     }
 }
 
@@ -1707,9 +1704,14 @@ Room* Connection::provideRoom(const QString& id, Omittable<JoinState> joinState)
             return nullptr;
         }
         d->roomMap.insert(roomKey, room);
-        d->firstTimeRooms.push_back(room);
         connect(room, &Room::beforeDestruction, this,
                 &Connection::aboutToDeleteRoom);
+        connect(room, &Room::baseStateLoaded, this, [this, room] {
+            emit loadedRoomState(room);
+            if (d->capabilities.roomVersions)
+                room->checkVersion();
+            // Otherwise, the version will be checked in reloadCapabilities()
+        });
         emit newRoom(room);
     }
     if (!joinState)
diff --git a/lib/jobs/syncjob.h b/lib/jobs/syncjob.h
index 830a7c71..b7bfbbb3 100644
--- a/lib/jobs/syncjob.h
+++ b/lib/jobs/syncjob.h
@@ -15,7 +15,7 @@ public:
     explicit SyncJob(const QString& since, const Filter& filter,
                      int timeout = -1, const QString& presence = {});
 
-    SyncData&& takeData() { return std::move(d); }
+    SyncData takeData() { return std::move(d); }
 
 protected:
     Status prepareResult() override;
diff --git a/lib/room.cpp b/lib/room.cpp
index 4d9f952c..4ba699b0 100644
--- a/lib/room.cpp
+++ b/lib/room.cpp
@@ -415,11 +415,6 @@ Room::Room(Connection* connection, QString id, JoinState initialJoinState)
     // https://marcmutz.wordpress.com/translated-articles/pimp-my-pimpl-%E2%80%94-reloaded/
     d->q = this;
     d->displayname = d->calculateDisplayname(); // Set initial "Empty room" name
-    connectUntil(connection, &Connection::loadedRoomState, this, [this](Room* r) {
-        if (this == r)
-            emit baseStateLoaded();
-        return this == r; // loadedRoomState fires only once per room
-    });
 #ifdef Quotient_E2EE_ENABLED
     connectSingleShot(this, &Room::encryption, this, [this, connection](){
         connection->encryptionUpdate(this);
@@ -1820,6 +1815,9 @@ Room::Changes Room::Private::updateStatsFromSyncData(const SyncRoomData& data,
 
 void Room::updateData(SyncRoomData&& data, bool fromCache)
 {
+    qCDebug(MAIN) << "--- Updating room" << id() << "/" << objectName();
+    bool firstUpdate = d->baseState.empty();
+
     if (d->prevBatch.isEmpty())
         d->prevBatch = data.timelinePrevBatch;
     setJoinState(data.joinState);
@@ -1845,6 +1843,9 @@ void Room::updateData(SyncRoomData&& data, bool fromCache)
         emit namesChanged(this);
 
     d->postprocessChanges(roomChanges, !fromCache);
+    if (firstUpdate)
+        emit baseStateLoaded();
+    qCDebug(MAIN) << "--- Finished updating room" << id() << "/" << objectName();
 }
 
 void Room::Private::postprocessChanges(Changes changes, bool saveState)
diff --git a/lib/syncdata.cpp b/lib/syncdata.cpp
index 78957cbe..95d3c7e4 100644
--- a/lib/syncdata.cpp
+++ b/lib/syncdata.cpp
@@ -142,7 +142,7 @@ SyncData::SyncData(const QString& cacheFileName)
                         << "is required; discarding the cache";
 }
 
-SyncDataList&& SyncData::takeRoomData() { return move(roomData); }
+SyncDataList SyncData::takeRoomData() { return move(roomData); }
 
 QString SyncData::fileNameForRoom(QString roomId)
 {
@@ -150,18 +150,18 @@ QString SyncData::fileNameForRoom(QString roomId)
     return roomId + ".json";
 }
 
-Events&& SyncData::takePresenceData() { return std::move(presenceData); }
+Events SyncData::takePresenceData() { return std::move(presenceData); }
 
-Events&& SyncData::takeAccountData() { return std::move(accountData); }
+Events SyncData::takeAccountData() { return std::move(accountData); }
 
-Events&& SyncData::takeToDeviceEvents() { return std::move(toDeviceEvents); }
+Events SyncData::takeToDeviceEvents() { return std::move(toDeviceEvents); }
 
 std::pair<int, int> SyncData::cacheVersion()
 {
     return { MajorCacheVersion, 2 };
 }
 
-DevicesList&& SyncData::takeDevicesList() { return std::move(devicesList); }
+DevicesList SyncData::takeDevicesList() { return std::move(devicesList); }
 
 QJsonObject SyncData::loadJson(const QString& fileName)
 {
diff --git a/lib/syncdata.h b/lib/syncdata.h
index 6b70140d..9358ec8f 100644
--- a/lib/syncdata.h
+++ b/lib/syncdata.h
@@ -98,15 +98,15 @@ public:
      */
     void parseJson(const QJsonObject& json, const QString& baseDir = {});
 
-    Events&& takePresenceData();
-    Events&& takeAccountData();
-    Events&& takeToDeviceEvents();
+    Events takePresenceData();
+    Events takeAccountData();
+    Events takeToDeviceEvents();
     const QHash<QString, int>& deviceOneTimeKeysCount() const
     {
         return deviceOneTimeKeysCount_;
     }
-    SyncDataList&& takeRoomData();
-    DevicesList&& takeDevicesList();
+    SyncDataList takeRoomData();
+    DevicesList takeDevicesList();
 
     QString nextBatch() const { return nextBatch_; }