From a910706986aa8e83e070d23525c02649d80c05c3 Mon Sep 17 00:00:00 2001
From: Christian Mollekopf <chrigi_1@fastmail.fm>
Date: Tue, 31 Jan 2017 19:01:10 +0100
Subject: Don't call into the model after the model has been destroyed.

---
 common/modelresult.cpp  | 49 +++++++++++++++++++++++++++-------------
 common/modelresult.h    |  1 +
 common/resultprovider.h | 60 +++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 85 insertions(+), 25 deletions(-)

diff --git a/common/modelresult.cpp b/common/modelresult.cpp
index 695072c..0825518 100644
--- a/common/modelresult.cpp
+++ b/common/modelresult.cpp
@@ -21,6 +21,7 @@
 
 #include <QDebug>
 #include <QThread>
+#include <QPointer>
 
 #include "log.h"
 
@@ -31,18 +32,26 @@ static uint qHash(const Sink::ApplicationDomain::ApplicationDomainType &type)
     return qHash(type.resourceInstanceIdentifier() + type.identifier());
 }
 
+static qint64 getIdentifier(const QModelIndex &idx)
+{
+    if (!idx.isValid()) {
+        return 0;
+    }
+    return idx.internalId();
+}
+
 template <class T, class Ptr>
 ModelResult<T, Ptr>::ModelResult(const Sink::Query &query, const QList<QByteArray> &propertyColumns, const Sink::Log::Context &ctx)
     : QAbstractItemModel(), mLogCtx(ctx.subContext("modelresult")), mPropertyColumns(propertyColumns), mQuery(query)
 {
 }
 
