Merge bitcoin-core/gui#336: Do not exit and re-enter main event loop during shutdown

451ca244db qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov)
f3a17bbe5f qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov)
b4e0d2c431 qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov)
332dea2852 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov)
c8bae37a7a qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov)
7fa91e8312 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov)
6f6fde30e7 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov)
59f7ba4fd7 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov)
7830cd0b35 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov)
13f618818d qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)

Pull request description:

  On master (1ef34ee25e) during shutdown `QApplication` exits the main event loop, then re-enter again.

  This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59.

  Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO).

  The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.

  This PR does not change behavior, and all touched dialogs are still application modal.
  As a follow up, a design research could suggest to make some dialogs window modal.

  NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.

ACKs for top commit:
  laanwj:
    Code review and lighly tested ACK 451ca244db
  promag:
    ACK 451ca244db, just changed signal to `quitRequested`.

Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447

src/qt/addressbookpage.cpp

@@ -182,14 +182,14 @@ void AddressBookPage::onEditAction()
     if(indexes.isEmpty())
         return;
 
-    EditAddressDialog dlg(
+    auto dlg = new EditAddressDialog(
         tab == SendingTab ?
         EditAddressDialog::EditSendingAddress :
         EditAddressDialog::EditReceivingAddress, this);
-    dlg.setModel(model);
+    dlg->setModel(model);
     QModelIndex origIndex = proxyModel->mapToSource(indexes.at(0));
-    dlg.loadRow(origIndex.row());
+    dlg->loadRow(origIndex.row());
-    dlg.exec();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void AddressBookPage::on_newAddress_clicked()

src/qt/bitcoin.cpp

@@ -55,6 +55,7 @@
 #include <QThread>
 #include <QTimer>
 #include <QTranslator>
+#include <QWindow>
 
 #if defined(QT_STATICPLUGIN)
 #include <QtPlugin>
@@ -259,6 +260,7 @@ void BitcoinApplication::createOptionsModel(bool resetSettings)
 void BitcoinApplication::createWindow(const NetworkStyle *networkStyle)
 {
     window = new BitcoinGUI(node(), platformStyle, networkStyle, nullptr);
+    connect(window, &BitcoinGUI::quitRequested, this, &BitcoinApplication::requestShutdown);
 
     pollShutdownTimer = new QTimer(window);
     connect(pollShutdownTimer, &QTimer::timeout, window, &BitcoinGUI::detectShutdown);
@@ -296,7 +298,7 @@ void BitcoinApplication::startThread()
 
     /*  communication to and from thread */
     connect(&m_executor.value(), &InitExecutor::initializeResult, this, &BitcoinApplication::initializeResult);
-    connect(&m_executor.value(), &InitExecutor::shutdownResult, this, &BitcoinApplication::shutdownResult);
+    connect(&m_executor.value(), &InitExecutor::shutdownResult, this, &QCoreApplication::quit);
     connect(&m_executor.value(), &InitExecutor::runawayException, this, &BitcoinApplication::handleRunawayException);
     connect(this, &BitcoinApplication::requestedInitialize, &m_executor.value(), &InitExecutor::initialize);
     connect(this, &BitcoinApplication::requestedShutdown, &m_executor.value(), &InitExecutor::shutdown);
@@ -326,13 +328,17 @@ void BitcoinApplication::requestInitialize()
 
 void BitcoinApplication::requestShutdown()
 {
+    for (const auto w : QGuiApplication::topLevelWindows()) {
+        w->hide();
+    }
+
     // Show a simple window indicating shutdown status
     // Do this first as some of the steps may take some time below,
     // for example the RPC console may still be executing a command.
     shutdownWindow.reset(ShutdownWindow::showShutdownWindow(window));
 
     qDebug() << __func__ << ": Requesting shutdown";
-    window->hide();
+
     // Must disconnect node signals otherwise current thread can deadlock since
     // no event loop is running.
     window->unsubscribeFromCoreSignals();
@@ -409,15 +415,10 @@ void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHead
         pollShutdownTimer->start(200);
     } else {
         Q_EMIT splashFinished(); // Make sure splash screen doesn't stick around during shutdown
-        quit(); // Exit first main loop invocation
+        requestShutdown();
     }
 }
 
-void BitcoinApplication::shutdownResult()
-{
-    quit(); // Exit second main loop invocation after shutdown finished
-}
-
 void BitcoinApplication::handleRunawayException(const QString &message)
 {
     QMessageBox::critical(
@@ -639,8 +640,6 @@ int GuiMain(int argc, char* argv[])
 #if defined(Q_OS_WIN)
             WinShutdownMonitor::registerShutdownBlockReason(QObject::tr("%1 didn't yet exit safely…").arg(PACKAGE_NAME), (HWND)app.getMainWinId());
 #endif
-            app.exec();
-            app.requestShutdown();
             app.exec();
             rv = app.getReturnValue();
         } else {

src/qt/bitcoin.h

@@ -61,8 +61,6 @@ public:
 
     /// Request core initialization
     void requestInitialize();
-    /// Request core shutdown
-    void requestShutdown();
 
     /// Get process return value
     int getReturnValue() const { return returnValue; }
@@ -77,7 +75,8 @@ public:
 
 public Q_SLOTS:
     void initializeResult(bool success, interfaces::BlockAndHeaderTipInfo tip_info);
-    void shutdownResult();
+    /// Request core shutdown
+    void requestShutdown();
     /// Handle runaway exceptions. Shows a message box with the problem and quits the program.
     void handleRunawayException(const QString &message);
 

src/qt/bitcoingui.cpp

@@ -373,7 +373,7 @@ void BitcoinGUI::createActions()
     m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
     m_mask_values_action->setCheckable(true);
 
-    connect(quitAction, &QAction::triggered, qApp, QApplication::quit);
+    connect(quitAction, &QAction::triggered, this, &BitcoinGUI::quitRequested);
     connect(aboutAction, &QAction::triggered, this, &BitcoinGUI::aboutClicked);
     connect(aboutQtAction, &QAction::triggered, qApp, QApplication::aboutQt);
     connect(optionsAction, &QAction::triggered, this, &BitcoinGUI::optionsClicked);
@@ -850,8 +850,8 @@ void BitcoinGUI::aboutClicked()
     if(!clientModel)
         return;
 
-    HelpMessageDialog dlg(this, true);
+    auto dlg = new HelpMessageDialog(this, /* about */ true);
-    dlg.exec();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void BitcoinGUI::showDebugWindow()
@@ -992,10 +992,11 @@ void BitcoinGUI::openOptionsDialogWithTab(OptionsDialog::Tab tab)
     if (!clientModel || !clientModel->getOptionsModel())
         return;
 
-    OptionsDialog dlg(this, enableWallet);
+    auto dlg = new OptionsDialog(this, enableWallet);
-    dlg.setCurrentTab(tab);
+    connect(dlg, &OptionsDialog::quitOnReset, this, &BitcoinGUI::quitRequested);
-    dlg.setModel(clientModel->getOptionsModel());
+    dlg->setCurrentTab(tab);
-    dlg.exec();
+    dlg->setModel(clientModel->getOptionsModel());
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, double nVerificationProgress, bool header, SynchronizationState sync_state)
@@ -1218,7 +1219,7 @@ void BitcoinGUI::closeEvent(QCloseEvent *event)
             // close rpcConsole in case it was open to make some space for the shutdown window
             rpcConsole->close();
 
-            QApplication::quit();
+            Q_EMIT quitRequested();
         }
         else
         {
@@ -1412,7 +1413,7 @@ void BitcoinGUI::detectShutdown()
     {
         if(rpcConsole)
             rpcConsole->hide();
-        qApp->quit();
+        Q_EMIT quitRequested();
     }
 }
 

src/qt/bitcoingui.h

@@ -214,6 +214,7 @@ private:
     void openOptionsDialogWithTab(OptionsDialog::Tab tab);
 
 Q_SIGNALS:
+    void quitRequested();
     /** Signal raised when a URI was entered or dragged to the GUI */
     void receivedURI(const QString &uri);
     /** Signal raised when RPC console shown */

src/qt/guiutil.cpp

@@ -36,6 +36,7 @@
 #include <QClipboard>
 #include <QDateTime>
 #include <QDesktopServices>
+#include <QDialog>
 #include <QDoubleValidator>
 #include <QFileDialog>
 #include <QFont>
@@ -970,4 +971,11 @@ void PrintSlotException(
     PrintExceptionContinue(exception, description.c_str());
 }
 
+void ShowModalDialogAndDeleteOnClose(QDialog* dialog)
+{
+    dialog->setAttribute(Qt::WA_DeleteOnClose);
+    dialog->setWindowModality(Qt::ApplicationModal);
+    dialog->show();
+}
+
 } // namespace GUIUtil

