Merge pull request #32 from Fxrh/kitsune-memory-care

Event objects leaks plugged

5 files changed, 55 insertions, 16 deletions

diff --git a/connection.cpp b/connection.cpp
index 9f56ec96..bc6d1f26 100644
--- a/connection.cpp
+++ b/connection.cpp
@@ -18,7 +18,6 @@
 
 #include "connection.h"
 #include "connectiondata.h"
-//#include "connectionprivate.h"
 #include "user.h"
 #include "events/event.h"
 #include "room.h"
@@ -172,7 +171,7 @@ void Connection::sync(int timeout)
     auto job = d->startSyncJob(filter, timeout);
     connect( job, &SyncJob::success, [=] () {
         d->data->setLastEvent(job->nextBatch());
-        for( const auto& roomData: job->roomData() )
+        for( auto& roomData: job->roomData() )
         {
             if ( Room* r = provideRoom(roomData.roomId) )
                 r->updateData(roomData);
diff --git a/jobs/syncjob.cpp b/jobs/syncjob.cpp
index 2b2705b1..59c40785 100644
--- a/jobs/syncjob.cpp
+++ b/jobs/syncjob.cpp
@@ -83,7 +83,7 @@ QString SyncJob::nextBatch() const
     return d->nextBatch;
 }
 
-const SyncData& SyncJob::roomData() const
+SyncData& SyncJob::roomData()
 {
     return d->roomData;
 }
diff --git a/jobs/syncjob.h b/jobs/syncjob.h
index ed99b38b..a9138a65 100644
--- a/jobs/syncjob.h
+++ b/jobs/syncjob.h
@@ -26,10 +26,42 @@
 
 namespace QMatrixClient
 {
+    /**
+     * @brief A crude wrapper around a container of pointers that owns pointers
+     * to contained objects
+     *
+     * Similar to vector<unique_ptr<>>, upon deletion, EventsHolder
+     * will delete all events contained in it.
+     */
+    template <typename ContainerT>
+    class Owning : public ContainerT
+    {
+        public:
+            Owning() = default;
+#if defined(_MSC_VER) && _MSC_VER < 1900
+            // Workaround: Dangerous (auto_ptr style) copy constructor because
+            // VS2013 (unnecessarily) instantiates EventList::QVector<>::toList()
+            // which instantiates QList< Owning<> > which needs the contained
+            // object to have a non-deleted copy constructor.
+            Owning(Owning& other) : ContainerT(std::move(other)) { }
+#else
+            Owning(Owning&) = delete;
+#endif
+            Owning(Owning&& other) : ContainerT(std::move(other)) { }
+            ~Owning() { for (auto e: *this) delete e; }
+
+            /**
+             * @brief returns the underlying events and releases the ownership
+             *
+             * Acts similar to unique_ptr::release.
+             */
+            ContainerT release() { return std::move(*this); }
+    };
+
     class SyncRoomData
     {
     public:
-        class EventList : public Events
+        class EventList : public Owning<Events>
         {
             private:
                 QString jsonKey;
@@ -55,7 +87,13 @@ namespace QMatrixClient
                      JoinState joinState_ = JoinState::Join,
                      const QJsonObject& room_ = QJsonObject());
     };
-    using SyncData = QVector<SyncRoomData>;
+}
+Q_DECLARE_TYPEINFO(QMatrixClient::SyncRoomData, Q_MOVABLE_TYPE);
+
+namespace QMatrixClient
+{
+    // QVector cannot work with non-copiable objects, std::vector can.
+    using SyncData = std::vector<SyncRoomData>;
 
     class ConnectionData;
     class SyncJob: public BaseJob
@@ -69,7 +107,7 @@ namespace QMatrixClient
             void setPresence(QString presence);
             void setTimeout(int timeout);
 
-            const SyncData& roomData() const;
+            SyncData& roomData();
             QString nextBatch() const;
 
         protected:
@@ -82,6 +120,5 @@ namespace QMatrixClient
             Private* d;
     };
 }
-Q_DECLARE_TYPEINFO(QMatrixClient::SyncRoomData, Q_MOVABLE_TYPE);
 
 #endif // QMATRIXCLIENT_SYNCJOB_H
diff --git a/room.cpp b/room.cpp
index e559bc37..e07426a7 100644
--- a/room.cpp
+++ b/room.cpp
@@ -119,7 +119,7 @@ QString Room::id() const
     return d->id;
 }
 
