Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jonasschnelli wants to merge 3 commits into from
Closed

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

jonasschnelli wants to merge 3 commits into from

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Feb 1, 2019

Fixes #15310

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.

promag
promag reviewed Feb 1, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

flack commented Feb 1, 2019

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
Copy link
Contributor Author

jonasschnelli commented Feb 1, 2019

@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 jonasschnelli force-pushed the 2019/01/qt_exec branch 3 times, most recently from fbb01cb to d3a7392 Compare February 1, 2019 23:40
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Feb 1, 2019

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.

promag
promag reviewed Feb 5, 2019
View reviewed changes
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Feb 7, 2019

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

laanwj
laanwj reviewed Feb 12, 2019
View reviewed changes
@laanwj
Copy link
Member

laanwj commented Feb 12, 2019

Concept ACK

@laanwj
Copy link
Member

laanwj commented Feb 18, 2019

LGTM now utACK 5e8dadb

@ryanofsky ryanofsky mentioned this pull request Feb 20, 2019
Merged
@jonasschnelli jonasschnelli mentioned this pull request Mar 17, 2019
Closed
@promag promag mentioned this pull request Mar 17, 2019
Merged
promag
promag reviewed Mar 17, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crashes with the following:

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

Then open Settings -> Encrypt Wallet..

Screenshot 2019-03-17 at 22 32 37

Confirm and when the following dialog opens:

Screenshot 2019-03-17 at 22 32 55

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

Should give something like:

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

src/qt/walletview.cpp
dlg.exec();

updateEncryptionStatus();
AskPassphraseDialog *dlg = new AskPassphraseDialog(status ? AskPassphraseDialog::Encrypt : AskPassphraseDialog::Decrypt, this);
Copy link
Contributor

@promag promag Mar 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, here and below: AskPassphraseDialog* dlg.

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Mar 18, 2019

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 jonasschnelli added this to the 0.18.0 milestone Mar 18, 2019
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Mar 18, 2019

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 18, 2019

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
Copy link
Contributor Author

jonasschnelli commented Mar 21, 2019

Closing for now in favour of #15614

@promag promag mentioned this pull request Apr 25, 2019
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

gui: crash if encrypt / change passphrase window is open and wallet is unloaded

5 participants

@jonasschnelli @flack @laanwj @DrahtBot @promag