src/qt/guiutil.h

@@ -41,6 +41,7 @@ class QAbstractButton;
 class QAbstractItemView;
 class QAction;
 class QDateTime;
+class QDialog;
 class QFont;
 class QKeySequence;
 class QLineEdit;
@@ -417,6 +418,11 @@ namespace GUIUtil
             type);
     }
 
+    /**
+     * Shows a QDialog instance asynchronously, and deletes it on close.
+     */
+    void ShowModalDialogAndDeleteOnClose(QDialog* dialog);
+
 } // namespace GUIUtil
 
 #endif // BITCOIN_QT_GUIUTIL_H

src/qt/optionsdialog.cpp

@@ -292,7 +292,8 @@ void OptionsDialog::on_resetButton_clicked()
 
         /* reset all options and close GUI */
         model->Reset();
-        QApplication::quit();
+        close();
+        Q_EMIT quitOnReset();
     }
 }
 

src/qt/optionsdialog.h

@@ -68,6 +68,7 @@ private Q_SLOTS:
 
 Q_SIGNALS:
     void proxyIpChecks(QValidatedLineEdit *pUiProxyIp, uint16_t nProxyPort);
+    void quitOnReset();
 
 private:
     Ui::OptionsDialog *ui;

src/qt/sendcoinsdialog.cpp

