Fix crash on selecting "Mask values" in transaction view #774

This PR fixes a crash bug that can be caused with the following steps:

• change to the "Transactions" view
• right-click on an arbitrary transaction -> "Show transaction details"
• close the transaction detail window again
• select menu item "Settings" -> "Mask values"

The problem is that the list of opened dialogs, tracked in the member variable m_opened_dialogs (introduced in #708, commit 4492de1be11f4131811f504a037342c78f480e20), is only ever appended with newly opened transaction detail dialog pointers, but never removed. This leads to dangling pointers in the list, and if the "Mask values" menu item is selected, a crash is caused in the course of trying to close the opened transaction detail dialogs (see closeOpenedDialogs() method). Fix this by removing a pointer of the list if the corresponding widget is destroyed.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

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.

Tagging @furszy who signaled interest in this crash bug while talking earlier.

Concept ACK

Good catch! I'm the culprit perhaps, I'll take a deeper look at it ASAP!

I took a quick look at the code and LGTM, the original issue with the multiple transaction details dialogs was caught by another reviewer and solved on the original PR #708 but this is a different one.

Just force-pushed a much simpler version (only three lines) which avoids the circular dependency and the introduction of new methods. The old version is still available here for reference (or in case the current solution has problems that I'm not aware of): https://github.com/bitcoin-core/gui/commit/757e58d6f4cc9592de3d960df0eee44f1cbf6c7d

tACK e26e665f9f64a962dd56053be817cc953e714847

I easily managed to reproduce the issue in masterfollowing the instructions in the description of the PR (bitcoin-qtcrashes with:Segmentation fault (core dumped)output).

I couldn't reproduce the error anymore while using the new bitcoin-qt version compiled with this branch.

I'd prefer this second approach better. Thank for fixing it!

tack e26e665f9f64a962dd56053be817cc953e714847

Could also:

diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
@@ -656,7 +656,7 @@
 {
     // close all dialogs opened from this view
     for (QDialog* dlg : m_opened_dialogs) {
-        dlg->close();
+        if (dlg) dlg->close();
     }
     m_opened_dialogs.clear();
 }

utACK e26e665f9

Could also:

diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
@@ -656,7 +656,7 @@
 {
     // close all dialogs opened from this view
     for (QDialog* dlg : m_opened_dialogs) {
-        dlg->close();
+        if (dlg) dlg->close();
     }
     m_opened_dialogs.clear();
 }

This wouldn't change the behaviour, as a pointer in the m_opened_dialogs list is not set to NULL after the dialog it points to is destroyed. I think it could work if we used guarded pointers like QPointer in the list instead, IIUC (but we don't seem to use them anywhere in our codebase).

Should this be backported?

Should this be backported?

Yes, I think so. The bug first occured with release 25.0 (according to git tag --contains 4492de1be11 in the main repo), so backporting to 25.x and 26.x would make sense.

ACK e26e665f9f64a962dd56053be817cc953e714847

I do agree with @theStack backporting (checked it with the git tag command) since the issue technically is the "conflict" between these 2 lines: https://github.com/bitcoin-core/gui/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/qt/transactionview.cpp#L532-L533

The last one (#L533) was added by the #708 (merged in v25 - I wanted to say I'm innocent but no haha), so another way to solve this issue is just by removing #L532, which I've tested and works fine.

ACK e26e665f9f64a962dd56053be817cc953e714847, tested on Ubuntu 22.04.

Will backport this once merged.

https://github.com/bitcoin-core/gui/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/qt/transactionview.cpp#L532-L533

The last one (#L533) was added by the #708 (merged in v25 - I wanted to say I'm innocent but no haha), so another way to solve this issue is just by removing #L532, which I've tested and works fine.

It will still allow to grow the size of m_opened_dialogs unbounded.

The current PR branch tackles with m_opened_dialogs size, which is the optimum solution.

Added to https://github.com/bitcoin/bitcoin/pull/28754 for 26.x.

Pulled this into a branch for 25.x backporting. See https://github.com/bitcoin/bitcoin/pull/28768.