Fix crash bug related to QDeclarativeListModel

If QDeclarativeListModel is deleted, all references to this object in
QDeclarativeListModelWorkerAgent and WorkerScript objects should be removed
and additional checking is needed when process the pending sync() events.

Change-Id: I12b1f06699cc908e684af0886cf06d811c3fceb4
Reviewed-by: Michael Brasser <michael.brasser@nokia.com>
Reviewed-by: Glenn Watson <glenn.watson@nokia.com>

diff --git a/src/declarative/qml/qdeclarativelistmodel.cpp b/src/declarative/qml/qdeclarativelistmodel.cpp
index eff3c46..65b3de9 100644 (file)
--- a/src/declarative/qml/qdeclarativelistmodel.cpp
+++ b/src/declarative/qml/qdeclarativelistmodel.cpp
@@ -1501,8 +1501,10 @@ QDeclarativeListModel::~QDeclarativeListModel()
         m_listModel->destroy();
         delete m_listModel;
 
-        if (m_mainThread && m_agent)
+        if (m_mainThread && m_agent) {
+            m_agent->modelDestroyed();
             m_agent->release();
+        }
     }
 
     m_listModel = 0;

diff --git a/src/declarative/qml/qdeclarativelistmodelworkeragent.cpp b/src/declarative/qml/qdeclarativelistmodelworkeragent.cpp
index c7f671c..3ae1b32 100644 (file)
--- a/src/declarative/qml/qdeclarativelistmodelworkeragent.cpp
+++ b/src/declarative/qml/qdeclarativelistmodelworkeragent.cpp
@@ -117,6 +117,11 @@ void QDeclarativeListModelWorkerAgent::release()
         delete this;
 }
 