@@ -399,9 +399,10 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
 
     const QString confirmation = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Confirm transaction proposal") : tr("Confirm send coins");
     const QString confirmButtonText = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Create Unsigned") : tr("Sign and send");
-    SendConfirmationDialog confirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this);
+    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this);
-    confirmationDialog.exec();
+    confirmationDialog->setAttribute(Qt::WA_DeleteOnClose);
-    QMessageBox::StandardButton retval = static_cast<QMessageBox::StandardButton>(confirmationDialog.result());
+    // TODO: Replace QDialog::exec() with safer QDialog::show().
+    const auto retval = static_cast<QMessageBox::StandardButton>(confirmationDialog->exec());
 
     if(retval != QMessageBox::Yes)
     {
@@ -914,9 +915,9 @@ void SendCoinsDialog::coinControlFeatureChanged(bool checked)
 // Coin Control: button inputs -> show actual coin control dialog
 void SendCoinsDialog::coinControlButtonClicked()
 {
-    CoinControlDialog dlg(*m_coin_control, model, platformStyle);
+    auto dlg = new CoinControlDialog(*m_coin_control, model, platformStyle);
-    dlg.exec();
+    connect(dlg, &QDialog::finished, this, &SendCoinsDialog::coinControlUpdateLabels);
-    coinControlUpdateLabels();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 // Coin Control: checkbox custom change address

src/qt/transactionview.cpp

@@ -504,22 +504,22 @@ void TransactionView::editLabel()
             // Determine type of address, launch appropriate editor dialog type
             QString type = modelIdx.data(AddressTableModel::TypeRole).toString();
 
-            EditAddressDialog dlg(
+            auto dlg = new EditAddressDialog(
                 type == AddressTableModel::Receive
                 ? EditAddressDialog::EditReceivingAddress
                 : EditAddressDialog::EditSendingAddress, this);
-            dlg.setModel(addressBook);
+            dlg->setModel(addressBook);
-            dlg.loadRow(idx);
+            dlg->loadRow(idx);
-            dlg.exec();
+            GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
         }
         else
         {
             // Add sending address
-            EditAddressDialog dlg(EditAddressDialog::NewSendingAddress,
+            auto dlg = new EditAddressDialog(EditAddressDialog::NewSendingAddress,
                 this);
-            dlg.setModel(addressBook);
+            dlg->setModel(addressBook);
-            dlg.setAddress(address);
+            dlg->setAddress(address);
-            dlg.exec();
+            GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
         }
     }
 }

src/qt/walletframe.cpp

@@ -221,10 +221,9 @@ void WalletFrame::gotoLoadPSBT(bool from_clipboard)
         return;
     }
 
