From c75ee8a6619e6c546121462d28e221bb8df11f19 Mon Sep 17 00:00:00 2001 From: tux3 Date: Fri, 17 Feb 2017 05:55:23 +0100 Subject: [PATCH] fix: Various IPC event handling and related bugs on startup Fixes #1926 : When an IPC event was processed locally, if the window was closed before the core could start, the event handler would be forever stuck in the background waiting for the core to start. We fix this by substituting QApplication::quit() by a Nexus::quit() function and a Nexus::isRunning() function, which gives us a condition for exiting blocking processEvents() loops. We cannot simply use QApplication::quit(), because this function has no effect before the start of the event loop. The problem was further exacerbated by the Tox URI event handler being (incorrectly) blocking. The IPC owner would block in this event handler, and the sender of the event would give up waiting and process the event itself a second time, potentially triggering the first bug. We fix the event handlers accordingly to be (mostly) non-blocking. Also fixes a related deadlock between ~Core and ~Profile in the case of an early exit --- src/core/core.cpp | 9 +++++--- src/ipc.cpp | 4 ++++ src/main.cpp | 9 +++++--- src/net/toxuri.cpp | 33 ++++++++++++++++++++------- src/nexus.cpp | 45 ++++++++++++++++++++++++++++++++++--- src/nexus.h | 7 ++++++ src/persistence/profile.cpp | 5 ++++- src/widget/gui.cpp | 23 ++++++++++++++----- src/widget/loginscreen.cpp | 5 +++++ src/widget/loginscreen.h | 4 ++++ src/widget/widget.cpp | 4 ++-- 11 files changed, 122 insertions(+), 26 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index 0a62a00f6..be00452bc 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -94,10 +94,13 @@ Core::~Core() Q_ARG(bool, false)); } coreThread->exit(0); - while (coreThread->isRunning()) + if (QThread::currentThread() != coreThread) { - qApp->processEvents(); - coreThread->wait(500); + while (coreThread->isRunning()) + { + qApp->processEvents(); + coreThread->wait(500); + } } deadifyTox(); diff --git a/src/ipc.cpp b/src/ipc.cpp index 7895b129b..630db31ac 100644 --- a/src/ipc.cpp +++ b/src/ipc.cpp @@ -175,6 +175,10 @@ bool IPC::isCurrentOwner() } } +/** + * @brief Register a handler for an IPC event + * @param handler The handler callback. Should not block for more than a second, at worst + */ void IPC::registerEventHandler(const QString &name, IPCEventHandler handler) { eventHandlers[name] = handler; diff --git a/src/main.cpp b/src/main.cpp index f9898bdeb..bc6dbe892 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -295,7 +295,8 @@ int main(int argc, char *argv[]) } } - Nexus::getInstance().start(); + Nexus& nexus = Nexus::getInstance(); + nexus.start(); // Event was not handled by already running instance therefore we handle it ourselves if (eventType == "uri") @@ -303,8 +304,10 @@ int main(int argc, char *argv[]) else if (eventType == "save") handleToxSave(firstParam.toUtf8()); - // Run - int errorcode = a.exec(); + // Run (unless we already quit before starting!) + int errorcode = 0; + if (nexus.isRunning()) + errorcode = a.exec(); Nexus::destroyInstance(); CameraSource::destroyInstance(); diff --git a/src/net/toxuri.cpp b/src/net/toxuri.cpp index b4a1154ac..c91f96066 100644 --- a/src/net/toxuri.cpp +++ b/src/net/toxuri.cpp @@ -51,16 +51,25 @@ bool toxURIEventHandler(const QByteArray& eventData) */ bool handleToxURI(const QString &toxURI) { - Core* core = Core::getInstance(); + Nexus& nexus = Nexus::getInstance(); + Core* core = nexus.getCore(); while (!core) { - core = Core::getInstance(); + if (!nexus.isRunning()) + return false; + + core = nexus.getCore(); qApp->processEvents(); } while (!core->isReady()) + { + if (!nexus.isRunning()) + return false; + qApp->processEvents(); + } QString toxaddr = toxURI.mid(4); @@ -70,18 +79,26 @@ bool handleToxURI(const QString &toxURI) toxId = Toxme::lookup(toxaddr); if (!toxId.isValid()) { - QMessageBox::warning(0, "qTox", - ToxURIDialog::tr("%1 is not a valid Toxme address.") - .arg(toxaddr)); + QMessageBox *messageBox = new QMessageBox(QMessageBox::Warning, "qTox", + QMessageBox::tr("%1 is not a valid Toxme address.") + .arg(toxaddr), QMessageBox::Ok, nullptr); + messageBox->setButtonText(QMessageBox::Ok, QMessageBox::tr("Ok")); + QObject::connect(messageBox, &QMessageBox::finished, messageBox, &QMessageBox::deleteLater); + messageBox->show(); return false; } } - ToxURIDialog dialog(0, toxaddr, QObject::tr("%1 here! Tox me maybe?", + ToxURIDialog *dialog = new ToxURIDialog(0, toxaddr, QObject::tr("%1 here! Tox me maybe?", "Default message in Tox URI friend requests. Write something appropriate!") .arg(Nexus::getCore()->getUsername())); - if (dialog.exec() == QDialog::Accepted) - Core::getInstance()->requestFriendship(toxId, dialog.getRequestMessage()); + QObject::connect(dialog, &ToxURIDialog::finished, [=](int result) { + if (result == QDialog::Accepted) + Core::getInstance()->requestFriendship(toxId, dialog->getRequestMessage()); + + dialog->deleteLater(); + }); + dialog->open(); return true; } diff --git a/src/nexus.cpp b/src/nexus.cpp index b08762ed5..20a330be8 100644 --- a/src/nexus.cpp +++ b/src/nexus.cpp @@ -59,15 +59,20 @@ Nexus::Nexus(QObject *parent) : QObject(parent), profile{nullptr}, widget{nullptr}, - loginScreen{nullptr} + loginScreen{nullptr}, + running{true}, + quitOnLastWindowClosed{true} { } Nexus::~Nexus() { delete widget; + widget = nullptr; delete loginScreen; + loginScreen = nullptr; delete profile; + profile = nullptr; Settings::getInstance().saveGlobal(); #ifdef Q_OS_MAC delete globalMenuBar; @@ -104,6 +109,14 @@ void Nexus::start() loginScreen = new LoginScreen(); + // We need this LastWindowClosed dance because the LoginScreen may be shown + // and closed in a processEvents() loop before the start of the real + // exec() event loop, meaning we wouldn't receive the onLastWindowClosed, + // and so we wouldn't have a chance to tell the processEvents() loop to quit. + qApp->setQuitOnLastWindowClosed(false); + connect(qApp, &QApplication::lastWindowClosed, this, &Nexus::onLastWindowClosed); + connect(loginScreen, &LoginScreen::closed, this, &Nexus::onLastWindowClosed); + #ifdef Q_OS_MAC globalMenuBar = new QMenuBar(0); dockMenu = new QMenu(globalMenuBar); @@ -161,14 +174,14 @@ void Nexus::showLogin() loginScreen->reset(); loginScreen->move(QApplication::desktop()->screen()->rect().center() - loginScreen->rect().center()); loginScreen->show(); - ((QApplication*)qApp)->setQuitOnLastWindowClosed(true); + quitOnLastWindowClosed = true; } void Nexus::showMainGUI() { assert(profile); - ((QApplication*)qApp)->setQuitOnLastWindowClosed(false); + quitOnLastWindowClosed = false; loginScreen->close(); // Create GUI @@ -220,6 +233,26 @@ void Nexus::showMainGUI() profile->startCore(); } +/** + * @brief Calls QApplication::quit(), and causes Nexus::isRunning() to return false + */ +void Nexus::quit() +{ + running = false; + qApp->quit(); +} + +/** + * @brief Returns true until Nexus::quit is called. + * + * Any blocking processEvents() loop should check this as a return condition, + * since the application can not quit until control is returned to the event loop. + */ +bool Nexus::isRunning() +{ + return running; +} + /** * @brief Returns the singleton instance. */ @@ -310,6 +343,12 @@ void Nexus::showLoginLater() QMetaObject::invokeMethod(&getInstance(), "showLogin", Qt::QueuedConnection); } +void Nexus::onLastWindowClosed() +{ + if (quitOnLastWindowClosed) + quit(); +} + #ifdef Q_OS_MAC void Nexus::retranslateUi() { diff --git a/src/nexus.h b/src/nexus.h index 080482f39..97437719b 100644 --- a/src/nexus.h +++ b/src/nexus.h @@ -43,6 +43,8 @@ class Nexus : public QObject public: void start(); void showMainGUI(); + void quit(); + bool isRunning(); static Nexus& getInstance(); static void destroyInstance(); @@ -84,6 +86,9 @@ private: QActionGroup* windowActions = nullptr; #endif +private slots: + void onLastWindowClosed(); + private: explicit Nexus(QObject *parent = 0); ~Nexus(); @@ -92,6 +97,8 @@ private: Profile* profile; Widget* widget; LoginScreen* loginScreen; + bool running; + bool quitOnLastWindowClosed; }; #endif // NEXUS_H diff --git a/src/persistence/profile.cpp b/src/persistence/profile.cpp index 763a39e6b..74ec53a7b 100644 --- a/src/persistence/profile.cpp +++ b/src/persistence/profile.cpp @@ -211,7 +211,10 @@ Profile::~Profile() saveToxSave(); } - delete core; + core->deleteLater(); + while (coreThread->isRunning()) + qApp->processEvents(); + delete coreThread; if (!isRemoved) { diff --git a/src/widget/gui.cpp b/src/widget/gui.cpp index f8cdd4296..21274e827 100644 --- a/src/widget/gui.cpp +++ b/src/widget/gui.cpp @@ -322,25 +322,34 @@ QString GUI::passwordDialog(const QString& cancel, const QString& body) void GUI::_clearContacts() { - Nexus::getDesktopGUI()->clearContactsList(); + Widget* w = Nexus::getDesktopGUI(); + if (w) + w->clearContactsList(); } void GUI::_setEnabled(bool state) { - Nexus::getDesktopGUI()->setEnabled(state); + Widget* w = Nexus::getDesktopGUI(); + if (w) + w->setEnabled(state); } void GUI::_setWindowTitle(const QString& title) { + QWidget* w = getMainWidget(); + if (!w) + return; if (title.isEmpty()) - getMainWidget()->setWindowTitle("qTox"); + w->setWindowTitle("qTox"); else - getMainWidget()->setWindowTitle("qTox - " + title); + w->setWindowTitle("qTox - " + title); } void GUI::_reloadTheme() { - Nexus::getDesktopGUI()->reloadTheme(); + Widget* w = Nexus::getDesktopGUI(); + if (w) + w->reloadTheme(); } void GUI::_showInfo(const QString& title, const QString& msg) @@ -366,7 +375,9 @@ void GUI::_showError(const QString& title, const QString& msg) void GUI::_showUpdateDownloadProgress() { - Nexus::getDesktopGUI()->showUpdateDownloadProgress(); + Widget* w = Nexus::getDesktopGUI(); + if (w) + w->showUpdateDownloadProgress(); } bool GUI::_askQuestion(const QString& title, const QString& msg, diff --git a/src/widget/loginscreen.cpp b/src/widget/loginscreen.cpp index 24dfb0c84..0ad34a07c 100644 --- a/src/widget/loginscreen.cpp +++ b/src/widget/loginscreen.cpp @@ -126,6 +126,11 @@ bool LoginScreen::event(QEvent* event) return QWidget::event(event); } +void LoginScreen::closeEvent(QCloseEvent*) +{ + emit closed(); +} + void LoginScreen::onNewProfilePageClicked() { ui->stackedWidget->setCurrentIndex(0); diff --git a/src/widget/loginscreen.h b/src/widget/loginscreen.h index fdf223480..82441842c 100644 --- a/src/widget/loginscreen.h +++ b/src/widget/loginscreen.h @@ -42,6 +42,10 @@ public: signals: void windowStateChanged(Qt::WindowStates states); + void closed(); + +protected: + virtual void closeEvent(QCloseEvent *event) final override; private slots: void onAutoLoginToggled(int state); diff --git a/src/widget/widget.cpp b/src/widget/widget.cpp index 632b5a17e..cdaef7191 100644 --- a/src/widget/widget.cpp +++ b/src/widget/widget.cpp @@ -594,7 +594,7 @@ void Widget::closeEvent(QCloseEvent *event) saveWindowGeometry(); saveSplitterGeometry(); QWidget::closeEvent(event); - qApp->quit(); + Nexus::getInstance().quit(); } } @@ -647,7 +647,7 @@ void Widget::onFailedToStartCore() critical.setText(tr("toxcore failed to start, the application will terminate after you close this message.")); critical.setIcon(QMessageBox::Critical); critical.exec(); - qApp->quit(); + Nexus::getInstance().quit(); } void Widget::onBadProxyCore()