qt: Plug many memory leaks #9190

laanwj · 2016-11-19T15:15:59Z

This plugs all the memory leaks that I could get a full traceback for in bitcoin-qt.

In most cases this means properly using the QObject parent hierarchy, in other cases (no qt object, or where parenting would have ancillary effects) I've used std::unique_ptr. I've introduced no new manual 'deletes' or 'frees'.

Before:

SUMMARY: AddressSanitizer: 2660170 byte(s) leaked in 39504 allocation(s).

After:

SUMMARY: AddressSanitizer: 25089 byte(s) leaked in 696 allocation(s).

The leaks that asan finds after this are likely Qt global objects, and some fontconfig related things. I was not able to track them down.

gmaxwell · 2016-11-19T19:48:46Z

huh. I run valgrind all the time on bitcoind... but I haven't on qt. Good finds! Reviewing...

laanwj · 2016-11-21T07:42:47Z

huh. I run valgrind all the time on bitcoind... but I haven't on qt. Good finds! Reviewing...

Yes, bitcoind was clean in asan too. Nice accomplishment. The GUI apparently has quite some leaks, some of which we probably can't help. None extremely serious - these are mostly objects allocated only once, although the thread leak is a bit scary for other reasons: there really shouldn't be a RPC executor thread before AppIinit() nor after Shutdown(). So it's a safety fix too.

@fanquake to answer your IRC question this was with Qt 5.5.1, the system library on Ubuntu 1604.

jonasschnelli · 2016-11-21T13:55:05Z

Tested ACK 4b2c2888868cb49806cd734ea36e7f5b032c3b00.
Looks much better now on OSX but still got some leaks reported (maybe OSX specific, maybe root source lies in Qt).

void PaymentServer::LoadRootCAs(X509_STORE* _store);
CDBEnvOpen via CWallet::Verify() and CWallet::LoadWallet()

laanwj · 2016-11-21T14:01:18Z

Huh, the leaks in PaymentServer::LoadRootCAs should have been solved with commit 4b2c2888868cb49806cd734ea36e7f5b032c3b00 . In any case I'm not claiming to plug all leaks, just most of them.

fanquake · 2016-11-22T06:10:42Z

@jonasschnelli Which version of Qt are you testing with?

I'm currently testing with Qt5.7, on OS X 10.12.1, seeing similar results. Screenshots below.

Payment Server:

SplashScreen:

sipa · 2016-11-22T22:33:57Z

Concept ACK

paveljanik · 2016-11-23T10:29:21Z

utACK 4b2c288

There are two more new QMenu();:

src/qt/coincontroldialog.cpp:    contextMenu = new QMenu();
src/qt/receiverequestdialog.cpp:    contextMenu = new QMenu();

None of these are very serious, and are leaks in objects that are created at most one time. In most cases this means properly using the QObject parent hierarchy, except for BanTablePriv/PeerTablePriv which are not QObject, so use a std::unique_ptr instead.

Make ownership of the QThread object clear, so that the RPCConsole can wait for the executor thread to quit before shutdown is called. This increases overall thread safety, and prevents some objects from leaking on exit.

Make splash screen queue its own deletion when it receives the finished command, instead of relying on WA_DeleteOnClose which doesn't work under these circumstances.

Store a reference to the shutdown window on BitcoinApplication, so that it will be deleted when exiting the main loop.

- Correctly manage the X509 and X509_STORE objects lifetime.

laanwj · 2016-11-23T11:38:15Z

@paveljanik Thanks! Verified that those don't get deleted (they likely didn't show up in my overview because I didn't check opening sub-windows apart from the debug window), so changed them too and re-pushed.

paveljanik · 2016-11-23T13:28:48Z

ACK ed998ea

ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan)

None of these are very serious, and are leaks in objects that are created at most one time. In most cases this means properly using the QObject parent hierarchy, except for BanTablePriv/PeerTablePriv which are not QObject, so use a std::unique_ptr instead. Github-Pull: bitcoin#9190 Rebased-From: 47db075

Make ownership of the QThread object clear, so that the RPCConsole can wait for the executor thread to quit before shutdown is called. This increases overall thread safety, and prevents some objects from leaking on exit. Github-Pull: bitcoin#9190 Rebased-From: 693384e

