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

@@ -94,10 +94,13 @@ Core::~Core()
                                       Q_ARG(bool, false));
     }
     coreThread->exit(0);
-    while (coreThread->isRunning())
+    if (QThread::currentThread() != coreThread)
     {
-        qApp->processEvents();
+        while (coreThread->isRunning())
-        coreThread->wait(500);
+        {
+            qApp->processEvents();
+            coreThread->wait(500);
+        }
     }
 
     deadifyTox();

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;

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
+    // Run (unless we already quit before starting!)
-    int errorcode = a.exec();
+    int errorcode = 0;
+    if (nexus.isRunning())
+        errorcode = a.exec();
 
     Nexus::destroyInstance();
     CameraSource::destroyInstance();

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",
+            QMessageBox *messageBox = new QMessageBox(QMessageBox::Warning, "qTox",
-                                         ToxURIDialog::tr("%1 is not a valid Toxme address.")
+                                   QMessageBox::tr("%1 is not a valid Toxme address.")
-                                         .arg(toxaddr));
+                                   .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)
+    QObject::connect(dialog, &ToxURIDialog::finished, [=](int result) {
-        Core::getInstance()->requestFriendship(toxId, dialog.getRequestMessage());
+        if (result == QDialog::Accepted)
+                Core::getInstance()->requestFriendship(toxId, dialog->getRequestMessage());
+
+        dialog->deleteLater();
+    });
+    dialog->open();
 
     return true;
 }

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()
 {

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

src/persistence/profile.cpp

@@ -211,7 +211,10 @@ Profile::~Profile()
         saveToxSave();
     }
 
-    delete core;
+    core->deleteLater();
+    while (coreThread->isRunning())
+        qApp->processEvents();
+
     delete coreThread;
     if (!isRemoved)
     {

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,

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);

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);

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()