gui: Set progressDialog to nullptr #16135

1. 
   promag commented at 4:07 pm on June 2, 2019: member

   If a progress notification > 0 arrives immediately after notification = 100 then progressDialog is a dangling pointer.

   Potential fix for #16134.

2. 
   promag commented at 4:08 pm on June 2, 2019: member

   To backport.
3. fanquake added the label GUI on Jun 2, 2019
4. fanquake added the label Needs backport on Jun 2, 2019
5. fanquake added this to the milestone 0.18.1 on Jun 2, 2019
6. 
   sidhujag commented at 8:41 pm on June 2, 2019: none

   NACK

   I think you missed the one in WalletView::showProgress here: https://github.com/bitcoin/bitcoin/blob/cd42553b1178a48a16017eff0b70669c84c3895c/src/qt/walletview.cpp#L318

7. 
   promag commented at 9:01 pm on June 2, 2019: member

   @sidhujag thanks, will update branch. Also updated OP. @sidhujag does this fixes your issue?
8. promag renamed this:
   gui: Set BitcoinGUI::progressDialog to nullptr
   gui: Set progressDialog to nullptr
   on Jun 2, 2019
9. 
   sidhujag commented at 9:12 pm on June 2, 2019: none

   @promag Yes I have applied it in both places and tested 5 times and didn’t see an issue, great job +1
10. sidhujag referenced this in commit 8b7943b647 on Jun 2, 2019
11. gui: Set progressDialog to nullptr d2ae6be80f
12. promag force-pushed on Jun 2, 2019
13. 
    promag commented at 9:20 pm on June 2, 2019: member

    @sidhujag updated with your finding.
14. 
    sidhujag commented at 2:02 am on June 3, 2019: none

    TACK
15. 
    hebasto commented at 9:47 am on June 3, 2019: member

    utACK d2ae6be80f6a0156021bf8c9b9d17cd4966ddffc
16. 
    fanquake commented at 1:33 pm on June 3, 2019: member

    tACK https://github.com/bitcoin/bitcoin/pull/16135/commits/d2ae6be80f6a0156021bf8c9b9d17cd4966ddffc

    On master (c7cfd20a77ce57d200b3b9e5e0dfb0d63818abdc) & 0.18 you can cause a segfault doing:

    0src/bitcoin-cli -regtest dumpwallet ~/downloads/wallet
    1src/bitcoin-cli -regtest importwallet ~/downloads/wallet
    

     0lldb Bitcoin-Qt.app -- -regtest
     1(lldb) target create "Bitcoin-Qt.app"
     2Current executable set to 'Bitcoin-Qt.app' (x86_64).
     3(lldb) settings set -- target.run-args  "-regtest"
     4(lldb) run
     5Process 10564 launched: '/Users/michael/github/bitcoin/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' (x86_64)
     62019-06-03 09:23:45.314524-0400 Bitcoin-Qt[10564:8394825] MessageTracer: Falling back to default whitelist
     7Process 10564 stopped
     8* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
     9    frame [#0](/bitcoin-bitcoin/0/): 0x0000000100e516f1 QtWidgets`QWidgetPrivate::close_helper(QWidgetPrivate::CloseMode) + 17
    10QtWidgets`QWidgetPrivate::close_helper:
    11->  0x100e516f1 <+17>: movl   0x140(%rdi), %eax
    12    0x100e516f7 <+23>: movb   $0x1, %bl
    13    0x100e516f9 <+25>: testl  $0x200, %eax              ; imm = 0x200 
    14    0x100e516fe <+30>: jne    0x100e5192c               ; <+588>
    15Target 0: (Bitcoin-Qt) stopped.
    

    I cannot cause the same crash using this PR.

17. 
    laanwj commented at 8:09 pm on June 3, 2019: member

    Yes, it is definitely a bug to hold on to something after deleteLater. utACK
18. laanwj merged this on Jun 3, 2019
19. laanwj closed this on Jun 3, 2019

    ---

20. laanwj referenced this in commit 38523721ac on Jun 3, 2019
21. 
    practicalswift commented at 9:36 pm on June 3, 2019: contributor

    Post-merge utACK d2ae6be80f6a0156021bf8c9b9d17cd4966ddffc

    FWIW:

     0$ git grep -A1 'deleteLater()'
     1src/qt/bitcoingui.cpp:            progressDialog->deleteLater();
     2src/qt/bitcoingui.cpp-            progressDialog = nullptr;
     3--
     4src/qt/paymentserver.cpp:    reply->deleteLater();
     5src/qt/paymentserver.cpp-
     6--
     7src/qt/sendcoinsdialog.cpp:        ui->entries->takeAt(0)->widget()->deleteLater();
     8src/qt/sendcoinsdialog.cpp-    }
     9--
    10src/qt/sendcoinsdialog.cpp:    entry->deleteLater();
    11src/qt/sendcoinsdialog.cpp-
    12--
    13src/qt/splashscreen.cpp:    deleteLater(); // No more need for this
    14src/qt/splashscreen.cpp-}
    15--
    16src/qt/walletview.cpp:            progressDialog->deleteLater();
    17src/qt/walletview.cpp-            progressDialog = nullptr;
    

22. MarcoFalke referenced this in commit 773d9f3f54 on Jun 7, 2019
23. fanquake removed the label Needs backport on Jun 7, 2019
24. 
    fanquake commented at 8:02 am on June 7, 2019: member

    Backported in #16035.
25. MarcoFalke referenced this in commit d80c558e02 on Jun 7, 2019
26. HashUnlimited referenced this in commit 7ed85b788e on Aug 23, 2019
27. Bushstar referenced this in commit d1de3d8411 on Aug 24, 2019
28. jasonbcox referenced this in commit a3081fc092 on Oct 8, 2020
29. PastaPastaPasta referenced this in commit b8e50742ef on Jun 27, 2021
30. PastaPastaPasta referenced this in commit c65766fad2 on Jun 28, 2021
31. PastaPastaPasta referenced this in commit 78137e6017 on Jun 29, 2021
32. PastaPastaPasta referenced this in commit 98b4883d77 on Jul 1, 2021
33. PastaPastaPasta referenced this in commit f2511863ae on Jul 1, 2021
34. PastaPastaPasta referenced this in commit 93c1999c2f on Jul 12, 2021
35. DrahtBot locked this on Dec 16, 2021

Contributors
promag sidhujag hebasto fanquake laanwj practicalswift

Labels
GUI

Milestone
0.18.1