Make splash screen queue its own deletion when it receives the finished command, instead of relying on WA_DeleteOnClose which doesn't work under these circumstances. Github-Pull: bitcoin#9190 Rebased-From: e4f126a

Store a reference to the shutdown window on BitcoinApplication, so that it will be deleted when exiting the main loop. Github-Pull: bitcoin#9190 Rebased-From: 5204598

- Correctly manage the X509 and X509_STORE objects lifetime. Github-Pull: bitcoin#9190 Rebased-From: ed998ea

ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan)

* Merge bitcoin#8996: Network activity toggle 19f46f1 Qt: New network_disabled icon (Luke Dashjr) 54cf997 RPC/Net: Use boolean consistently for networkactive, and remove from getinfo (Luke Dashjr) b2b33d9 Overhaul network activity toggle (Jonas Schnelli) 32efa79 Qt: Add GUI feedback and control of network activity state. (Jon Lund Steffensen) e38993b RPC: Add "togglenetwork" method to toggle network activity temporarily (Jon Lund Steffensen) 7c9a98a Allow network activity to be temporarily suspended. (Jon Lund Steffensen) * Revert on-click behavior of network status icon to showing peers list Stay with the way Dash handled clicking on the status icon * Add theme support for network disabled icon * Merge bitcoin#8874: Multiple Selection for peer and ban tables 1077577 Fix auto-deselection of peers (Andrew Chow) addfdeb Multiple Selection for peer and ban tables (Andrew Chow) * Merge bitcoin#9190: qt: Plug many memory leaks ed998ea qt: Avoid OpenSSL certstore-related memory leak (Wladimir J. van der Laan) 5204598 qt: Avoid shutdownwindow-related memory leak (Wladimir J. van der Laan) e4f126a qt: Avoid splash-screen related memory leak (Wladimir J. van der Laan) 693384e qt: Prevent thread/memory leak on exiting RPCConsole (Wladimir J. van der Laan) 47db075 qt: Plug many memory leaks (Wladimir J. van der Laan) * Merge bitcoin#9218: qt: Show progress overlay when clicking spinner icon 042f9fa qt: Show progress overlay when clicking spinner icon (Wladimir J. van der Laan) 827d9a3 qt: Replace NetworkToggleStatusBarControl with generic ClickableLabel (Wladimir J. van der Laan) * Merge bitcoin#9266: Bugfix: Qt/RPCConsole: Put column enum in the right places df17fe0 Bugfix: Qt/RPCConsole: Put column enum in the right places (Luke Dashjr) * Merge bitcoin#9255: qt: layoutAboutToChange signal is called layoutAboutToBeChanged f36349e qt: Remove on_toggleNetworkActiveButton_clicked from RPCConsole (Wladimir J. van der Laan) 297cc20 qt: layoutAboutToChange signal is called layoutAboutToBeChanged (Wladimir J. van der Laan) * Use UniValue until bitcoin PR bitcoin#8788 is backported Network active toggle was already based on "[RPC] Give RPC commands more information about the RPC request" We need to use the old UniValue style until that one is backported * Merge bitcoin#8906: [qt] sync-overlay: Don't show progress twice fafeec3 [qt] sync-overlay: Don't show progress twice (MarcoFalke) * Merge bitcoin#8985: Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() 3154d6e [Qt] use NotifyHeaderTip's height and date for the progress update (Jonas Schnelli) 0a261b6 Use pindexBestHeader instead of setBlockIndexCandidates for NotifyHeaderTip() (Jonas Schnelli) * Merge bitcoin#9280: [Qt] Show ModalOverlay by pressing the progress bar, allow hiding 89a3723 [Qt] Show ModalOverlay by pressing the progress bar, disabled show() in sync mode (Jonas Schnelli) * Merge bitcoin#9461: [Qt] Improve progress display during headers-sync and peer-finding 40ec7c7 [Qt] Improve progress display during headers-sync and peer-finding (Jonas Schnelli) * Merge bitcoin#9588: qt: Use nPowTargetSpacing constant fa4d478 qt: Use nPowTargetSpacing constant (MarcoFalke) * Hide modal overlay forever when syncing has catched up Don't allow to open it again by clicking on the progress bar and spinner icon. Currently the overlay does not show meaningful information about masternode sync and it gives the impression of being stuck after the block chain sync is done. * Don't include chainparams.h in sendcoinsdialog.cpp This was just a remainder of a backported PR which meant to change some calculation in this file which does not apply to Dash. * Also check for fNetworkActive in ConnectNode * Merge bitcoin#9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) 988d300 [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) (practicalswift) * Merge bitcoin#11237: qt: Fixing division by zero in time remaining c8d38ab Refactor tipUpdate as per style guide (MeshCollider) 3b69a08 Fix division by zero in time remaining (MeshCollider) Pull request description: Fixes bitcoin#10291, bitcoin#11265 progressDelta may be 0 (or even negative according to 11265), this checks for that and prints unknown if it is, because we cannot calculate an estimate for the time remaining (would be infinite or negative). Tree-SHA512: bc5708e5ed6e4670d008219558c5fbb25709bd99a32c98ec39bb74f94a0b7fa058f3d03389ccdd39e6723e6b5b48e34b13ceee7c051c2db631e51d8ec3e1d68c

