Merge lp:~renatofilho/qtorganizer5-eds/fix-1423185 into lp:qtorganizer5-eds

[Renato Araujo Oliveira Filho]

Fixed engine crash while destroying and requests still running.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:74
http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-ci/196/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-amd64-ci/3/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-armhf-ci/3/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtorganizer5-eds-vivid-i386-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtorganizer5-eds-ci/196/rebuild

review: Needs Fixing (continuous-integration)

[Alan Pope 🍺🐧🐱 🦄 (popey)]

I've been unable to crash the app with this patch on my Nexus 7. Thanks Renato!

review: Approve

=== modified file 'organizer/qorganizer-eds-engine.cpp'
--- organizer/qorganizer-eds-engine.cpp	2015-02-26 17:30:35 +0000
+++ organizer/qorganizer-eds-engine.cpp	2015-03-06 22:48:43 +0000
@@ -98,6 +98,7 @@
 {
     while(m_runningRequests.count()) {
         QOrganizerAbstractRequest *req = m_runningRequests.keys().first();
+        req->cancel();
         QOrganizerEDSEngine::requestDestroyed(req);
     }
 
@@ -624,7 +625,7 @@
                                        &uids,
                                        &gError);
     if (gError) {
-        qWarning() << "Fail to create items:" << gError->message;
+        qWarning() << "Fail to create items:" << (void*) data << gError->message;
         g_error_free(gError);
         gError = 0;
 
@@ -1073,6 +1074,7 @@
         data->cancel();
         return true;
     }
+    qWarning() << "Request is not running" << (void*) req;
     return false;
 }
 
@@ -1178,6 +1180,11 @@
                          << QOrganizerItemType::TypeTodoOccurrence;
 }
 
+int QOrganizerEDSEngine::runningRequestCount() const
+{
+    return m_runningRequests.count();
+}
+
 void QOrganizerEDSEngine::onSourceAdded(const QString &collectionId)
 {
     d->watch(collectionId);

=== modified file 'organizer/qorganizer-eds-engine.h'
--- organizer/qorganizer-eds-engine.h	2014-05-24 21:09:00 +0000
+++ organizer/qorganizer-eds-engine.h	2015-03-06 22:48:43 +0000
@@ -125,6 +125,9 @@
     virtual QList<QtOrganizer::QOrganizerItemDetail::DetailType> supportedItemDetails(QtOrganizer::QOrganizerItemType::ItemType itemType) const;
     virtual QList<QtOrganizer::QOrganizerItemType::ItemType> supportedItemTypes() const;
 
+    // debug
+    int runningRequestCount() const;
+
 protected Q_SLOTS:
     void onSourceAdded(const QString &collectionId);
     void onSourceRemoved(const QString &collectionId);

=== modified file 'organizer/qorganizer-eds-requestdata.cpp'
--- organizer/qorganizer-eds-requestdata.cpp	2014-10-01 13:45:34 +0000
+++ organizer/qorganizer-eds-requestdata.cpp	2015-03-06 22:48:43 +0000
@@ -71,24 +71,13 @@
 
 void RequestData::cancel()
 {
-    QMutexLocker locker(&m_canceling);
     if (m_cancellable) {
-        GCancellable *cancellable = m_cancellable;
-        gulong id = g_cancellable_connect(cancellable,
-                                          (GCallback) RequestData::onCancelled,
-                                          this, NULL);
-
-
-        // cancel
-        g_cancellable_cancel(cancellable);
-
-        // wait the cancel
-        if (cancellable) {
-            wait();
-        }
-
-        // done
-        g_cancellable_disconnect(cancellable, id);
+        g_cancellable_cancel(m_cancellable);
+    }
+
+    if (isLive()) {
+        finish(QOrganizerManager::UnspecifiedError,
+               QOrganizerAbstractRequest::CanceledState);
     }
 }
 
@@ -110,42 +99,26 @@
     return result;
 }
 
