Skip to content

gui: Fix leak in CoinControlDialog::updateView #18578

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 13, 2020
Merged

gui: Fix leak in CoinControlDialog::updateView #18578

merged 1 commit into from
May 13, 2020

Conversation

promag
Copy link
Contributor

@promag promag commented Apr 9, 2020 •
edited
Loading

Taken from #17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where CCoinControlWidgetItem are unnecessarily created and leaked.

@fanquake fanquake added the GUI label Apr 9, 2020
@promag promag changed the title gui: Fix itemWalletAddress leak when not tree mode gui: Fix leak in CoinControlDialog::updateView Apr 11, 2020
@promag
Copy link
Contributor Author

promag commented May 4, 2020

@jonasschnelli friendly ping.

@hebasto
Copy link
Member

hebasto commented May 4, 2020

Concept ACK.

@hebasto
Copy link
Member

hebasto commented May 4, 2020

@promag How could I observe the Qt warning that is fixed by f95ebe7?

hebasto
hebasto reviewed May 4, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a3a02dd:

This constructor

bitcoin/src/qt/coincontroldialog.h

Line 34 in a3a02dd

explicit CCoinControlWidgetItem(int type = Type) : QTreeWidgetItem(type) {}
is unused now and could be removed.

@promag promag force-pushed the 2020-fix-coincontroldialog-leak branch from a3a02dd to d8d9342 Compare May 4, 2020 09:12
@promag
Copy link
Contributor Author

promag commented May 4, 2020

@promag How could I observe the Qt warning that is fixed by f95ebe7?

I've seen the warning while working on #17457.

is unused now and could be removed.

Thanks, removed.

@hebasto
Copy link
Member

hebasto commented May 4, 2020

@promag How could I observe the Qt warning that is fixed by f95ebe7?

I've seen the warning while working on #17457.

Mind providing steps to reproduce? I've already tried to run test_bitcoin-qt with QT_QPA_PLATFORM=xcb and with unset QT_QPA_PLATFORM, and did not spot "Cannot queue arguments of type size_t" warning.
I'm so curious because it could be the first qRegisterMetaType in the qt/test/test_main.cpp.

Another suggestion: if this commit is related to #17457, maybe move it to #17457?

@jonasschnelli
Copy link
Contributor

The first commit looks indeed after a memory leak fix.
How relevant is the second commit f95ebe7?

@promag promag force-pushed the 2020-fix-coincontroldialog-leak branch from d8d9342 to e8123ea Compare May 4, 2020 11:06
@promag
Copy link
Contributor Author

promag commented May 4, 2020

@jonasschnelli @hebasto dropped f95ebe7 and rebased.

hebasto
hebasto approved these changes May 4, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e8123ea, tested on Linux Mint 19.3.

luke-jr
luke-jr approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a3a02dd

@jonasschnelli jonasschnelli added this to the 0.20.0 milestone May 13, 2020
@jonasschnelli
Copy link
Contributor

utACK e8123ea

