gui: Defer removeAndDeleteWallet when no modal widget is active

promag commented at 6:35 pm on March 17, 2019: member

Fixes #15310.

promag commented at 6:44 pm on March 17, 2019: member

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

fanquake added the label GUI on Mar 17, 2019

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

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 added this to the milestone 0.18.0 on Mar 18, 2019

jonasschnelli commented at 9:01 am on March 18, 2019: contributor

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 commented at 9:27 am on March 18, 2019: member

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.

promag commented at 7:13 pm on March 21, 2019: member

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

MarcoFalke changed the base branch on Mar 21, 2019

MarcoFalke renamed this:
~~0.18: gui: Defer removeAndDeleteWallet when no modal widget is active~~
gui: Defer removeAndDeleteWallet when no modal widget is active
on Mar 21, 2019

MarcoFalke commented at 7:28 pm on March 21, 2019: member

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

promag force-pushed on Mar 21, 2019

promag commented at 7:46 pm on March 21, 2019: member

Rebased.

MarcoFalke commented at 8:01 pm on March 21, 2019: member

Should update OP with something like “Addresses #15310”?

gmaxwell commented at 8:07 pm on March 21, 2019: contributor

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

promag commented at 8:08 pm on March 21, 2019: member

@MarcoFalke yes, after rebased with master.

MarcoFalke commented at 8:14 pm on March 21, 2019: member

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 commented at 9:37 pm on March 21, 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:

#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.

in src/qt/walletcontroller.cpp:103 in f33e9bcda8 outdated

 97@@ -97,7 +98,16 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
 98     m_wallets.push_back(wallet_model);
 99 
100     connect(wallet_model, &WalletModel::unload, [this, wallet_model] {
101-        removeAndDeleteWallet(wallet_model);
102+        // Defer removeAndDeleteWallet when no modal widget is active.
103+        if (QApplication::activeModalWidget()) {
104+            connect(qApp, &QApplication::focusChanged, wallet_model, [this, wallet_model]() {

promag commented at 10:12 pm on March 21, 2019:

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 approved

luke-jr commented at 10:35 pm on March 21, 2019: member

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

promag commented at 0:16 am on March 22, 2019: member

@luke-jr care to elaborate?

gui: Defer removeAndDeleteWallet when no modal widget is active a10972bc03

promag force-pushed on Mar 22, 2019

jonasschnelli commented at 10:06 am on March 22, 2019: contributor

Tested ACK a10972bc03e1ebbe79c3edc929c91ed53b391c9a

jonasschnelli merged this on Mar 22, 2019

jonasschnelli closed this on Mar 22, 2019

jonasschnelli referenced this in commit abd914ed34 on Mar 22, 2019

laanwj referenced this in commit b022242887 on Mar 22, 2019

luke-jr commented at 3:57 pm on March 22, 2019: member

@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 commented at 4:15 pm on March 22, 2019: member

@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 commented at 9:56 pm on March 22, 2019: member

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

luke-jr commented at 9:57 pm on March 22, 2019: member

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

PastaPastaPasta referenced this in commit 7017bc4620 on Sep 21, 2021

PastaPastaPasta referenced this in commit 167363d4bd on Sep 24, 2021

kittywhiskers referenced this in commit 4094e3313e on Oct 12, 2021

DrahtBot locked this on Dec 16, 2021

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

Conflicts