-    PSBTOperationsDialog* dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);
+    auto dlg = new PSBTOperationsDialog(this, currentWalletModel(), clientModel);
     dlg->openWithPSBT(psbtx);
-    dlg->setAttribute(Qt::WA_DeleteOnClose);
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
-    dlg->exec();
 }
 
 void WalletFrame::encryptWallet()

src/qt/walletmodel.cpp

@@ -506,9 +506,10 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
         questionString.append(tr("Warning: This may pay the additional fee by reducing change outputs or adding inputs, when necessary. It may add a new change output if one does not already exist. These changes may potentially leak privacy."));
     }
 
-    SendConfirmationDialog confirmationDialog(tr("Confirm fee bump"), questionString);
+    auto confirmationDialog = new SendConfirmationDialog(tr("Confirm fee bump"), questionString);
-    confirmationDialog.exec();
+    confirmationDialog->setAttribute(Qt::WA_DeleteOnClose);
-    QMessageBox::StandardButton retval = static_cast<QMessageBox::StandardButton>(confirmationDialog.result());
+    // TODO: Replace QDialog::exec() with safer QDialog::show().
+    const auto retval = static_cast<QMessageBox::StandardButton>(confirmationDialog->exec());
 
     // cancel sign&broadcast if user doesn't want to bump the fee
     if (retval != QMessageBox::Yes) {

src/qt/walletview.cpp

@@ -205,11 +205,10 @@ void WalletView::showOutOfSyncWarning(bool fShow)
 
 void WalletView::encryptWallet()
 {
-    AskPassphraseDialog dlg(AskPassphraseDialog::Encrypt, this);
+    auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Encrypt, this);
-    dlg.setModel(walletModel);
+    dlg->setModel(walletModel);
-    dlg.exec();
+    connect(dlg, &QDialog::finished, this, &WalletView::encryptionStatusChanged);
-
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
-    Q_EMIT encryptionStatusChanged();
 }
 
 void WalletView::backupWallet()
@@ -234,19 +233,18 @@ void WalletView::backupWallet()
 
 void WalletView::changePassphrase()
 {
-    AskPassphraseDialog dlg(AskPassphraseDialog::ChangePass, this);
+    auto dlg = new AskPassphraseDialog(AskPassphraseDialog::ChangePass, this);
-    dlg.setModel(walletModel);
+    dlg->setModel(walletModel);
-    dlg.exec();
+    GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
 }
 
 void WalletView::unlockWallet()
 {
     // Unlock wallet when requested by wallet model
-    if (walletModel->getEncryptionStatus() == WalletModel::Locked)
+    if (walletModel->getEncryptionStatus() == WalletModel::Locked) {
-    {
+        auto dlg = new AskPassphraseDialog(AskPassphraseDialog::Unlock, this);
-        AskPassphraseDialog dlg(AskPassphraseDialog::Unlock, this);
+        dlg->setModel(walletModel);
-        dlg.setModel(walletModel);
+        GUIUtil::ShowModalDialogAndDeleteOnClose(dlg);
-        dlg.exec();
     }
 }