Merge pull request #249

7fd80a1 async close: make sure curretWallet isn't used after shutdown started (Jaquee)
eb97bda code clean up and wallet close consistency (Jaquee)
ead9bd7 WalletManager: lock wallet while closing (Jaquee)

main.qml

@@ -109,30 +109,8 @@ ApplicationWindow {
             ctrlPressed = false
     }
 
-    function mousePressed(obj, mouseX, mouseY) {
-//        if(obj.objectName === "appWindow")
-//            obj = rootItem
-
-//        var tmp = rootItem.mapFromItem(obj, mouseX, mouseY)
-//        if(tmp !== undefined) {
-//            mouseX = tmp.x
-//            mouseY = tmp.y
-//        }
-
-//        if(currentItem !== undefined) {
-//            var tmp_x = rootItem.mapToItem(currentItem, mouseX, mouseY).x
-//            var tmp_y = rootItem.mapToItem(currentItem, mouseX, mouseY).y
-
-//            if(!currentItem.containsPoint(tmp_x, tmp_y)) {
-//                currentItem.hide()
-//                currentItem = undefined
-//            }
-//        }
-    }
-
-    function mouseReleased(obj, mouseX, mouseY) {
-
-    }
+    function mousePressed(obj, mouseX, mouseY) {}
+    function mouseReleased(obj, mouseX, mouseY) {}
 
     function openWalletFromFile(){
         persistentSettings.restore_height = 0
@@ -153,22 +131,11 @@ ApplicationWindow {
             translationManager.setLanguage(locale.split("_")[0]);
         }
 
-        // disconnect handlers before connecting
-        middlePanel.paymentClicked.disconnect(handlePayment);
-        // TODO: remove if statement when PR #111 is merged
-        if(typeof(handleCheckPayment) !== "undefined") {
-            middlePanel.checkPaymentClicked.disconnect(handleCheckPayment);
-        }
-        middlePanel.paymentClicked.connect(handlePayment);
-        middlePanel.sweepUnmixableClicked.connect(handleSweepUnmixable);
-        // basicPanel.paymentClicked.connect(handlePayment);
-        middlePanel.checkPaymentClicked.connect(handleCheckPayment);
-
-        // currentWallet is defined on daemon address change - close/reopen
-        // TODO: strict comparison here (!==) causes crash after passwordDialog on previously crashed unsynced wallets
-        if (currentWallet != undefined) {
-            console.log("closing currentWallet")
-            walletManager.closeWallet(currentWallet);
+        // If currentWallet exists, we're just switching daemon - close/reopen wallet
+        if (typeof currentWallet !== "undefined" && currentWallet !== null) {
+            console.log("Daemon change - closing " + currentWallet)
+            walletManager.closeWalletAsync();
+            currentWallet = undefined
         } else {
 
             // set page to transfer if not changing daemon
@@ -216,7 +183,11 @@ ApplicationWindow {
         currentWallet.moneyReceived.disconnect(onWalletMoneyReceived)
         currentWallet.transactionCreated.disconnect(onTransactionCreated)
         currentWallet.connectionStatusChanged.disconnect(onWalletConnectionStatusChanged)
+        middlePanel.paymentClicked.disconnect(handlePayment);
+        middlePanel.sweepUnmixableClicked.disconnect(handleSweepUnmixable);
+        middlePanel.checkPaymentClicked.disconnect(handleCheckPayment);
 
+        // connect handlers
         currentWallet.refreshed.connect(onWalletRefresh)
         currentWallet.updated.connect(onWalletUpdate)
         currentWallet.newBlock.connect(onWalletNewBlock)
@@ -224,7 +195,9 @@ ApplicationWindow {
         currentWallet.moneyReceived.connect(onWalletMoneyReceived)
         currentWallet.transactionCreated.connect(onTransactionCreated)
         currentWallet.connectionStatusChanged.connect(onWalletConnectionStatusChanged)
-
+        middlePanel.paymentClicked.connect(handlePayment);
+        middlePanel.sweepUnmixableClicked.connect(handleSweepUnmixable);
+        middlePanel.checkPaymentClicked.connect(handleCheckPayment);
 
         console.log("initializing with daemon address: ", persistentSettings.daemon_address)
         console.log("Recovering from seed: ", persistentSettings.is_recovering)
@@ -248,7 +221,9 @@ ApplicationWindow {
             if (appWindow.password === '') {
                 console.error("Error opening wallet with empty password: ", wallet.errorString);
                 console.log("closing wallet async : " + wallet.address)
-                walletManager.closeWalletAsync(wallet)
+                walletManager.closeWalletAsync()
+                currentWallet = undefined
+
                 // try to open wallet with password;
                 passwordDialog.open(wallet.path);
             } else {
@@ -259,7 +234,8 @@ ApplicationWindow {
                 informationPopup.text = qsTr("Couldn't open wallet: ") + wallet.errorString;
                 informationPopup.icon = StandardIcon.Critical
                 console.log("closing wallet async : " + wallet.address)
-                walletManager.closeWalletAsync(wallet);
+                walletManager.closeWalletAsync();
+                currentWallet = undefined
                 informationPopup.open()
                 informationPopup.onCloseCallback = function() {
                     passwordDialog.open(wallet.path)
@@ -619,7 +595,6 @@ ApplicationWindow {
         middlePanel.enabled = enable;
         leftPanel.enabled = enable;
         rightPanel.enabled = enable;
-        // basicPanel.enabled = enable;
     }
 
     function showProcessingSplash(message) {
@@ -640,14 +615,10 @@ ApplicationWindow {
     function showWizard(){
         walletInitialized = false;
         splashCounter = 0;
-        // we can't close async here. Gui crashes if wallet is open
-        if (currentWallet != undefined) {
-            walletManager.closeWallet(currentWallet);
-            currentWallet = undefined
-        }
+        walletManager.closeWalletAsync();
+        currentWallet = undefined
         wizard.restart();
         rootItem.state = "wizard"
-
     }
 
 
@@ -702,8 +673,6 @@ ApplicationWindow {
         property bool   is_recovering : false
     }
 
-    // TODO: replace with customized popups
-
     // Information dialog
     StandardDialog {
         // dynamically change onclose handler
@@ -748,7 +717,6 @@ ApplicationWindow {
         id: passwordDialog
 
         onAccepted: {
-            appWindow.currentWallet = null
             appWindow.initialize();
         }
         onRejected: {
@@ -1036,12 +1004,8 @@ ApplicationWindow {
         }
     }
     onClosing: {
-        // Make sure wallet is closed before app exit (~Wallet() isn't always invoked)
-        // Daemon shutdown is handled with signal/slot in main.cpp
-        if (currentWallet != undefined) {
-            walletManager.closeWallet(currentWallet);
-            currentWallet = undefined
-        }
+        // Close wallet
+        walletManager.closeWallet();
         // Stop daemon
         daemonManager.stop();
     }

src/libwalletqt/WalletManager.cpp

@@ -7,10 +7,11 @@
 #include <QDebug>
 #include <QUrl>
 #include <QtConcurrent/QtConcurrent>
+#include <QMutex>
+#include <QMutexLocker>
 
 WalletManager * WalletManager::m_instance = nullptr;
 
-
 WalletManager *WalletManager::instance()
 {
     if (!m_instance) {
@@ -23,27 +24,37 @@ WalletManager *WalletManager::instance()
 Wallet *WalletManager::createWallet(const QString &path, const QString &password,
                                     const QString &language, bool testnet)
 {
+    QMutexLocker locker(&m_mutex);
+    if (m_currentWallet) {
+        qDebug() << "Closing open m_currentWallet" << m_currentWallet;
+        delete m_currentWallet;
+    }
     Monero::Wallet * w = m_pimpl->createWallet(path.toStdString(), password.toStdString(),
                                                   language.toStdString(), testnet);
-    Wallet * wallet = new Wallet(w);
-    return wallet;
+    m_currentWallet  = new Wallet(w);
+    return m_currentWallet;
 }
 
 Wallet *WalletManager::openWallet(const QString &path, const QString &password, bool testnet)
 {
+    QMutexLocker locker(&m_mutex);
+    if (m_currentWallet) {
+        qDebug() << "Closing open m_currentWallet" << m_currentWallet;
+        delete m_currentWallet;
+    }
     qDebug("%s: opening wallet at %s, testnet = %d ",
            __PRETTY_FUNCTION__, qPrintable(path), testnet);
 
     Monero::Wallet * w =  m_pimpl->openWallet(path.toStdString(), password.toStdString(), testnet);
     qDebug("%s: opened wallet: %s, status: %d", __PRETTY_FUNCTION__, w->address().c_str(), w->status());
-    Wallet * wallet = new Wallet(w);
+    m_currentWallet  = new Wallet(w);
 
     // move wallet to the GUI thread. Otherwise it wont be emitting signals
-    if (wallet->thread() != qApp->thread()) {
-        wallet->moveToThread(qApp->thread());
+    if (m_currentWallet->thread() != qApp->thread()) {
+        m_currentWallet->moveToThread(qApp->thread());
     }
 
-    return wallet;
+    return m_currentWallet;
 }
 
 void WalletManager::openWalletAsync(const QString &path, const QString &password, bool testnet)
@@ -63,23 +74,34 @@ void WalletManager::openWalletAsync(const QString &path, const QString &password
 
 Wallet *WalletManager::recoveryWallet(const QString &path, const QString &memo, bool testnet, quint64 restoreHeight)
 {
+    QMutexLocker locker(&m_mutex);
+    if (m_currentWallet) {
+        qDebug() << "Closing open m_currentWallet" << m_currentWallet;
+        delete m_currentWallet;
+    }
     Monero::Wallet * w = m_pimpl->recoveryWallet(path.toStdString(), memo.toStdString(), testnet, restoreHeight);
-    Wallet * wallet = new Wallet(w);
-    return wallet;
+    m_currentWallet = new Wallet(w);
+    return m_currentWallet;
 }
 
 
-QString WalletManager::closeWallet(Wallet *wallet)
+QString WalletManager::closeWallet()
 {
-    QString result = wallet->address();
-    delete wallet;
+    QMutexLocker locker(&m_mutex);
+    QString result;
+    if (m_currentWallet) {
+        result = m_currentWallet->address();
+        delete m_currentWallet;
+    } else {
+        qCritical() << "Trying to close non existing wallet " << m_currentWallet;
+        result = "0";
+    }
     return result;
 }
 
-void WalletManager::closeWalletAsync(Wallet *wallet)
+void WalletManager::closeWalletAsync()
 {
-    QFuture<QString> future = QtConcurrent::run(this, &WalletManager::closeWallet,
-                                                wallet);
+    QFuture<QString> future = QtConcurrent::run(this, &WalletManager::closeWallet);
     QFutureWatcher<QString> * watcher = new QFutureWatcher<QString>();
     watcher->setFuture(future);
 

src/libwalletqt/WalletManager.h

@@ -4,6 +4,8 @@
 #include <QObject>
 #include <QUrl>
 #include <wallet/wallet2_api.h>
+#include <QMutex>
+#include <QPointer>
 
 class Wallet;
 namespace Monero {
@@ -52,17 +54,15 @@ public:
                                        bool testnet = false, quint64 restoreHeight = 0);
 
     /*!
-     * \brief closeWallet - closes wallet and frees memory
-     * \param wallet
+     * \brief closeWallet - closes current open wallet and frees memory
      * \return wallet address
      */
-    Q_INVOKABLE QString closeWallet(Wallet * wallet);
+    Q_INVOKABLE QString closeWallet();
 
     /*!
      * \brief closeWalletAsync - asynchronous version of "closeWallet"
-     * \param wallet - wallet pointer;
      */
-    Q_INVOKABLE void closeWalletAsync(Wallet * wallet);
+    Q_INVOKABLE void closeWalletAsync();
 
     //! checks is given filename is a wallet;
     Q_INVOKABLE bool walletExists(const QString &path) const;
@@ -125,6 +125,8 @@ private:
     explicit WalletManager(QObject *parent = 0);
     static WalletManager * m_instance;
     Monero::WalletManager * m_pimpl;
+    QMutex m_mutex;
+    QPointer<Wallet> m_currentWallet;
 
 };
 

wizard/WizardCreateWallet.qml

@@ -78,7 +78,7 @@ Item {
 
         // Always delete the wallet object before creating new - we could be stepping back from recovering wallet
         if (typeof settingsObject.wallet !== 'undefined') {
-            settingsObject.wallet.destroy()
+            walletManager.closeWallet()
             console.log("deleting wallet")
         }
 

wizard/WizardRecoveryWallet.qml

@@ -79,7 +79,7 @@ Item {
             settingsObject['wallet'] = wallet;
             settingsObject['is_recovering'] = true;
         } else {
-            walletManager.closeWallet(wallet);
+            walletManager.closeWallet();
         }
         return success;
     }