@jonasschnelli jonasschnelli merged commit 8d17f8d into bitcoin:master May 13, 2020
@promag promag deleted the 2020-fix-coincontroldialog-leak branch May 13, 2020 08:29
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 14, 2020
@jonasschnelli
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
4460ecf 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 14, 2020
@promag @fanquake
gui: Fix itemWalletAddress leak when not tree mode
63e957a 
Github-Pull: bitcoin#18578
Rebased-From: e8123ea
@fanquake fanquake mentioned this pull request May 14, 2020
Merged
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 15, 2020
@promag @fanquake
gui: Fix itemWalletAddress leak when not tree mode
315ae14 
Github-Pull: bitcoin#18578
Rebased-From: e8123ea
maflcko pushed a commit that referenced this pull request May 15, 2020
Merge #18973: [0.20] Final backports for rc2
17bdf2a 
245c862 test: disable script fuzz tests (fanquake)
9a8fb4c fuzz: Remove enumeration of expected deserialization exceptions in ProcessMessage(...) fuzzer (practicalswift)
6161c94 [net processing] Only send a getheaders for one block in an INV (John Newbery)
cf2a6e2 test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)
cc7d344 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
37a6207 test: Add unregister_validation_interface_race test (MarcoFalke)
ff4dc20 gui: Fix manual coin control with multiple wallets loaded (João Barbosa)
ed0afe8 test: Add test for conflicted wallet tx notifications (Russell Yanofsky)
251e321 rpc: Relock wallet only if most recent callback (João Barbosa)
ca4dac4 rpc: Add mutex to guard deadlineTimers (João Barbosa)
a3fe458 [docs] Improve commenting in ProcessGetData() (John Newbery)
011532e [test] test that an invalid GETDATA doesn't prevent processing of future messages (Amiti Uttarwar)
1e73d72 [net processing] ignore unknown INV types in GETDATA messages (Amiti Uttarwar)
fb82173 [net processing] ignore tx GETDATA from blocks-only peers (Amiti Uttarwar)
315ae14 gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Backports the following PRs to the 0.20 branch:

  * #18578: gui: Fix leak in CoinControlDialog::updateView
  * #18808: [net processing] Drop unknown types in getdata
  * #18814: rpc: Relock wallet only if most recent callback
  * #18878: test: Add test for conflicted wallet tx notifications
  * #18894: gui: Fix manual coin control with multiple wallets loaded
  * #18742: miner: Avoid stack-use-after-return in validationinterface
  * #18962: net processing: Only send a getheaders for one block in an INV
  * #18975: test: Remove const to work around compiler error on xenial

ACKs for top commit:
  promag:
    Tested ACK 245c862 coin control with multiple wallets.
  laanwj:
    ACK 245c862
  MarcoFalke:
    ACK 245c862 solved the conflicts myself as a sanity check. Did not re-review 🍷

Tree-SHA512: 285e5a5fad5bbeba6032742c65dc68836e8eccfcceda9e69fec4ddd162a3f61679a96f9bbe3d434267835af67c21ac4c05accf6f63e827c2eb47203c6daafe31
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 7, 2020
@MarkLTZ
[gui] Fix leak in CoinControlDialog::updateView (bitcoin#18578)
a4b5be9
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 28, 2021
@promag @PiRK
gui: Fix itemWalletAddress leak when not tree mode
225d8df 
Summary: This is a backport of Core [[bitcoin/bitcoin#18578 | PR18578]]

Test Plan:
`ninja && src/qt/bitcoin-qt`
In the Send tab, click the "Inputs" button, select "Tree mode".

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D9097
backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021
@promag @backpacker69
gui: Fix itemWalletAddress leak when not tree mode
20f91cf 
Github-Pull: #18578
Rebased-From: e8123ea
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
66a9fa0 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
472a487 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
0863d79 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
b03649b 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
7434733 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
1421e71 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
@jonasschnelli @PastaPastaPasta
Merge bitcoin#18578: gui: Fix leak in CoinControlDialog::updateView
8af8bbc 
e8123ea gui: Fix itemWalletAddress leak when not tree mode (João Barbosa)

Pull request description:

  Taken from bitcoin#17457, the first commit is a similar to 88a94f7 but for test binary, and the second commit fixes a leak where `CCoinControlWidgetItem` are unnecessarily created and leaked.

ACKs for top commit:
  jonasschnelli:
    utACK e8123ea
  hebasto:
    ACK e8123ea, tested on Linux Mint 19.3.

Tree-SHA512: 8b43cb29de103842ce5f048de51222919540d3212d2873c16731145e856178644041924ad0e9a58c2ff08f209a9b4ac26dc9965289eb719da233c0984f93631e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@luke-jr luke-jr luke-jr approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.
5 participants
@promag @hebasto @jonasschnelli @luke-jr @fanquake