Fix wallet list hover crash on shutdown #765
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
hebasto
changed the title
Wouldn't be enough to use an instead of The former assumes that:
Also see a discussion in #680. |
furszy
force-pushed
the
2023_gui_fix_crash_wallet_list
branch
from
030cbec
to
09c0c71
Compare
Addressing potential crashes during shutdown. The most noticeable one can be triggered by hovering over the wallet list as the app shuts down.
Opening the top bar menu when the app is being destroyed freezes the GUI shutdown process for no reason. No menu action can be executed. Note: This behavior is consistent with how the tray icon menu is cleared too.
furszy
force-pushed
the
2023_gui_fix_crash_wallet_list
branch
from
09c0c71
to
8b6470a
Compare
There was a problem hiding this comment.
Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers.
Wouldn't be enough to use an
QObject::connectoverload with the additionalcontextparameter, i.e.QObject::connect(const QObject* sender, PointerToMemberFunction signal, const QObject* context, Functor functor, Qt::ConnectionType type)instead of
QObject::connect(const QObject* sender, PointerToMemberFunction signal, Functor functor)The former assumes that:
The connection will automatically disconnect if the sender or the context is destroyed. However, you should take care that any objects used within the functor are still alive when the signal is emitted.
Also see a discussion in #680.
Tackled. Thanks.
I was a bit doubtful initially because we are manually deleting the wallet_controller field instead of letting QT do it for us.. but, thanks to the QObject::destroyed signal, this works fine.
Other than that, while testing the changes, I saw that the GUI shutdown process was being frozen when the user hovers over the non-triggerable top bar menu actions. So, the second commit also fixes this by clearing the menu (same as we do with the tray icon menu).
|
Unfortunately, it introduced a regression. See #770. |
…actions" f09bfab Revert "gui: provide wallet controller context to wallet actions" (Hennadii Stepanov) Pull request description: The commit 7066e89 from #765 breaks "Open Wallet", "Close Wallet" and "Close All Wallets" items in the File menu (at least on Ubuntu 23.10 + Wayland). Reverting it to avoid this regression in the 26.0 release. ACKs for top commit: furszy: ACK f09bfab for including it in v26. jarolrod: ACK f09bfab Tree-SHA512: fedc621c8e9bf84a263b0c28da53225febe0267d0123830a6192297f38e40726e1613e003b634215e7d16791ba6eab52fb4baab3da9637f6660b6ae1ae98462b
Small follow-up to #751.
Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
Future Note:
This surely happen in other places as well, we should re-work the way we connect signals. Register
lambas without any precaution can leave dangling pointers.