Fixes bitcoin-core/gui#604. Fixes bitcoin/bitcoin#25146. Fixes bitcoin/bitcoin#26340.
SplashScreen::deleteLater() does not guarantee deletion of the m_splash object prior to the wallet context deletion. If the latter happens first, the segfault follows.
Why is this so much more complicated than https://github.com/bitcoin/bitcoin/issues/25146#issuecomment-1129356954 ?
Because this change makes the resulted code simpler.
377@@ -381,6 +378,16 @@ void BitcoinApplication::requestShutdown()
378 void BitcoinApplication::initializeResult(bool success, interfaces::BlockAndHeaderTipInfo tip_info)
379 {
380 qDebug() << __func__ << ": Initialization result: " << success;
381+
382+ if (m_splash) {
383+ // If success, m_splash is no longer needed.
0370fee74b2d665851214f15c0ba2d28e544a40b
according to the comment here, the condition above should be if (success) ?
The if (m_splash) condition actually guards the case when bitcoin-qt is run with -nosplash.
In the comment, I was going to emphasize on the fact the we are going to delete m_splash regardless of success.
But motivation of such deletion depends on success.
I think I have a similar question to luke. With this PR Now there are two different manual deletions for wallet handler in two different places. The BitcoinApplication::requestShutdown() method has:
0 // Delete wallet controller here manually, instead of relying on Qt object
1 // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
2 // walletmodel m_handle_* notification handlers are deleted before wallets
3 // are unloaded, which can simplify wallet implementations. It also avoids
4 // these notifications having to be handled while GUI objects are being
5 // destroyed, making GUI code less fragile as well.
6 delete m_wallet_controller;
7 m_wallet_controller = nullptr;
And the BitcoinApplication::initializeResult method has:
0 // If success, m_splash is no longer needed.
1 // Otherwise, it must be deleted prior to requesting shutdown
2 // to make sure the `SplashScreen::m_connected_wallet_handlers`
3 // is deleted before the wallet context is.
4 delete m_splash;
5 m_splash = nullptr;
Shouldn’t both wallet handler deletions be consolidated together? Or is there something that makes them different from each other?
With this PR Now there are two different manual deletions for wallet handler in two different places.
Correct. But indirectly, via deleting of objects, members of which are wallet handlers.
Shouldn’t both wallet handler deletions be consolidated together? Or is there something that makes them different from each other?
Those wallet handlers belong to very different objects, m_wallet_controller and m_splash, with different roles and life cycles.
OTOH, the suggested approach eliminates duplication of Q_EMIT splashFinished(), removes a signal, a slot, and two connections, and makes code less spaghetti-like.
I agree this the approach in this PR is clean and simplifies code. I think I’m just wondering if it is sufficient to only delete the splash screen object in initializeResult, or if it the splash screen should also be deleted in requestShutdown? Will initializeResult always happen before requestShutdown? Also wondering why requestShutdown is the right place to delete the wallet controller.
The comments do a good job of explaining why it is important to delete these objects, but it’s unclear why the objects are deleted in these places. It’s possible there aren’t concrete reasons, but I’m asking to learn if there are, and maybe improve the comments.
Wouldn’t make sense to unsubscribe from the core signals before the deleteLater call so the object can be freed whenever QT wants without any fear of receiving a post-controller-deletion signal?
In other words, calling SplashScreen::unsubscribeFromCoreSignals() right before the deleteLater() call inside SplashScreen::finish(), or in this latest changes, change the delete for a deleteLater and call the unsubscribeFromCoreSignals just before it.
I agree this the approach in this PR is clean and simplifies code. I think I’m just wondering if it is sufficient to only delete the splash screen object in initializeResult, or if it the splash screen should also be deleted in requestShutdown? Will initializeResult always happen before requestShutdown? Also wondering why requestShutdown is the right place to delete the wallet controller.
The comments do a good job of explaining why it is important to delete these objects, but it’s unclear why the objects are deleted in these places. It’s possible there aren’t concrete reasons, but I’m asking to learn if there are, and maybe improve the comments.
The universal entry point of the shutdown process is BitcoinApplication::requestShutdown() which handles both internal and OS-initiated cases, and it always runs in the “main” GUI thread. That is why it is the only right place to delete the wallet controller (considering the current implementation of wallet handlers).
OTOH, BitcoinApplication::initializeResult() is called via Qt::QueuedConnection from the “qt-init”/“shutoff” thread. Therefore, there are no guarantees that it always happens before BitcoinApplication::requestShutdown() (for example, in case when the latter is triggered by OS’s QEvent::Quit).
re: #605 (comment)
OTOH, BitcoinApplication::initializeResult() is called via Qt::QueuedConnection from the “qt-init”/“shutoff” thread. Therefore, there are no guarantees that it always happens before BitcoinApplication::requestShutdown() (for example, in case when the latter is triggered by OS’s QEvent::Quit).
So if QEvent::Quit happens, and BitcoinApplication::requestShutdown() runs before BitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?
If so, it seems like maybe we need to add delete m_splash; m_splash = nullptr; lines to both of the requestShutdown() and initializeResult() methods, not just one of the methods. Or is this not the case?
re: #605 (comment)
Wouldn’t make sense to unsubscribe from the core signals before the
deleteLatercall so the object can be freed whenever QT wants without any fear of receiving a post-controller-deletion signal?
Unclear if it would make things more complicated to partially shut down wallet controller and splash screen objects by disconnecting them from signals, and let Qt delete them later, than to simply delete these objects ourselves when we know they are no longer needed. But maybe it would be more targeted and not actually more complicated. Qt does seem to not really care whether we delete the objects or it will. Both approaches seem like they would be ok.
So if
QEvent::Quithappens, andBitcoinApplication::requestShutdown()runs beforeBitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?
Correct, although this scenario is unlikely. Nevertheless, the code is flawed.
If so, it seems like maybe we need to add
delete m_splash; m_splash = nullptr;lines to both of therequestShutdown()andinitializeResult()methods, not just one of the methods. Or is this not the case?
Would it cleaner to introduce a dedicated SplashScreen::releaseWalletHandler() member function, and leave m_splash object lifetime management to Qt framework? The same approach could be used for wallet controller.
So if
QEvent::Quithappens, andBitcoinApplication::requestShutdown()runs beforeBitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?
Added a commit to prevent such a scenario.
460@@ -460,7 +461,7 @@ WId BitcoinApplication::getMainWinId() const
461
462 bool BitcoinApplication::event(QEvent* e)
463 {
464- if (e->type() == QEvent::Quit) {
465+ if (m_initialized && e->type() == QEvent::Quit) {
In commit “qt: Ignore QEvent::Quit until full initialization” (d4227f7449d080f1a30e97847ef8d73d18bf872d)
This seems like a pretty indirect way of preventing requestShutdown() from running before initializeResult() so splashscreen will be deleted before the wallet and won’t hold dangling references to the wallet.
It also seems pretty fragile, because this is just one requestShutdown() caller when there could be other callers now or in the future that fail to delete the splash screen before calling requestShutdown.
Also the commit seems to make code more complicated. There are no comments to explain the logic, and the commit message just has a vague message about requestShutdown being error prune, not saying why it is (or should be) error prone.
I think a better approach here would just to make the requestShutdown method delete splash screen and wallet at the same time, so requestShutdown calls are not error-prone and don’t need to be avoided like this. Something like
0delete m_splash;
1m_splash = nullptr;
2delete m_wallet_controller;
3m_wallet_controller = nullptr;
in requestShutdown like achow originally proposed https://github.com/bitcoin/bitcoin/issues/25146#issuecomment-1129356954 would make sense I think.
Additionally, it does seem good to delete the splash screen in initializeResult as you are doing in your first commit “qt: Delete splash screen widget early” (c7d24585080b3c22a56dfae600d0570b12a0537a), since that also makes sense and simplifiies code. In both cases I think it’s good to delete the splash screen as soon as the splash screen is no longer needed.
Code review ACK d4227f7449d080f1a30e97847ef8d73d18bf872d, but I tend to think it would be good to keep the first three commits, drop the fourth commit “Ignore QEvent::Quit until full initialization”, and add a new commit with achow’s suggestion also deleting the splash screen in requestShutdown() so requestShutdown() is less fragile. (See comments below)
Would it cleaner to introduce a dedicated
SplashScreen::releaseWalletHandler()member function, and leavem_splashobject lifetime management to Qt framework? The same approach could be used for wallet controller.
IMO it is cleanest just to delete the splash screen as soon as you don’t need the splash screen, rather than keep the splash screen alive but remove the wallet handler underneath it. If it were actually helpful to the user to keep showing the splash screen while unloading, maybe a more complicated approach like this could be good, but otherwise it seems like it wouldn’t improve things.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See 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.
No conflicts as of last run.
Updated d4227f7449d080f1a30e97847ef8d73d18bf872d -> d2cfc7d6afad634d1debf7cdf5488f4953a975d9 (pr605.03 -> pr605.04):
drop the fourth commit “Ignore QEvent::Quit until full initialization”, and add a new commit with achow’s suggestion also deleting the splash screen in requestShutdown() so requestShutdown() is less fragile
Done.
tACK d2cfc7d6afad634d1debf7cdf5488f4953a975d9
I have reviewed the code and I agree it can be merged. Additionally did testing where I quit in various scenarios where the splash screen was still loading and quiting afterwards as well.
In relation to https://github.com/bitcoin/bitcoin/issues/26340, I still couldn’t recreate the issue. But since the splash screen would be the only core window showing at this time, it’s reasonable to think it could be related.
396 // these notifications having to be handled while GUI objects are being
397 // destroyed, making GUI code less fragile as well.
398 delete m_wallet_controller;
399 m_wallet_controller = nullptr;
400+ delete m_splash;
401+ m_splash = nullptr;
#ifdef ENABLE_WALLET scope.
As GUI uses the splash screen even when it runs without a wallet (--disable-wallet), the object will not be deleted if a shutdown is requested during load.
the object will not be deleted if a shutdown is requested during load.
~It will, via Qt parent-child relation.~
Early widget deletion is crucial when wallet code is active only.
the object will not be deleted if a shutdown is requested during load.
It will, via Qt parent-child relation.
The SplashScreen has no parent. It’s a standalone dialog (check the view constructor).
You can try it by adding a print inside the splash screen destructor and call requestShutdown at any time during load (blocking initializeResult execution).
This ensures that during shutdown, including failed initialization, the
`SplashScreen::m_connected_wallet_handlers` is deleted before the wallet
context is.
This fix causes a crash elsewhere: https://github.com/bitcoin/bitcoin/issues/26340#issuecomment-1290740422
ACK https://github.com/bitcoin-core/gui/commit/1b228497fa729c512a15bdfa80f61a610abfe8a5
