Cleanup thread cleaning and fix PinPAD crash

Fixes #346

Signed-off-by: Raul Metsma <raul@metsma.ee>

Author: metsma

src/controller/controller.cpp

@@ -28,10 +28,11 @@
 #include "threads/waitforcardthread.hpp"
 
 #include "utils/utils.hpp"
+
 #include "inputoutputmode.hpp"
 #include "writeresponse.hpp"
 
-#include <QApplication>
+#include <QCoreApplication>
 
 using namespace pcsc_cpp;
 using namespace electronic_id;
@@ -44,19 +45,11 @@ const QString RESP_USER_CANCEL = QStringLiteral("ERR_WEBEID_USER_CANCELLED");
 
 QVariantMap makeErrorObject(const QString& errorCode, const QString& errorMessage)
 {
-    const auto errorBody = QVariantMap {
-        {QStringLiteral("code"), errorCode},
-        {QStringLiteral("message"), errorMessage},
-    };
-    return {{QStringLiteral("error"), errorBody}};
-}
-
-void interruptThread(QThread* thread)
-{
-    qDebug() << "Interrupting thread" << uintptr_t(thread);
-    thread->disconnect();
-    thread->requestInterruption();
-    ControllerChildThread::waitForControllerNotify.wakeAll();
+    return {{QStringLiteral("error"),
+             QVariantMap {
+                 {QStringLiteral("code"), errorCode},
+                 {QStringLiteral("message"), errorMessage},
+             }}};
 }
 
 } // namespace
@@ -67,17 +60,19 @@ void Controller::run()
     const bool isInCommandLineMode = bool(command);
     isInStdinMode = !isInCommandLineMode;
 
-    qInfo() << qApp->applicationName() << "app" << qApp->applicationVersion() << "running in"
+    qInfo() << QCoreApplication::applicationName() << "app"
+            << QCoreApplication::applicationVersion() << "running in"
             << (isInStdinMode ? "stdin/stdout" : "command-line") << "mode";
 
     try {
         // TODO: cut out stdin mode separate class to avoid bugs in safari mode
         if (isInStdinMode) {
             // In stdin/stdout mode we first output the version as required by the WebExtension
             // and then wait for the actual command.
-            writeResponseToStdOut(isInStdinMode,
-                                  {{QStringLiteral("version"), qApp->applicationVersion()}},
-                                  "get-version");
+            writeResponseToStdOut(
+                isInStdinMode,
+                {{QStringLiteral("version"), QCoreApplication::applicationVersion()}},
+                "get-version");
 
             command = readCommandFromStdin();
         }
@@ -111,11 +106,14 @@ void Controller::startCommandExecution()
     REQUIRE_NON_NULL(commandHandler)
 
     // Reader monitor thread setup.
-    WaitForCardThread* waitForCardThread = new WaitForCardThread(this);
+    auto* waitForCardThread = new WaitForCardThread(this);
+    connect(this, &Controller::stopCardEventMonitorThread, waitForCardThread,
+            &WaitForCardThread::requestInterruption);
     connect(waitForCardThread, &WaitForCardThread::statusUpdate, this, &Controller::statusUpdate);
     connect(waitForCardThread, &WaitForCardThread::cardsAvailable, this,
             &Controller::onCardsAvailable);
-    saveChildThreadPtrAndConnectFailureFinish(waitForCardThread);
+    connect(waitForCardThread, &ControllerChildThread::failure, this,
+            &Controller::onCriticalFailure);
 
     // UI setup.
     window = WebEidUI::createAndShowDialog(commandHandler->commandType());
@@ -126,33 +124,6 @@ void Controller::startCommandExecution()
     waitForCardThread->start();
 }
 
