Delete splash screen widget early

[hebasto]

Fixes bitcoin-core/gui#604. Fixes bitcoin/bitcoin#25146.

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.

[luke-jr]

Why is this so much more complicated than https://github.com/bitcoin/bitcoin/issues/25146#issuecomment-1129356954 ?

[hebasto]

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.

[hebasto]

Updated 66646b6f4d63a6bad09ac6bca8247a2549f002d0 -> ec8d6aacd97b8a7d98de3943c626cf49fe4f1ac8 (pr605.01 -> pr605.02, diff):

• dropped unneeded condition

[ryanofsky]

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:

    // Delete wallet controller here manually, instead of relying on Qt object
    // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
    // walletmodel m_handle_* notification handlers are deleted before wallets
    // are unloaded, which can simplify wallet implementations. It also avoids
    // these notifications having to be handled while GUI objects are being
    // destroyed, making GUI code less fragile as well.
    delete m_wallet_controller;
    m_wallet_controller = nullptr;

And the BitcoinApplication::initializeResult method has:

    // If success, m_splash is no longer needed.
    // Otherwise, it must be deleted prior to requesting shutdown
    // to make sure the `SplashScreen::m_connected_wallet_handlers`
    // is deleted before the wallet context is.
    delete m_splash;
    m_splash = nullptr;

Shouldn't both wallet handler deletions be consolidated together? Or is there something that makes them different from each other?

[hebasto]

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.

[ryanofsky]

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.

[furszy]

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.

[hebasto]

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

[ryanofsky]

re: https://github.com/bitcoin-core/gui/pull/605#issuecomment-1137436764

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: https://github.com/bitcoin-core/gui/pull/605#issuecomment-1136396327

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?

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.

[hebasto]

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?

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 the requestShutdown() and initializeResult() 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.

[hebasto]

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?

Added a commit to prevent such a scenario.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, dooglus, john-moffett
Stale ACK	ryanofsky, achow101, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

[hebasto]

Updated d4227f7449d080f1a30e97847ef8d73d18bf872d -> d2cfc7d6afad634d1debf7cdf5488f4953a975d9 (pr605.03 -> pr605.04):

• rebased
• addressed @ryanofsky's comments

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.

cc @achow101 @dooglus @furszy @luke-jr @promag @ryanofsky

[achow101]

Code Review ACK d2cfc7d6afad634d1debf7cdf5488f4953a975d9

[dooglus]

~~This fix causes a crash elsewhere: https://github.com/bitcoin/bitcoin/issues/26340#issuecomment-1290740422~~

ACK https://github.com/bitcoin-core/gui/commit/1b228497fa729c512a15bdfa80f61a610abfe8a5