bitcoin#9190

laanwj added the GUI label Nov 19, 2016

laanwj added 5 commits November 23, 2016 12:33

qt: Plug many memory leaks

47db075

None of these are very serious, and are leaks in objects that are created at most one time. In most cases this means properly using the QObject parent hierarchy, except for BanTablePriv/PeerTablePriv which are not QObject, so use a std::unique_ptr instead.

qt: Prevent thread/memory leak on exiting RPCConsole

693384e

Make ownership of the QThread object clear, so that the RPCConsole can wait for the executor thread to quit before shutdown is called. This increases overall thread safety, and prevents some objects from leaking on exit.

qt: Avoid splash-screen related memory leak

e4f126a

Make splash screen queue its own deletion when it receives the finished command, instead of relying on WA_DeleteOnClose which doesn't work under these circumstances.

qt: Avoid shutdownwindow-related memory leak

5204598

Store a reference to the shutdown window on BitcoinApplication, so that it will be deleted when exiting the main loop.

qt: Avoid OpenSSL certstore-related memory leak

ed998ea

- Correctly manage the X509 and X509_STORE objects lifetime.

laanwj force-pushed the 2016_11_plug_leaks branch from 4b2c288 to ed998ea Compare November 23, 2016 11:38

laanwj merged commit ed998ea into bitcoin:master Nov 24, 2016

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Dec 2, 2016

qt: Avoid OpenSSL certstore-related memory leak

6f7841c

- Correctly manage the X509 and X509_STORE objects lifetime. Github-Pull: bitcoin#9190 Rebased-From: ed998ea

jonasschnelli mentioned this pull request Feb 21, 2017

Fix segfault crash when shutdown the GUI in disablewallet mode #9817

Merged

lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Nov 11, 2018

qt: Plug many memory leaks

f6a1ce5

bitcoin#9190

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

qt: Plug many memory leaks #9190

qt: Plug many memory leaks #9190

Uh oh!

laanwj commented Nov 19, 2016

Uh oh!

gmaxwell commented Nov 19, 2016

Uh oh!

laanwj commented Nov 21, 2016

Uh oh!

jonasschnelli commented Nov 21, 2016

Uh oh!

laanwj commented Nov 21, 2016

Uh oh!

fanquake commented Nov 22, 2016

Uh oh!

sipa commented Nov 22, 2016

Uh oh!

paveljanik commented Nov 23, 2016

Uh oh!

laanwj commented Nov 23, 2016

Uh oh!

paveljanik commented Nov 23, 2016

Uh oh!

Uh oh!

qt: Plug many memory leaks #9190

qt: Plug many memory leaks #9190

Uh oh!

Conversation

laanwj commented Nov 19, 2016

Uh oh!

gmaxwell commented Nov 19, 2016

Uh oh!

laanwj commented Nov 21, 2016

Uh oh!

jonasschnelli commented Nov 21, 2016

Uh oh!

laanwj commented Nov 21, 2016

Uh oh!

fanquake commented Nov 22, 2016

Uh oh!

sipa commented Nov 22, 2016

Uh oh!

paveljanik commented Nov 23, 2016

Uh oh!

laanwj commented Nov 23, 2016

Uh oh!

paveljanik commented Nov 23, 2016

Uh oh!

Uh oh!