From ead9bd70be47be3403e296ae14ea1b03bfa80bf7 Mon Sep 17 00:00:00 2001 From: Jaquee Date: Tue, 13 Dec 2016 23:44:44 +0100 Subject: [PATCH 1/3] WalletManager: lock wallet while closing WalletManager: Make sure only one wallet is open --- src/libwalletqt/WalletManager.cpp | 52 ++++++++++++++++++++++--------- src/libwalletqt/WalletManager.h | 12 ++++--- 2 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/libwalletqt/WalletManager.cpp b/src/libwalletqt/WalletManager.cpp index 37f04f71..0ff8d74d 100644 --- a/src/libwalletqt/WalletManager.cpp +++ b/src/libwalletqt/WalletManager.cpp @@ -7,10 +7,11 @@ #include #include #include +#include +#include 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 future = QtConcurrent::run(this, &WalletManager::closeWallet, - wallet); + QFuture future = QtConcurrent::run(this, &WalletManager::closeWallet); QFutureWatcher * watcher = new QFutureWatcher(); watcher->setFuture(future); diff --git a/src/libwalletqt/WalletManager.h b/src/libwalletqt/WalletManager.h index 7dcff8fa..88425f6f 100644 --- a/src/libwalletqt/WalletManager.h +++ b/src/libwalletqt/WalletManager.h @@ -4,6 +4,8 @@ #include #include #include +#include +#include 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 m_currentWallet; }; From eb97bda3a369c03d092ce92dcc0b3e28fdd57a8a Mon Sep 17 00:00:00 2001 From: Jaquee Date: Sun, 4 Dec 2016 13:13:59 +0100 Subject: [PATCH 2/3] code clean up and wallet close consistency --- main.qml | 77 ++++++++------------------------- wizard/WizardCreateWallet.qml | 2 +- wizard/WizardRecoveryWallet.qml | 2 +- 3 files changed, 20 insertions(+), 61 deletions(-) diff --git a/main.qml b/main.qml index a601596c..9cb918e9 100644 --- a/main.qml +++ b/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,10 @@ 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(); } else { // set page to transfer if not changing daemon @@ -216,7 +182,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 +194,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 +220,7 @@ 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() // try to open wallet with password; passwordDialog.open(wallet.path); } else { @@ -259,7 +231,7 @@ 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(); informationPopup.open() informationPopup.onCloseCallback = function() { passwordDialog.open(wallet.path) @@ -619,7 +591,6 @@ ApplicationWindow { middlePanel.enabled = enable; leftPanel.enabled = enable; rightPanel.enabled = enable; - // basicPanel.enabled = enable; } function showProcessingSplash(message) { @@ -640,14 +611,9 @@ 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(); wizard.restart(); rootItem.state = "wizard" - } @@ -702,8 +668,6 @@ ApplicationWindow { property bool is_recovering : false } - // TODO: replace with customized popups - // Information dialog StandardDialog { // dynamically change onclose handler @@ -748,7 +712,6 @@ ApplicationWindow { id: passwordDialog onAccepted: { - appWindow.currentWallet = null appWindow.initialize(); } onRejected: { @@ -1036,12 +999,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(); } diff --git a/wizard/WizardCreateWallet.qml b/wizard/WizardCreateWallet.qml index 81ee9ba7..cf26c7bc 100644 --- a/wizard/WizardCreateWallet.qml +++ b/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") } diff --git a/wizard/WizardRecoveryWallet.qml b/wizard/WizardRecoveryWallet.qml index c5ff8bc9..2c75225e 100644 --- a/wizard/WizardRecoveryWallet.qml +++ b/wizard/WizardRecoveryWallet.qml @@ -79,7 +79,7 @@ Item { settingsObject['wallet'] = wallet; settingsObject['is_recovering'] = true; } else { - walletManager.closeWallet(wallet); + walletManager.closeWallet(); } return success; } From 7fd80a1be2bf91026d12b023c1ea9f5d58246b62 Mon Sep 17 00:00:00 2001 From: Jaquee Date: Wed, 14 Dec 2016 13:11:20 +0100 Subject: [PATCH 3/3] async close: make sure curretWallet isn't used after shutdown started --- main.qml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/main.qml b/main.qml index 9cb918e9..44fd3f3e 100644 --- a/main.qml +++ b/main.qml @@ -135,6 +135,7 @@ ApplicationWindow { 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 @@ -221,6 +222,8 @@ ApplicationWindow { console.error("Error opening wallet with empty password: ", wallet.errorString); console.log("closing wallet async : " + wallet.address) walletManager.closeWalletAsync() + currentWallet = undefined + // try to open wallet with password; passwordDialog.open(wallet.path); } else { @@ -232,6 +235,7 @@ ApplicationWindow { informationPopup.icon = StandardIcon.Critical console.log("closing wallet async : " + wallet.address) walletManager.closeWalletAsync(); + currentWallet = undefined informationPopup.open() informationPopup.onCloseCallback = function() { passwordDialog.open(wallet.path) @@ -612,6 +616,7 @@ ApplicationWindow { walletInitialized = false; splashCounter = 0; walletManager.closeWalletAsync(); + currentWallet = undefined wizard.restart(); rootItem.state = "wizard" }