qt: Plug many memory leaks #9190

1. 
   laanwj commented at 3:15 pm on November 19, 2016: member

   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:

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

   After:

   0SUMMARY: 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.

2. laanwj added the label GUI on Nov 19, 2016
3. 
   gmaxwell commented at 7:48 pm on November 19, 2016: contributor

   huh. I run valgrind all the time on bitcoind… but I haven’t on qt. Good finds! Reviewing…
4. 
   laanwj commented at 7:42 am on November 21, 2016: member

   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.

5. 
   jonasschnelli commented at 1:55 pm on November 21, 2016: contributor

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

   

6. 
   laanwj commented at 2:01 pm on November 21, 2016: member

   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.
7. 
   fanquake commented at 6:10 am on November 22, 2016: member

   @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:

8. 
   sipa commented at 10:33 pm on November 22, 2016: member

   Concept ACK
9. 
   paveljanik commented at 10:29 am on November 23, 2016: contributor

   utACK https://github.com/bitcoin/bitcoin/commit/4b2c2888868cb49806cd734ea36e7f5b032c3b00

   There are two more new QMenu();:

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

10. qt: Plug many memory leaks

    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.

    47db075377
11. qt: Prevent thread/memory leak on exiting RPCConsole

    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.

    693384eedb
12. qt: Avoid splash-screen related memory leak

    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.

    e4f126a7ba
13. qt: Avoid shutdownwindow-related memory leak

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

    5204598f8d
14. qt: Avoid OpenSSL certstore-related memory leak

    - Correctly manage the X509 and X509_STORE objects lifetime.

    ed998ea7a0
15. 
    laanwj commented at 11:38 am on November 23, 2016: member

    @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.
16. laanwj force-pushed on Nov 23, 2016
17. 
    paveljanik commented at 1:28 pm on November 23, 2016: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/9190/commits/ed998ea7a0ecf294211b06e9ef82f1548a621a1d
18. laanwj merged this on Nov 24, 2016
19. laanwj closed this on Nov 24, 2016

    ---

20. laanwj referenced this in commit db5e22e053 on Nov 24, 2016
21. luke-jr referenced this in commit dc46b10a08 on Dec 2, 2016
22. luke-jr referenced this in commit c12f4e93b9 on Dec 2, 2016
23. luke-jr referenced this in commit e4bea4fb84 on Dec 2, 2016
24. luke-jr referenced this in commit e5ad693f91 on Dec 2, 2016
25. luke-jr referenced this in commit 6f7841c4d4 on Dec 2, 2016
26. codablock referenced this in commit 3631e402a1 on Sep 9, 2017
27. codablock referenced this in commit fee9bf347e on Sep 11, 2017
28. UdjinM6 referenced this in commit 91d99fcd3f on Sep 11, 2017
29. lateminer referenced this in commit f6a1ce534f on Nov 11, 2018
30. MarcoFalke locked this on Sep 8, 2021

Contributors
laanwj gmaxwell jonasschnelli fanquake sipa paveljanik

Labels
GUI