-Room::Timeline Room::messageEvents() const
+const Room::Timeline& Room::messageEvents() const
 {
     return d->messageEvents;
 }
@@ -329,7 +329,7 @@ QString Room::roomMembername(QString userId) const
     return roomMembername(connection()->user(userId));
 }
 
-void Room::updateData(const SyncRoomData& data)
+void Room::updateData(SyncRoomData& data)
 {
     if( d->prevBatch.isEmpty() )
         d->prevBatch = data.timelinePrevBatch;
@@ -339,7 +339,7 @@ void Room::updateData(const SyncRoomData& data)
 
     // State changes can arrive in a timeline event; so check those.
     processStateEvents(data.timeline);
-    addNewMessageEvents(data.timeline);
+    addNewMessageEvents(data.timeline.release());
 
     for( Event* ephemeralEvent: data.ephemeral )
     {
@@ -452,6 +452,7 @@ void Room::processStateEvents(const Events& events)
         if( event->type() == EventType::RoomMember )
         {
             RoomMemberEvent* memberEvent = static_cast<RoomMemberEvent*>(event);
+            // Can't use d->member() below because the user may be not a member (yet)
             User* u = d->connection->user(memberEvent->userId());
             u->processEvent(event);
             if( memberEvent->membership() == MembershipType::Join )
@@ -472,9 +473,10 @@ void Room::processEphemeralEvent(Event* event)
     {
         TypingEvent* typingEvent = static_cast<TypingEvent*>(event);
         d->usersTyping.clear();
-        for( const QString& user: typingEvent->users() )
+        for( const QString& userId: typingEvent->users() )
         {
-            d->usersTyping.append(d->connection->user(user));
+            if (auto m = d->member(userId))
+                d->usersTyping.append(m);
         }
         emit typingChanged();
     }
@@ -486,7 +488,8 @@ void Room::processEphemeralEvent(Event* event)
             const auto receipts = receiptEvent->receiptsForEvent(eventId);
             for( const Receipt& r: receipts )
             {
-                d->lastReadEvent.insert(d->connection->user(r.userId), eventId);
+                if (auto m = d->member(r.userId))
+                    d->lastReadEvent.insert(m, eventId);
             }
         }
     }
diff --git a/room.h b/room.h
index 1c29aa01..7266ae70 100644
--- a/room.h
+++ b/room.h
@@ -37,13 +37,13 @@ namespace QMatrixClient
     {
             Q_OBJECT
         public:
-            using Timeline = Events;
+            using Timeline = Owning<Events>;
 
             Room(Connection* connection, QString id);
             virtual ~Room();
 
             Q_INVOKABLE QString id() const;
-            Q_INVOKABLE Timeline messageEvents() const;
+            Q_INVOKABLE const Timeline& messageEvents() const;
             Q_INVOKABLE QString name() const;
             Q_INVOKABLE QStringList aliases() const;
             Q_INVOKABLE QString canonicalAlias() const;
@@ -66,7 +66,7 @@ namespace QMatrixClient
              */
             Q_INVOKABLE QString roomMembername(QString userId) const;
 
-            Q_INVOKABLE void updateData( const SyncRoomData& data );
+            Q_INVOKABLE void updateData(SyncRoomData& data );
             Q_INVOKABLE void setJoinState( JoinState state );
 
             Q_INVOKABLE void markMessageAsRead( Event* event );