Qt: avoid AskPassphraseDialog synchronous QDialog.exec() calls

jonasschnelli commented at 8:38 am on February 1, 2019: contributor

If this is an acceptable approach then this shall be adapted (eventually in this PR) to other areas where we use QDialog.exec().

The synchronous QDialog.exec() call interacts badly with multiwallet (especially unloading wallets).

From the Qt docs:

Note: Avoid using this function; instead, use open(). Unlike exec(), open() is asynchronous, and does not spin an additional event loop. This prevents a series of dangerous bugs from happening (e.g. deleting the dialog’s parent while the dialog is open via exec()). When using open() you can connect to the finished() signal of QDialog to be notified when the dialog is closed.

jonasschnelli added the label GUI on Feb 1, 2019

promag commented at 2:30 pm on February 1, 2019: member

Concept ACK.

Is unique_ptr necessary? Could use deleteLater slot instead?

nit 1, AskPassphraseDialog constructor could do setModal(true)? nit 2, AskPassphraseDialog could receive wallet model instance?

flack commented at 5:45 pm on February 1, 2019: contributor

maybe that’s a stupid question, but is there any reason not to make a function which opens the dialog? Because to me it looks like it’s basically three times the same block of code in the same file with only a different first argument in AskPassphraseDialog (ok, and the second connect call in the first code block, but that could also be in an if I guess)

(Disclaimer: Not a C++ expert)

jonasschnelli commented at 11:24 pm on February 1, 2019: contributor

@flack: you are right. As there are many other places to refactor further. The intention of this PR is to fix a bug and not to refactor code and I propose to do refactoring later.

jonasschnelli force-pushed on Feb 1, 2019

jonasschnelli commented at 11:45 pm on February 1, 2019: contributor

Followed @promag advice and changed… … from unique_ptr to usage of QT’s deleteLater which I think is the superior solution (the only advantage of a unique_ptr would be that even if QDialog::finished will never gets fired [bug in QT] it would deallocate the instance correctly on opening of a new dialog or unloading a walletView,…. also, I think the deleteLater approach will confuse static analysers [pretty sure they will report a leak]). … moved the setModal() to the AskPassphraseDialog constructor

The by @promag proposed walletModel change should be done in another PR since its a medium size refactor.

in src/qt/askpassphrasedialog.cpp:79 in d3a73926e7 outdated

73@@ -74,6 +74,9 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
74     connect(ui->passEdit1, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
75     connect(ui->passEdit2, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
76     connect(ui->passEdit3, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
77+
78+    /* always use modal userflow blocking */
79+    setModal(true);

promag commented at 1:31 am on February 5, 2019:

Can also setAttribute(Qt::WA_DeleteOnClose) and ditch connections to deleteLater?

jonasschnelli force-pushed on Feb 7, 2019

jonasschnelli commented at 7:01 am on February 7, 2019: contributor

Thanks for the further optimisation @promag! Implemented your suggestion.

in src/qt/askpassphrasedialog.cpp:82 in 92ee8696e5 outdated

73@@ -74,6 +74,12 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
74     connect(ui->passEdit1, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
75     connect(ui->passEdit2, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
76     connect(ui->passEdit3, &QLineEdit::textChanged, this, &AskPassphraseDialog::textChanged);
77+
78+    // always use modal userflow blocking
79+    setModal(true);
80+
81+    // auto-delete instance after close event
82+    setAttribute(Qt::WA_DeleteOnClose);

laanwj commented at 1:38 pm on February 12, 2019:

I don’t think this should be set in the constructor. It is not the dialog’s responsibility to know about its own object lifetime. So I think it’s more clear to do so at the call site.

promag commented at 3:39 pm on February 15, 2019:

This was suggested by me in #15313 (review) and now I agree with you. @jonasschnelli sorry ☺️

jonasschnelli commented at 7:07 pm on February 17, 2019:

Yes. I agree with @laanwj. Fixed.

laanwj commented at 1:38 pm on February 12, 2019: member

Concept ACK

jonasschnelli force-pushed on Feb 17, 2019

laanwj commented at 7:32 am on February 18, 2019: member

LGTM now utACK https://github.com/bitcoin/bitcoin/pull/15313/commits/5e8dadbcec674a09f6836075bad531395c294399

in src/qt/walletview.cpp:244 in 5e8dadbcec outdated

240@@ -241,11 +241,11 @@ void WalletView::encryptWallet(bool status)
241 {
242     if(!walletModel)
243         return;
244-    AskPassphraseDialog dlg(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Decrypt, this);
245-    dlg.setModel(walletModel);
246-    dlg.exec();
247-
248-    updateEncryptionStatus();
249+    AskPassphraseDialog *dlg = new AskPassphraseDialog(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Decrypt, this);

promag commented at 10:24 pm on March 17, 2019:

nit, here and below: AskPassphraseDialog* dlg.

promag commented at 10:38 pm on March 17, 2019: member

Crashes with the following:

0./src/qt/bitcoin-qt -regtest -wallet=x1 -server

Then open Settings -> Encrypt Wallet..

0./src/bitcoin-cli -regtest unloadwallet x1

Should give something like:

0bitcoin-qt(70767,0x1116265c0) malloc: *** error for object 0x7ffeecb93160: pointer being freed was not allocated
1bitcoin-qt(70767,0x1116265c0) malloc: *** set a breakpoint in malloc_error_break to debug
2[1]    70767 abort      ./src/qt/bitcoin-qt -regtest -wallet=x1 -server

jonasschnelli commented at 8:00 am on March 18, 2019: contributor

As said:

If this is an acceptable approach then this shall be adapted (eventually in this PR) to other areas where we use QDialog.exec().

There are still a couple of places where dialogs are “synchronous” and this needs to be fixed… but I agree with @promag that the one around encryption should be fixes to cover the complete issue. I’ll have a look soon.

jonasschnelli force-pushed on Mar 18, 2019

Qt: avoid AskPassphraseDialog synchronous QDialog.exec() calls b6c360957f

gui: Defer removeAndDeleteWallet when no modal widget is active 79c45b3887

Only delay walletmodel removal when modal dialogs are synchronous 886bd1fde5

jonasschnelli force-pushed on Mar 18, 2019

jonasschnelli added this to the milestone 0.18.0 on Mar 18, 2019

jonasschnelli commented at 10:46 am on March 18, 2019: contributor

Combined this with #15614 to gain a safe unloading-wallet behaviour in the GUI

DrahtBot commented at 1:23 pm on March 18, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15204 (gui: Add Open External Wallet action by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonasschnelli commented at 7:09 pm on March 21, 2019: contributor

Closing for now in favour of #15614

jonasschnelli closed this on Mar 21, 2019

MarcoFalke locked this on Dec 16, 2021

Qt: avoid AskPassphraseDialog synchronous QDialog.exec() calls #15313

Conflicts