-static qint64 getIdentifier(const QModelIndex &idx)
+template <class T, class Ptr>
+ModelResult<T, Ptr>::~ModelResult()
 {
-    if (!idx.isValid()) {
-        return 0;
+    if (mEmitter) {
+        mEmitter->waitForMethodExecutionEnd();
     }
-    return idx.internalId();
 }
 
 template <class T, class Ptr>
@@ -259,33 +268,41 @@ void ModelResult<T, Ptr>::setEmitter(const typename Sink::ResultEmitter<Ptr>::Pt
 {
     setFetcher([this](const Ptr &parent) { mEmitter->fetch(parent); });
 
-    emitter->onAdded([this](const Ptr &value) {
+    QPointer<QObject> guard(this);
+    emitter->onAdded([this, guard](const Ptr &value) {
         SinkTraceCtx(mLogCtx) << "Received addition: " << value->identifier();
-        threadBoundary.callInMainThread([this, value]() {
+        Q_ASSERT(guard);
+        threadBoundary.callInMainThread([this, value, guard]() {
+            Q_ASSERT(guard);
             add(value);
         });
     });
-    emitter->onModified([this](const Ptr &value) {
+    emitter->onModified([this, guard](const Ptr &value) {
         SinkTraceCtx(mLogCtx) << "Received modification: " << value->identifier();
+        Q_ASSERT(guard);
         threadBoundary.callInMainThread([this, value]() {
             modify(value);
         });
     });
-    emitter->onRemoved([this](const Ptr &value) {
+    emitter->onRemoved([this, guard](const Ptr &value) {
         SinkTraceCtx(mLogCtx) << "Received removal: " << value->identifier();
+        Q_ASSERT(guard);
         threadBoundary.callInMainThread([this, value]() {
             remove(value);
         });
     });
-    emitter->onInitialResultSetComplete([this](const Ptr &parent, bool fetchedAll) {
+    emitter->onInitialResultSetComplete([this, guard](const Ptr &parent, bool fetchedAll) {
         SinkTraceCtx(mLogCtx) << "Initial result set complete. Fetched all: " << fetchedAll;
-        const qint64 parentId = parent ? qHash(*parent) : 0;
-        const auto parentIndex = createIndexFromId(parentId);
-        mEntityChildrenFetchComplete.insert(parentId);
-        if (fetchedAll) {
-            mEntityAllChildrenFetched.insert(parentId);
-        }
-        emit dataChanged(parentIndex, parentIndex, QVector<int>() << ChildrenFetchedRole);
+        Q_ASSERT(guard);
+        threadBoundary.callInMainThread([=]() {
+            const qint64 parentId = parent ? qHash(*parent) : 0;
+            const auto parentIndex = createIndexFromId(parentId);
+            mEntityChildrenFetchComplete.insert(parentId);
+            if (fetchedAll) {
+                mEntityAllChildrenFetched.insert(parentId);
+            }
+            emit dataChanged(parentIndex, parentIndex, QVector<int>() << ChildrenFetchedRole);
+        });
     });
     mEmitter = emitter;
 }
diff --git a/common/modelresult.h b/common/modelresult.h
index daa48bd..f30a8e1 100644
--- a/common/modelresult.h
+++ b/common/modelresult.h
@@ -42,6 +42,7 @@ public:
     };
 
     ModelResult(const Sink::Query &query, const QList<QByteArray> &propertyColumns, const Sink::Log::Context &);
+    ~ModelResult();
 
     void setEmitter(const typename Sink::ResultEmitter<Ptr>::Ptr &);
 
diff --git a/common/resultprovider.h b/common/resultprovider.h
index cda4dac..86138db 100644
--- a/common/resultprovider.h
+++ b/common/resultprovider.h
@@ -22,6 +22,8 @@
 
 #include <functional>
 #include <memory>
+#include <QMutexLocker>
+#include <QPointer>
 
 namespace Sink {
 
@@ -82,21 +84,21 @@ public:
     void add(const T &value)
     {
         if (auto strongRef = mResultEmitter.toStrongRef()) {
-            strongRef->addHandler(value);
+            strongRef->add(value);
         }
     }
 
     void modify(const T &value)
     {
         if (auto strongRef = mResultEmitter.toStrongRef()) {
-            strongRef->modifyHandler(value);
+            strongRef->modify(value);
         }
     }
 
     void remove(const T &value)
     {
         if (auto strongRef = mResultEmitter.toStrongRef()) {
-            strongRef->removeHandler(value);
+            strongRef->remove(value);
         }
     }
 
@@ -194,6 +196,15 @@ public:
 
     virtual ~ResultEmitter()
     {
+        //Try locking in case we're in the middle of an execution in another thread
+        QMutexLocker locker{&mMutex};
+    }
+
+    virtual void waitForMethodExecutionEnd()
+    {
+        //If we're in the middle of a method execution, this will block until the method is done.
+        QMutexLocker locker{&mMutex};
+        mDone = true;
     }
 
     void onAdded(const std::function<void(const DomainType &)> &handler)
@@ -226,38 +237,55 @@ public:
         clearHandler = handler;
     }
 
+    bool guardOk()
+    {
+        return !mDone;
+    }
+
     void add(const DomainType &value)
     {
-        addHandler(value);
+        QMutexLocker locker{&mMutex};
+        if (guardOk()) {
+            addHandler(value);
+        }
     }
 
     void modify(const DomainType &value)
     {
-        modifyHandler(value);
+        QMutexLocker locker{&mMutex};
+        if (guardOk()) {
+            modifyHandler(value);
+        }
     }
 
     void remove(const DomainType &value)
     {
-        removeHandler(value);
+        QMutexLocker locker{&mMutex};
+        if (guardOk()) {
+            removeHandler(value);
+        }
     }
 
     void initialResultSetComplete(const DomainType &parent, bool replayedAll)
     {
-        if (initialResultSetCompleteHandler) {
+        QMutexLocker locker{&mMutex};
+        if (initialResultSetCompleteHandler && guardOk()) {
             initialResultSetCompleteHandler(parent, replayedAll);
         }
     }
 
     void complete()
     {
-        if (completeHandler) {
+        QMutexLocker locker{&mMutex};
+        if (completeHandler && guardOk()) {
             completeHandler();
         }
     }
 
     void clear()
     {
-        if (clearHandler) {
+        QMutexLocker locker{&mMutex};
+        if (clearHandler && guardOk()) {
             clearHandler();
         }
     }
@@ -285,6 +313,8 @@ private:
     std::function<void(void)> clearHandler;
 
     std::function<void(const DomainType &parent)> mFetcher;
+    QMutex mMutex;
+    bool mDone = false;
 };
 
 template <class DomainType>
@@ -293,6 +323,18 @@ class AggregatingResultEmitter : public ResultEmitter<DomainType>
 public:
     typedef QSharedPointer<AggregatingResultEmitter<DomainType>> Ptr;
 
+    ~AggregatingResultEmitter()
+    {
+    }
+
+    virtual void waitForMethodExecutionEnd() Q_DECL_OVERRIDE
+    {
+        for (const auto &emitter : mEmitter) {
+            emitter->waitForMethodExecutionEnd();
+        }
+        ResultEmitter<DomainType>::waitForMethodExecutionEnd();
+    }
+
     void addEmitter(const typename ResultEmitter<DomainType>::Ptr &emitter)
     {
         Q_ASSERT(emitter);
-- 
cgit v1.2.3