-void Controller::saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread)
-{
-    REQUIRE_NON_NULL(childThread)
-    // Save the thread pointer in child thread tracking map to request interruption and wait for
-    // it to quit in waitForChildThreads().
-    childThreads[uintptr_t(childThread)] = childThread;
-
-    connect(childThread, &ControllerChildThread::failure, this, &Controller::onCriticalFailure);
-
-    // When the thread is finished, remove the pointer from the tracking map and call deleteLater()
-    // on it to free the thread object. Although the thread objects are freed through the Qt object
-    // tree ownership system anyway, it is better to delete them immediately when they finish.
-    connect(childThread, &ControllerChildThread::finished, this, [this, childThread]() {
-        QScopedPointer<ControllerChildThread, QScopedPointerDeleteLater> deleteLater {childThread};
-
-        const auto threadPtrAddress = uintptr_t(childThread);
-        if (childThreads.count(threadPtrAddress) && childThreads[threadPtrAddress]) {
-            childThreads[threadPtrAddress] = nullptr;
-            childThread->wait();
-            qDebug() << "Thread" << threadPtrAddress << "finished";
-        } else {
-            qWarning() << "Controller child thread" << childThread
-                       << "is missing or null in finish slot";
-        }
-    });
-}
-
 void Controller::connectOkCancelWaitingForPinPad()
 {
     REQUIRE_NON_NULL(window)
@@ -192,9 +163,10 @@ void Controller::onCardsAvailable(
 void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availableEids)
 {
     try {
-        CommandHandlerRunThread* commandHandlerRunThread =
+        auto* commandHandlerRunThread =
             new CommandHandlerRunThread(this, *commandHandler, availableEids);
-        saveChildThreadPtrAndConnectFailureFinish(commandHandlerRunThread);
+        connect(commandHandlerRunThread, &ControllerChildThread::failure, this,
+                &Controller::onCriticalFailure);
         connectRetry(commandHandlerRunThread);
 
         // When the command handler run thread retrieves certificates successfully, call
@@ -213,51 +185,32 @@ void Controller::runCommandHandler(const std::vector<ElectronicID::ptr>& availab
 
 void Controller::onCertificatesLoaded()
 {
-    CardEventMonitorThread* cardEventMonitorThread =
-        new CardEventMonitorThread(this, std::string(commandType()));
-    saveChildThreadPtrAndConnectFailureFinish(cardEventMonitorThread);
-    cardEventMonitorThreadKey = uintptr_t(cardEventMonitorThread);
+    auto* cardEventMonitorThread = new CardEventMonitorThread(this, commandType());
+    connect(cardEventMonitorThread, &ControllerChildThread::failure, this,
+            &Controller::onCriticalFailure);
+    connect(this, &Controller::stopCardEventMonitorThread, cardEventMonitorThread,
+            &WaitForCardThread::requestInterruption);
     connect(cardEventMonitorThread, &CardEventMonitorThread::cardEvent, this, &Controller::onRetry);
     cardEventMonitorThread->start();
 }
 
-void Controller::stopCardEventMonitorThread()
-{
-    if (cardEventMonitorThreadKey) {
-        try {
-            auto cardEventMonitorThread = childThreads.at(cardEventMonitorThreadKey);
-            cardEventMonitorThreadKey = 0;
-            if (cardEventMonitorThread) {
-                interruptThread(cardEventMonitorThread);
-            }
-        } catch (const std::out_of_range&) {
-            qWarning() << "Card event monitor thread" << cardEventMonitorThreadKey
-                       << "is missing from childThreads map in stopCardEventMonitorThread()";
-            cardEventMonitorThreadKey = 0;
-        }
-    }
-}
-
 void Controller::disposeUI()
 {
-    if (window) {
-        window->disconnect();
-        // As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
-        window->close();
-        window = nullptr;
-    }
+    delete window;
+    window = nullptr;
 }
 
 void Controller::onConfirmCommandHandler(const EidCertificateAndPinInfo& certAndPinInfo)
 {
-    stopCardEventMonitorThread();
+    emit stopCardEventMonitorThread();
 
     try {
-        CommandHandlerConfirmThread* commandHandlerConfirmThread =
+        auto* commandHandlerConfirmThread =
             new CommandHandlerConfirmThread(this, *commandHandler, window, certAndPinInfo);
         connect(commandHandlerConfirmThread, &CommandHandlerConfirmThread::completed, this,
                 &Controller::onCommandHandlerConfirmCompleted);
-        saveChildThreadPtrAndConnectFailureFinish(commandHandlerConfirmThread);
+        connect(commandHandlerConfirmThread, &ControllerChildThread::failure, this,
+                &Controller::onCriticalFailure);
         connectRetry(commandHandlerConfirmThread);
 
         commandHandlerConfirmThread->start();
@@ -351,6 +304,7 @@ void Controller::onPinPadCancel()
 
 void Controller::onCriticalFailure(const QString& error)
 {
+    emit stopCardEventMonitorThread();
     qCritical() << "Exiting due to command" << std::string(commandType())
                 << "fatal error:" << error;
     _result =
@@ -373,18 +327,15 @@ void Controller::exit()
 
 void Controller::waitForChildThreads()
 {
-    // Waiting for child threads must not happen in destructor.
-    // See https://tombarta.wordpress.com/2008/07/10/gcc-pure-virtual-method-called/ for details.
-    for (const auto& childThread : childThreads) {
-        auto thread = childThread.second;
-        if (thread) {
-            interruptThread(thread);
-            // Waiting for PIN input on PIN pad may take a long time, call processEvents() so that
-            // the UI doesn't freeze.
-            while (thread->isRunning()) {
-                thread->wait(100); // in milliseconds
-                QCoreApplication::processEvents();
-            }
+    for (auto* thread : findChildren<QThread*>()) {
+        qDebug() << "Interrupting thread" << uintptr_t(thread);
+        thread->disconnect();
+        thread->requestInterruption();
+        ControllerChildThread::waitForControllerNotify.wakeAll();
+        // Call processEvents() so that the UI doesn't freeze.
+        while (thread->isRunning()) {
+            thread->wait(100ms);
+            QCoreApplication::processEvents();
         }
     }
 }

src/controller/controller.hpp

@@ -41,7 +41,8 @@ class Controller : public QObject
 
 signals:
     void quit();
-    void statusUpdate(const RetriableError status);
+    void statusUpdate(RetriableError status);
+    void stopCardEventMonitorThread();
 
 public: // slots
     void run();
@@ -76,18 +77,13 @@ class Controller : public QObject
     void runCommandHandler(const std::vector<electronic_id::ElectronicID::ptr>& availableEids);
     void connectOkCancelWaitingForPinPad();
     void connectRetry(const ControllerChildThread* childThread);
-    void saveChildThreadPtrAndConnectFailureFinish(ControllerChildThread* childThread);
-    void stopCardEventMonitorThread();
     void disposeUI();
     void exit();
     void waitForChildThreads();
     CommandType commandType();
 
     CommandWithArgumentsPtr command;
     CommandHandler::ptr commandHandler = nullptr;
-    std::unordered_map<uintptr_t, observer_ptr<ControllerChildThread>> childThreads;
-    // Key of card event monitor thread in childThreads map.
-    uintptr_t cardEventMonitorThreadKey = 0;
     // As the Qt::WA_DeleteOnClose flag is set, the dialog is deleted automatically.
     observer_ptr<WebEidUI> window = nullptr;
     QVariantMap _result;

src/controller/threads/cardeventmonitorthread.hpp

@@ -33,22 +33,23 @@ class CardEventMonitorThread : public ControllerChildThread
     using eid_ptr_vector = std::vector<electronic_id::ElectronicID::ptr>;
 
     CardEventMonitorThread(QObject* parent, std::string commandType) :
-        ControllerChildThread(parent), cmdType(std::move(commandType))
+        ControllerChildThread(std::move(commandType), parent)
     {
     }
 
     void run() override
     {
         QMutexLocker lock {&controllerChildThreadMutex};
 
-        beforeRun();
+        qDebug() << "Starting" << metaObject()->className() << uintptr_t(this) << "for command"
+                 << commandType();
 
         auto initialCards = getSupportedCardsIgnoringExceptions();
         sortByReaderNameAndAtr(initialCards);
 
         while (!isInterruptionRequested()) {
 
-            waitForControllerNotify.wait(&controllerChildThreadMutex, ONE_SECOND);
+            waitForControllerNotify.wait(&controllerChildThreadMutex, 1s);
 
             eid_ptr_vector updatedCards {};
 
@@ -57,7 +58,8 @@ class CardEventMonitorThread : public ControllerChildThread
                 sortByReaderNameAndAtr(updatedCards);
             } catch (const std::exception& error) {
                 // Ignore smart card layer errors, they will be handled during next card operation.
-                qWarning() << className << "ignoring" << commandType() << "error:" << error;
+                qWarning() << metaObject()->className() << "ignoring" << commandType()
+                           << "error:" << error;
             }
 
             // If interruption was requested during wait, exit without emitting.
@@ -67,7 +69,7 @@ class CardEventMonitorThread : public ControllerChildThread
 
             // If there was a change in connected supported cards, exit after emitting a card event.
             if (!areEqualByReaderNameAndAtr(initialCards, updatedCards)) {
-                qDebug() << className << "card change detected";
+                qDebug() << metaObject()->className() << "card change detected";
                 emit cardEvent();
                 return;
             }
@@ -90,9 +92,10 @@ class CardEventMonitorThread : public ControllerChildThread
                 return electronic_id::availableSupportedCards();
             } catch (const std::exception& error) {
                 // Ignore smart card layer errors, they will be handled during next card operation.
-                qWarning() << className << "ignoring" << commandType() << "error:" << error;
+                qWarning() << metaObject()->className() << "ignoring" << commandType()
+                           << "error:" << error;
             }
-            waitForControllerNotify.wait(&controllerChildThreadMutex, ONE_SECOND);
+            waitForControllerNotify.wait(&controllerChildThreadMutex, 1s);
         }
         // Interruption was requested, return empty list.
         return {};
@@ -110,16 +113,10 @@ class CardEventMonitorThread : public ControllerChildThread
 
     static bool areEqualByReaderNameAndAtr(const eid_ptr_vector& a, const eid_ptr_vector& b)
     {
-        // std::equal requires that second range is not shorter than first, so compare size first.
-        return a.size() == b.size()
-            && std::equal(a.cbegin(), a.cend(), b.cbegin(),
+        return std::equal(a.cbegin(), a.cend(), b.cbegin(), b.cend(),
                           [](const eid_ptr& c1, const eid_ptr& c2) {
                               return c1->smartcard().readerName() == c2->smartcard().readerName()
                                   && c1->smartcard().atr() == c2->smartcard().atr();
                           });
     }
-
-    const std::string& commandType() const override { return cmdType; }
-
-    const std::string cmdType;
 };

src/controller/threads/commandhandlerconfirmthread.hpp

@@ -31,8 +31,8 @@ class CommandHandlerConfirmThread : public ControllerChildThread
 public:
     CommandHandlerConfirmThread(QObject* parent, CommandHandler& handler, WebEidUI* w,
                                 const EidCertificateAndPinInfo& certAndPin) :
-        ControllerChildThread(parent), commandHandler(handler),
-        cmdType(commandHandler.commandType()), window(w), certAndPinInfo(certAndPin)
+        ControllerChildThread(handler.commandType(), parent), commandHandler(handler), window(w),
+        certAndPinInfo(certAndPin)
     {
     }
 
@@ -47,10 +47,7 @@ class CommandHandlerConfirmThread : public ControllerChildThread
         emit completed(result);
     }
 
-    const std::string& commandType() const override { return cmdType; }
-
     CommandHandler& commandHandler;
-    const std::string cmdType;
     WebEidUI* window;
     EidCertificateAndPinInfo certAndPinInfo;
 };

src/controller/threads/commandhandlerrunthread.hpp

@@ -31,8 +31,7 @@ class CommandHandlerRunThread : public ControllerChildThread
 public:
     CommandHandlerRunThread(QObject* parent, CommandHandler& handler,
                             const std::vector<electronic_id::ElectronicID::ptr>& eids) :
-        ControllerChildThread(parent), commandHandler(handler),
-        cmdType(commandHandler.commandType()), eids(eids)
+        ControllerChildThread(handler.commandType(), parent), commandHandler(handler), eids(eids)
     {
         // Connect retry signal to retry signal to pass it up from the command handler.
         connect(&commandHandler, &CommandHandler::retry, this, &ControllerChildThread::retry);
@@ -41,9 +40,6 @@ class CommandHandlerRunThread : public ControllerChildThread
 private:
     void doRun() override { commandHandler.run(eids); }
 
-    const std::string& commandType() const override { return cmdType; }
-
     CommandHandler& commandHandler;
-    const std::string cmdType;
     std::vector<electronic_id::ElectronicID::ptr> eids;
 };