+void QDeclarativeListModelWorkerAgent::modelDestroyed()
+{
+    m_orig = 0;
+}
+
 int QDeclarativeListModelWorkerAgent::count() const
 {
     return m_copy->count();
@@ -178,49 +183,50 @@ void QDeclarativeListModelWorkerAgent::sync()
 bool QDeclarativeListModelWorkerAgent::event(QEvent *e)
 {
     if (e->type() == QEvent::User) {
-
+        bool cc = false;
         QMutexLocker locker(&mutex);
-        Sync *s = static_cast<Sync *>(e);
-
-        const QList<Change> &changes = s->data.changes;
-
-        bool cc = m_orig->count() != s->list->count();
-
-        QHash<int, QDeclarativeListModel *> targetModelDynamicHash;
-        QHash<int, ListModel *> targetModelStaticHash;
-
-        Q_ASSERT(m_orig->m_dynamicRoles == s->list->m_dynamicRoles);
-        if (m_orig->m_dynamicRoles)
-            QDeclarativeListModel::sync(s->list, m_orig, &targetModelDynamicHash);
-        else
-            ListModel::sync(s->list->m_listModel, m_orig->m_listModel, &targetModelStaticHash);
-
-        for (int ii = 0; ii < changes.count(); ++ii) {
-            const Change &change = changes.at(ii);
-
-            QDeclarativeListModel *model = 0;
-            if (m_orig->m_dynamicRoles) {
-                model = targetModelDynamicHash.value(change.modelUid);
-            } else {
-                ListModel *lm = targetModelStaticHash.value(change.modelUid);
-                if (lm)
-                    model = lm->m_modelCache;
-            }
+        if (m_orig) {
+            Sync *s = static_cast<Sync *>(e);
+            const QList<Change> &changes = s->data.changes;
+
+            cc = m_orig->count() != s->list->count();
+
+            QHash<int, QDeclarativeListModel *> targetModelDynamicHash;
+            QHash<int, ListModel *> targetModelStaticHash;
+
+            Q_ASSERT(m_orig->m_dynamicRoles == s->list->m_dynamicRoles);
+            if (m_orig->m_dynamicRoles)
+                QDeclarativeListModel::sync(s->list, m_orig, &targetModelDynamicHash);
+            else
+                ListModel::sync(s->list->m_listModel, m_orig->m_listModel, &targetModelStaticHash);
+
+            for (int ii = 0; ii < changes.count(); ++ii) {
+                const Change &change = changes.at(ii);
+
+                QDeclarativeListModel *model = 0;
+                if (m_orig->m_dynamicRoles) {
+                    model = targetModelDynamicHash.value(change.modelUid);
+                } else {
+                    ListModel *lm = targetModelStaticHash.value(change.modelUid);
+                    if (lm)
+                        model = lm->m_modelCache;
+                }
 
-            if (model) {
-                switch (change.type) {
-                case Change::Inserted:
-                    emit model->itemsInserted(change.index, change.count);
-                    break;
-                case Change::Removed:
-                    emit model->itemsRemoved(change.index, change.count);
-                    break;
-                case Change::Moved:
-                    emit model->itemsMoved(change.index, change.to, change.count);
-                    break;
-                case Change::Changed:
-                    emit model->itemsChanged(change.index, change.count, change.roles);
-                    break;
+                if (model) {
+                    switch (change.type) {
+                    case Change::Inserted:
+                        emit model->itemsInserted(change.index, change.count);
+                        break;
+                    case Change::Removed:
+                        emit model->itemsRemoved(change.index, change.count);
+                        break;
+                    case Change::Moved:
+                        emit model->itemsMoved(change.index, change.to, change.count);
+                        break;
+                    case Change::Changed:
+                        emit model->itemsChanged(change.index, change.count, change.roles);
+                        break;
+                    }
                 }
             }
         }

diff --git a/src/declarative/qml/qdeclarativelistmodelworkeragent_p.h b/src/declarative/qml/qdeclarativelistmodelworkeragent_p.h
index f84d598..f2c971c 100644 (file)
--- a/src/declarative/qml/qdeclarativelistmodelworkeragent_p.h
+++ b/src/declarative/qml/qdeclarativelistmodelworkeragent_p.h
@@ -108,6 +108,7 @@ public:
 
         QDeclarativeListModelWorkerAgent *a;
     };
+    void modelDestroyed();
 protected:
     virtual bool event(QEvent *);
 

diff --git a/src/declarative/qml/qdeclarativeworkerscript.cpp b/src/declarative/qml/qdeclarativeworkerscript.cpp
index b17c026..5328311 100644 (file)
--- a/src/declarative/qml/qdeclarativeworkerscript.cpp
+++ b/src/declarative/qml/qdeclarativeworkerscript.cpp
@@ -316,7 +316,6 @@ v8::Handle<v8::Object> QDeclarativeWorkerScriptEnginePrivate::getWorker(WorkerSc
 
 bool QDeclarativeWorkerScriptEnginePrivate::event(QEvent *event)
 {
-    // XXX must handle remove request
     if (event->type() == (QEvent::Type)WorkerDataEvent::WorkerData) {
         WorkerDataEvent *workerEvent = static_cast<WorkerDataEvent *>(event);
         processMessage(workerEvent->workerId(), workerEvent->data());
@@ -328,6 +327,10 @@ bool QDeclarativeWorkerScriptEnginePrivate::event(QEvent *event)
     } else if (event->type() == (QEvent::Type)WorkerDestroyEvent) {
         emit stopThread();
         return true;
+    } else if (event->type() == (QEvent::Type)WorkerRemoveEvent::WorkerRemove) {
+        WorkerRemoveEvent *workerEvent = static_cast<WorkerRemoveEvent *>(event);
+        workers.remove(workerEvent->workerId());
+        return true;
     } else {
         return QObject::event(event);
     }
@@ -513,7 +516,11 @@ int QDeclarativeWorkerScriptEngine::registerWorkerScript(QDeclarativeWorkerScrip
 
 void QDeclarativeWorkerScriptEngine::removeWorkerScript(int id)
 {
-    QCoreApplication::postEvent(d, new WorkerRemoveEvent(id));
+    QDeclarativeWorkerScriptEnginePrivate::WorkerScript* script = d->workers.value(id);
+    if (script) {
+        script->owner = 0;
+        QCoreApplication::postEvent(d, new WorkerRemoveEvent(id));
+    }
 }
 
 void QDeclarativeWorkerScriptEngine::executeUrl(int id, const QUrl &url)

diff --git a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
index ff0d3fa..4eb723b 100644 (file)
--- a/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
+++ b/tests/auto/declarative/qdeclarativelistmodel/tst_qdeclarativelistmodel.cpp
@@ -1481,6 +1481,28 @@ void tst_qdeclarativelistmodel::worker_remove_element()
 
     delete item;
     qApp->processEvents();
+
+    {
+        //don't crash if model was deleted earlier
+        QDeclarativeListModel* model = new QDeclarativeListModel;
+        model->setDynamicRoles(dynamicRoles);
+        QDeclarativeEngine eng;
+        QDeclarativeComponent component(&eng, testFileUrl("workerremoveelement.qml"));
+        QQuickItem *item = createWorkerTest(&eng, &component, model);
+        QVERIFY(item != 0);
+
+        QVERIFY(QMetaObject::invokeMethod(item, "addItem"));
+
+        QVERIFY(model->count() == 1);
+
+        QVERIFY(QMetaObject::invokeMethod(item, "removeItemViaWorker"));
+        QVERIFY(QMetaObject::invokeMethod(item, "doSync"));
+        delete model;
+        qApp->processEvents(); //must not crash here
+        waitForWorker(item);
+
+        delete item;
+    }
 }
 
 void tst_qdeclarativelistmodel::worker_remove_list_data()