Skip to content

gui: Defer removeAndDeleteWallet when no modal widget is active#15614

Merged
jonasschnelli merged 1 commit intobitcoin:masterfrom
promag:2019-03-wallet-modal-widget
Mar 22, 2019
Merged

gui: Defer removeAndDeleteWallet when no modal widget is active#15614
jonasschnelli merged 1 commit intobitcoin:masterfrom
promag:2019-03-wallet-modal-widget

Conversation

@promag
Copy link
Contributor

@promag promag commented Mar 17, 2019 •
edited by fanquake
Loading

Fixes #15310.

@promag
Copy link
Contributor Author

promag commented Mar 17, 2019 •
edited
Loading

Alternative to #15313 for 0.18, more details in http://www.erisian.com.au/bitcoin-core-dev/log-2019-03-16.html (line 279) cc @gmaxwell @jonasschnelli

@promag promag mentioned this pull request Mar 17, 2019
Closed
@fanquake fanquake added the GUI label Mar 17, 2019
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Mar 18, 2019

Concept ACK for 0.18.

Looks like a hack (but a good one)... I think we should merge this into 0.18 and properly remove synchronous dialogs in 0.19.

@jonasschnelli jonasschnelli added this to the 0.18.0 milestone Mar 18, 2019
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Mar 18, 2019

Hmm... second thought: I'm unsure about this. This waits until the users closes the dialogs (rather then closing them explicit). This may lead to an RPC timeout in unload wallet...

Maybe we combine this with #15313?

@promag
Copy link
Contributor Author

promag commented Mar 18, 2019

I don't think that's a problem. The RPC client can timeout (can also happen for other reasons, rescanning for instance?) but the RPC handler still waits. This fixes a unlikely crash (I mean, is there a good reason to do this?) without major changes and can be reverted easily if the future master refactor is backport.

@jonasschnelli jonasschnelli mentioned this pull request Mar 18, 2019
Closed
@promag
Copy link
Contributor Author

promag commented Mar 21, 2019

I'll verify if QWindow::activeChanged can be used instead of QApplication::focusChanged.

@maflcko maflcko changed the base branch from 0.18 to master March 21, 2019 19:27
@maflcko maflcko changed the title 0.18: gui: Defer removeAndDeleteWallet when no modal widget is active gui: Defer removeAndDeleteWallet when no modal widget is active Mar 21, 2019
@maflcko
Copy link
Member

maflcko commented Mar 21, 2019

Changed base, needs commit cherry-picked on some other base as well, obviously.

@promag promag force-pushed the 2019-03-wallet-modal-widget branch from ae59821 to f33e9bc Compare March 21, 2019 19:45
@promag
Copy link
Contributor Author

promag commented Mar 21, 2019

Rebased.

@maflcko
Copy link
Member

maflcko commented Mar 21, 2019 •
edited
Loading

Should update OP with something like "Addresses #15310"?

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 21, 2019

This seems like a better workaround to the issue for the short term.

@promag
Copy link
Contributor Author

promag commented Mar 21, 2019

@MarcoFalke yes, after rebased with master.

@maflcko
Copy link
Member

maflcko commented Mar 21, 2019

