Unregister wallet notifications before unloading wallets by ryanofsky · Pull Request #360 · bitcoin-core/gui

ryanofsky · 2021-06-10T17:34:16Z

This change was originally part of both bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 and is required for both because it avoids the IPC wallet implementation in bitcoin/bitcoin#10102 and the WalletContext implementation in bitcoin/bitcoin#19101 needing to deal with notification objects that have stale pointers to deleted wallets.

promag

Code review ACK 93cc53a.

hebasto · 2021-08-05T13:00:40Z

src/qt/bitcoin.cpp

+    // 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


Delete wallet controller here manually... This makes sure walletmodel m_handle_* notification handlers are deleted before wallets are unloaded, which can simplify wallet implementations.

Sorry, but I cannot get it.
To achieve the goal--notification handler deletion--the ~WalletModel destructor must be called. But I cannot see the way how deletion of the m_wallet_controller could works here, as an instance of the WalletController class does not own WalletModel objects, because WalletController::m_wallets is std::vector<WalletModel*>, not std::vector<std::unique_ptr<WalletModel>>.

Did I miss the point?

re: #360 (comment)

To achieve the goal--notification handler deletion--the ~WalletModel destructor must be called. But I cannot see the way how deletion of the m_wallet_controller could works here, as an instance of the WalletController class does not own WalletModel objects, because WalletController::m_wallets is std::vector<WalletModel*>, not std::vector<std::unique_ptr<WalletModel>>.

Did I miss the point?

No, this is a reasonable question because wallet controller's ownership of wallet models is implicit instead of explicit. But m_wallet vector usage is just emulating unique_ptr without using unique_ptr for some reason (probably no reason).

In the case where an m_wallet entry is erased, the pointer is deleted:

gui/src/qt/walletcontroller.cpp

Lines 179 to 186 in d67330d

{

QMutexLocker locker(&m_mutex);

m_wallets.erase(std::remove(m_wallets.begin(), m_wallets.end(), wallet_model));

}

Q_EMIT walletRemoved(wallet_model);

// Currently this can trigger the unload since the model can hold the last

// CWallet shared pointer.

delete wallet_model;

In the case where the m_wallet vector is cleared, the pointers are deleted:

gui/src/qt/walletcontroller.cpp

Lines 108 to 114 in d67330d

QMutexLocker locker(&m_mutex);

for (WalletModel* wallet_model : m_wallets) {

wallet_model->wallet().remove();

Q_EMIT walletRemoved(wallet_model);

delete wallet_model;

}

m_wallets.clear();

And in the case where the controller object is destroyed, any models that weren't cleared or erased are destroyed:

gui/src/qt/walletcontroller.cpp

Lines 142 to 146 in d67330d

GUIUtil::ObjectInvoke(this, [wallet_model, this] {

wallet_model->setParent(this);

}, GUIUtil::blockingGUIThreadConnection());

m_wallets.push_back(wallet_model);

I could add more details to this comment, but I don't think it would be appropriate to mention implementation details of the wallet controller outside the wallet controller (especially when implementation seems unnecessarily complicated and could probably be simplified later). So hopefully this reply clears things up.

Thanks for looking into this!

hebasto

Approach ACK 93cc53a.

The forced destroying of m_wallet_controller indeed achieves the desired goals.

But I'm still uncomfortable with the suggested comment, because we still rely on Qt object ownership.

Setting a ~WalletModel as a breakpoint clearly proves the statement above:

$ lldb ./src/qt/bitcoin-qt -- -signet
(lldb) target create "./src/qt/bitcoin-qt"
Current executable set to '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64).
(lldb) settings set -- target.run-args  "-signet"
(lldb) br s -n ~WalletModel
Breakpoint 1: 2 locations.
(lldb) run
Process 116788 launched: '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64)
Process 116788 stopped
* thread #1, name = 'bitcoin-qt', stop reason = breakpoint 1.2
    frame #0: 0x000055555565c07a bitcoin-qt`WalletModel::~WalletModel(this=0x00005555571b4aa0) at walletmodel.cpp:61:1
