From ff8f1ac8caa86006a887d9104886aadd23f8750c Mon Sep 17 00:00:00 2001 From: Charles Yin Date: Fri, 20 Jan 2012 15:41:16 +1000 Subject: [PATCH] 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 Reviewed-by: Glenn Watson --- src/declarative/qml/qdeclarativelistmodel.cpp | 4 +- .../qml/qdeclarativelistmodelworkeragent.cpp | 88 ++++++++++++---------- .../qml/qdeclarativelistmodelworkeragent_p.h | 1 + src/declarative/qml/qdeclarativeworkerscript.cpp | 11 ++- .../tst_qdeclarativelistmodel.cpp | 22 ++++++ 5 files changed, 82 insertions(+), 44 deletions(-) diff --git a/src/declarative/qml/qdeclarativelistmodel.cpp b/src/declarative/qml/qdeclarativelistmodel.cpp index eff3c46..65b3de9 100644 --- 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 --- 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(e); - - const QList &changes = s->data.changes; - - bool cc = m_orig->count() != s->list->count(); - - QHash targetModelDynamicHash; - QHash 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(e); + const QList &changes = s->data.changes; + + cc = m_orig->count() != s->list->count(); + + QHash targetModelDynamicHash; + QHash 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 --- 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 --- a/src/declarative/qml/qdeclarativeworkerscript.cpp +++ b/src/declarative/qml/qdeclarativeworkerscript.cpp @@ -316,7 +316,6 @@ v8::Handle QDeclarativeWorkerScriptEnginePrivate::getWorker(WorkerSc bool QDeclarativeWorkerScriptEnginePrivate::event(QEvent *event) { - // XXX must handle remove request if (event->type() == (QEvent::Type)WorkerDataEvent::WorkerData) { WorkerDataEvent *workerEvent = static_cast(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(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 --- 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() -- 2.7.4