-bool RequestData::isCanceling()
-{
-    bool result = true;
-    if (m_canceling.tryLock()) {
-        result = false;
-        m_canceling.unlock();
-    }
-    return result;
-}
-
 void RequestData::deleteLater()
 {
-    if (isWaiting() || isCanceling()) {
+    if (isWaiting()) {
         // still running
         return;
     }
-    m_parent->m_runningRequests.remove(m_req);
+    if (!m_parent.isNull()) {
+        m_parent->m_runningRequests.remove(m_req);
+    }
     delete this;
 }
 
-void RequestData::finish(QOrganizerManager::Error error, QtOrganizer::QOrganizerAbstractRequest::State state)
+void RequestData::finish(QOrganizerManager::Error error,
+                         QtOrganizer::QOrganizerAbstractRequest::State state)
 {
     Q_UNUSED(error);
     Q_UNUSED(state);
     m_finished = true;
 }
 
-void RequestData::onCancelled(GCancellable *cancellable, RequestData *self)
-{
-    Q_UNUSED(cancellable);
-    if (self->m_req) {
-        self->finish(QOrganizerManager::UnspecifiedError, QOrganizerAbstractRequest::CanceledState);
-    }
-    self->m_cancellable = 0;
-}
-
 void RequestData::setClient(EClient *client)
 {
     if (m_client == client) {

=== modified file 'organizer/qorganizer-eds-requestdata.h'
--- organizer/qorganizer-eds-requestdata.h	2014-05-25 19:46:49 +0000
+++ organizer/qorganizer-eds-requestdata.h	2015-03-06 22:48:43 +0000
@@ -44,7 +44,6 @@
     virtual void finish(QtOrganizer::QOrganizerManager::Error error, QtOrganizer::QOrganizerAbstractRequest::State state) = 0;
     void wait();
     bool isWaiting();
-    bool isCanceling();
 
     template<class T>
     T* request() const {
@@ -53,15 +52,16 @@
 
     template<class T>
     void emitChangeset(T *cs) {
-        m_parent->d->emitSharedSignals<T>(cs);
+        if (!m_parent.isNull()) {
+             m_parent->d->emitSharedSignals<T>(cs);
+        }
     }
 
 protected:
-    QOrganizerEDSEngine *m_parent;
+    QPointer<QOrganizerEDSEngine> m_parent;
     EClient *m_client;
     QtOrganizer::QOrganizerItemChangeSet m_changeSet;
     QMutex m_waiting;
-    QMutex m_canceling;
     bool m_finished;
 
     virtual ~RequestData();
@@ -69,8 +69,6 @@
 private:
     QPointer<QtOrganizer::QOrganizerAbstractRequest> m_req;
     GCancellable *m_cancellable;
-
-    static void onCancelled(GCancellable *cancellable, RequestData *self);
 };
 
 #endif

=== modified file 'tests/unittest/cancel-operation-test.cpp'
--- tests/unittest/cancel-operation-test.cpp	2014-10-06 14:07:54 +0000
+++ tests/unittest/cancel-operation-test.cpp	2015-03-06 22:48:43 +0000
@@ -40,8 +40,9 @@
     void init()
     {
         EDSBaseTest::init();
-
-        m_engine = QOrganizerEDSEngine::createEDSEngine(QMap<QString, QString>());
+        QMap<QString, QString> parameters;
+        parameters.insert("sleepMode", "true");
+        m_engine = QOrganizerEDSEngine::createEDSEngine(parameters);
 
         QtOrganizer::QOrganizerManager::Error error;
         m_collection = QOrganizerCollection();
@@ -59,20 +60,17 @@
         EDSBaseTest::cleanup();
     }
 
-    void cancelOperationBeforeStart()
+    void cancelOperationAfterStart()
     {
-        // initial collections
-        QList<QOrganizerCollection> collections = m_engine->collections(0);
-        int initialCollectionsCount = collections.count();
-
         QOrganizerEvent event;
         event.setStartDateTime(QDateTime::currentDateTime());
         event.setDisplayLabel("displayLabelValue");
         event.setDescription("descriptionValue");
         event.setCollectionId(m_collection.id());
 
+        QSignalSpy createdItem(m_engine, SIGNAL(itemsAdded(QList<QOrganizerItemId>)));
+
         // Try cancel a create item operation
-        QSignalSpy createdItem(m_engine, SIGNAL(itemsAdded(QList<QOrganizerItemId>)));
         QOrganizerItemSaveRequest req;
         req.setItem(event);
         m_engine->startRequest(&req);
@@ -81,11 +79,89 @@
         QCOMPARE(req.state(), QOrganizerAbstractRequest::CanceledState);
         QTRY_COMPARE(createdItem.count(), 0);
 
-        // check if collection was not create
-        collections = m_engine->collections(0);
-        QCOMPARE(collections.count(), initialCollectionsCount);
-    }
-
+        QTRY_COMPARE(m_engine->runningRequestCount(), 0);
+    }
+
+    void deleteManagerBeforeRequestFinish()
+    {
+        QOrganizerEvent event;
+        event.setStartDateTime(QDateTime::currentDateTime());
+        event.setDisplayLabel("displayLabelValue");
+        event.setDescription("descriptionValue");
+        event.setCollectionId(m_collection.id());
+
+        QOrganizerEDSEngine *engine = QOrganizerEDSEngine::createEDSEngine(QMap<QString, QString>());
+
+        // Start a request
+        QOrganizerItemSaveRequest req;
+        req.setItem(event);
+        engine->startRequest(&req);
+        QCOMPARE(req.state(), QOrganizerAbstractRequest::ActiveState);
+
+        // delete engine
+        delete engine;
+    }
+
+    void cancelBeforeStart()
+    {
+        QOrganizerEvent event;
+        event.setStartDateTime(QDateTime::currentDateTime());
+        event.setDisplayLabel("displayLabelValue");
+        event.setDescription("descriptionValue");
+        event.setCollectionId(m_collection.id());
+
+        // Cancel before start
+        QOrganizerItemSaveRequest req;
+        req.setItem(event);
+        m_engine->cancelRequest(&req);
+
+        QTRY_COMPARE(m_engine->runningRequestCount(), 0);
+    }
+
+    void destroyRequestAfterStart()
+    {
+        QOrganizerEvent event;
+        event.setStartDateTime(QDateTime::currentDateTime());
+        event.setDisplayLabel("displayLabelValue");
+        event.setDescription("descriptionValue");
+        event.setCollectionId(m_collection.id());
+
+        QOrganizerManager *mgr = new QOrganizerManager("eds");
+        QOrganizerItemSaveRequest *req = new QOrganizerItemSaveRequest;
+        req->setManager(mgr);
+        req->setItem(event);
+        req->start();
+        req->deleteLater();
+
+        delete mgr;
+    }
+
+    void startMultipleRequests()
+    {
+        QList<QOrganizerCollection> collections = m_engine->collections(0);
+
+        QList<QOrganizerItemSaveRequest*> requests;
+        for(int i=0; i < 100; i++) {
+            QOrganizerEvent event;
+            event.setStartDateTime(QDateTime::currentDateTime());
+            event.setDisplayLabel(QString("displayLabelValue_%1").arg(i));
+            event.setDescription(QString("descriptionValue_%2").arg(i));
+            event.setCollectionId(m_collection.id());
+
+            QOrganizerItemSaveRequest *req = new QOrganizerItemSaveRequest;
+            req->setItem(event);
+            m_engine->startRequest(req);
+            QCOMPARE(req->state(), QOrganizerAbstractRequest::ActiveState);
+            requests << req;
+        }
+
+        Q_FOREACH(QOrganizerItemSaveRequest *r, requests) {
+            m_engine->cancelRequest(r);
+            QCOMPARE(r->state(), QOrganizerAbstractRequest::CanceledState);
+        }
+
+        QTRY_COMPARE(m_engine->runningRequestCount(), 0);
+    }
 };
 
 QTEST_MAIN(CancelOperationTest)