(lldb) bt
* thread #1, name = 'bitcoin-qt', stop reason = breakpoint 1.2
  * frame #0: 0x000055555565c07a bitcoin-qt`WalletModel::~WalletModel(this=0x00005555571b4aa0) at walletmodel.cpp:61:1
    frame #1: 0x00007ffff7a6701e libQt5Core.so.5`QObjectPrivate::deleteChildren() + 110
    frame #2: 0x00007ffff7a715ef libQt5Core.so.5`QObject::~QObject() + 1455
    frame #3: 0x0000555555655157 bitcoin-qt`WalletController::~WalletController(this=0x0000555557358d10) at walletcontroller.cpp:61:1
    frame #4: 0x00005555556551cf bitcoin-qt`WalletController::~WalletController(this=0x0000555557358d10) at walletcontroller.cpp:57:1
    frame #5: 0x00005555555d9c63 bitcoin-qt`BitcoinApplication::requestShutdown(this=0x00007fffffffd730) at bitcoin.cpp:362:5
    frame #6: 0x00005555555dc269 bitcoin-qt`GuiMain(argc=2, argv=<unavailable>) at bitcoin.cpp:650:17
    frame #7: 0x00005555555d70ea bitcoin-qt`main(argc=<unavailable>, argv=<unavailable>) at main.cpp:21:43
    frame #8: 0x00007ffff61fa0b3 libc.so.6`__libc_start_main + 243
    frame #9: 0x00005555555d700e bitcoin-qt`_start + 46

A WalletModel object becomes a child of the WalletController object here:

gui/src/qt/walletcontroller.cpp

Line 143 in 03826ae

wallet_model->setParent(this);

ryanofsky · 2021-08-12T16:54:31Z

But I'm still uncomfortable with the suggested comment, because we still rely on Qt object ownership.

Hmm, maybe you have a suggestion to reword the comment. The comment "Delete wallet controller here manually, instead of relying on Qt object tracking" is not saying we no longer rely on Qt object tracking generally. It is just saying we no longer rely on it to delete the wallet controller object.

The wallet model is a different object from the wallet controller. It would be nice to manage wallet model objects with smart pointer ownership, but not necessary. This PR is necessary and is blocking bitcoin/bitcoin#10102 and bitcoin/bitcoin#19101 because these PR's can't deal with the GUI having stale walletmodel m_handle_* notification handlers associated with unloaded wallets.

hebasto · 2021-08-12T16:58:50Z

@ryanofsky

The comment "Delete wallet controller here manually, instead of relying on Qt object tracking" is not saying we no longer rely on Qt object tracking generally. It is just saying we no longer rely on it to delete the wallet controller object.

You are right.

hebasto

ACK 93cc53a

hebasto added the Wallet label Jun 10, 2021

hebasto changed the title ~~gui: Unregister wallet notifications before unloading wallets~~ Unregister wallet notifications before unloading wallets Jun 10, 2021

ryanofsky mentioned this pull request Jun 10, 2021

refactor: remove ::vpwallets and related global variables bitcoin/bitcoin#19101

Merged

promag reviewed Jun 11, 2021

View reviewed changes

ryanofsky mentioned this pull request Jun 15, 2021

Multiprocess bitcoin bitcoin/bitcoin#10102

Draft

hebasto reviewed Aug 5, 2021

View reviewed changes

hebasto reviewed Aug 7, 2021

View reviewed changes

hebasto approved these changes Aug 12, 2021

View reviewed changes

hebasto merged commit 439e58c into bitcoin-core:master Aug 12, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unregister wallet notifications before unloading wallets#360

Unregister wallet notifications before unloading wallets#360
hebasto merged 1 commit intobitcoin-core:masterfrom
ryanofsky:pr/qtwd

ryanofsky commented Jun 10, 2021

Uh oh!

promag left a comment •

edited

Loading

Uh oh!

hebasto Aug 5, 2021

Uh oh!

ryanofsky Aug 5, 2021 •

edited

Loading

Uh oh!

hebasto left a comment

Uh oh!

ryanofsky commented Aug 12, 2021

Uh oh!

hebasto commented Aug 12, 2021

Uh oh!

hebasto left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	{
	QMutexLocker locker(&m_mutex);
	m_wallets.erase(std::remove(m_wallets.begin(), m_wallets.end(), wallet_model));
	}
	Q_EMIT walletRemoved(wallet_model);
	// Currently this can trigger the unload since the model can hold the last
	// CWallet shared pointer.
	delete wallet_model;

	QMutexLocker locker(&m_mutex);
	for (WalletModel* wallet_model : m_wallets) {
	wallet_model->wallet().remove();
	Q_EMIT walletRemoved(wallet_model);
	delete wallet_model;
	}
	m_wallets.clear();

	GUIUtil::ObjectInvoke(this, [wallet_model, this] {
	wallet_model->setParent(this);
	}, GUIUtil::blockingGUIThreadConnection());

	m_wallets.push_back(wallet_model);

Conversation

ryanofsky commented Jun 10, 2021

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 12, 2021

Uh oh!

hebasto commented Aug 12, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

promag left a comment •

edited

Loading

ryanofsky Aug 5, 2021 •

edited

Loading