tested ACK f33e9bcda8a34385872555c1857bbabdf5c68134 (No longer crashes and having the modal gui dialog open will block the unloadwallet RPC, didn't review code too closely)

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 21, 2019 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15478 (wip: gui: Refactor open wallet activity by promag)
  • #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.

promag
promag commented Mar 21, 2019
View reviewed changes
src/qt/walletcontroller.cpp Outdated
Copy link
Contributor Author

@promag promag Mar 21, 2019

Choose a reason for hiding this comment

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

Just noting that the 3rd parameter wallet_model is used as the destination context, which means:

  • the connection is destroyed when the wallet_model is destroyed
  • the connection runs on the wallet model thread (GUI thread) due to Qt::QueuedConnection.

luke-jr
luke-jr approved these changes Mar 21, 2019
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

I suspect there's some race conditions here, but strict improvement over the current condition, so utACK

@promag
Copy link
Contributor Author

promag commented Mar 22, 2019

@luke-jr care to elaborate?

@promag promag force-pushed the 2019-03-wallet-modal-widget branch from 030a0f0 to a10972b Compare March 22, 2019 09:51
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Mar 22, 2019

Tested ACK a10972b

@jonasschnelli jonasschnelli merged commit a10972b into bitcoin:master Mar 22, 2019
jonasschnelli added a commit that referenced this pull request Mar 22, 2019
@jonasschnelli
Merge #15614: gui: Defer removeAndDeleteWallet when no modal widget i…
abd914e 
…s active

a10972b gui: Defer removeAndDeleteWallet when no modal widget is active (João Barbosa)

Pull request description:

  Fixes #15310.

Tree-SHA512: ac91a4e37020d3a854830c50c0a7a45c2c0537f80be492ec5e9ba7daf90725e912f9dcc324605493599c36180e1d3bcdfa86840b7325cba208b7e93fbe7be368
@jonasschnelli jonasschnelli mentioned this pull request Mar 22, 2019
Merged
laanwj added a commit that referenced this pull request Mar 22, 2019
@laanwj
Merge #15641: Backport #15614 to 0.18: gui: Defer removeAndDeleteWall…
b022242 
…et when no modal widget is active

98a24a2 gui: Defer removeAndDeleteWallet when no modal widget is active (João Barbosa)

Pull request description:

  0.18 Backport of #15614

Tree-SHA512: 8f785705325364ecfa37045090f10ca615f457e279789b0ce0d61f2667f491bce9b44f5e5cdeeecf63d61356213d9a97a3578841295cc8480cf42e037c2decb2
@luke-jr
Copy link
Member

luke-jr commented Mar 22, 2019

@promag

line 104 evaluates to false, then a problematic modal window is opened before line 111

or

line 104 evaluates to true, then the problematic modal window is closed before focusWindowChanged is connected; wallet closing then waits for some arbitrarily future time

@promag
Copy link
Contributor Author

promag commented Mar 22, 2019

@luke-jr it's impossible, both callbacks runs in the GUI thread and so:

  • first case, no modal can be opened;
  • second case, the modal can't be closed before connecting the signal.

@luke-jr
Copy link
Member

luke-jr commented Mar 22, 2019

I thought the whole point of the Qt::QueuedConnection was to move the callback to the GUI thread later?

@luke-jr
Copy link
Member

luke-jr commented Mar 22, 2019

Ah, nevermind, I get it - the focusWindowChanged might be in another thread.

PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 21, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#15614: gui: Defer removeAndDeleteWallet when no modal w…
7017bc4 
…idget is active

a10972b gui: Defer removeAndDeleteWallet when no modal widget is active (João Barbosa)

Pull request description:

  Fixes bitcoin#15310.

Tree-SHA512: ac91a4e37020d3a854830c50c0a7a45c2c0537f80be492ec5e9ba7daf90725e912f9dcc324605493599c36180e1d3bcdfa86840b7325cba208b7e93fbe7be368
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#15614: gui: Defer removeAndDeleteWallet when no modal w…
167363d 
…idget is active

a10972b gui: Defer removeAndDeleteWallet when no modal widget is active (João Barbosa)

Pull request description:

  Fixes bitcoin#15310.

Tree-SHA512: ac91a4e37020d3a854830c50c0a7a45c2c0537f80be492ec5e9ba7daf90725e912f9dcc324605493599c36180e1d3bcdfa86840b7325cba208b7e93fbe7be368
@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

1 more reviewer

@luke-jr luke-jr luke-jr approved these changes

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

7 participants

@promag @jonasschnelli @maflcko @gmaxwell @DrahtBot @luke-jr @fanquake