fix importwallet crash due to out of scope wallet reference #16134

1. 
   sidhujag commented at 4:39 am on June 2, 2019: none

   showProgress hold’s a reference that is invalidated because wallet is used outside of the locked scope and inside of RescanWallet it is locked into a new context.. calling showProgress outside of the wallet lock context results in intermittent crashes of core.
2. fix importwallet crash due to out of scope wallet reference

   showProgress hold's a reference that is invalidated because wallet is used outside of the locked scope and inside of RescanWallet it is locked into a new context.. calling showProgress outside of the wallet lock context results in intermittent crashes of core.

   43423786a6
3. fanquake added the label RPC/REST/ZMQ on Jun 2, 2019
4. fanquake added the label Wallet on Jun 2, 2019
5. 
   MarcoFalke commented at 8:26 am on June 2, 2019: member

   Could you explain which member needs to be guarded by the lock, but istn’t. Also how does the crash look like? Are there issues open about the crash?
6. 
   sidhujag commented at 2:14 pm on June 2, 2019: none

   I think it’s the wallet and locked chain it’s outside the scope and the crash is in showProgress on memory cleanup. I can attach the user log on my OS X to show the exact line and stack but it’s likely because in rescanwallet it tries to update showProgress again and I don’t know if those calls are idempotent
7. 
   fanquake commented at 2:16 pm on June 2, 2019: member

   @sidhujag If you have logs / more info please put them up here.
8. 
   sidhujag commented at 2:25 pm on June 2, 2019: none

   Thread 0 Crashed:: bitcoin-main Dispatch queue: com.apple.main-thread 0 org.qt-project.QtWidgets 0x0000000103b4d4a7 QWidgetPrivate::close_helper(QWidgetPrivate::CloseMode) + 23 1 bitcoin-qt 0x0000000102687ab3 BitcoinGUI::showProgress(QString const&, int) + 67 (bitcoingui.cpp:1433) 2 org.qt-project.QtCore 0x0000000104807e42 QMetaObject::activate(QObject*, int, int, void**) + 1474 3 bitcoin-qt 0x000000010275cefc ClientModel::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 732 4 org.qt-project.QtCore 0x00000001048009f1 QObject::event(QEvent*) + 769 5 org.qt-project.QtWidgets 0x0000000103b2446d QApplicationPrivate::notify_helper(QObject*, QEvent*) + 269 6 org.qt-project.QtWidgets 0x0000000103b25897 QApplication::notify(QObject*, QEvent*) + 583 7 org.qt-project.QtCore 0x00000001047d6f64 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 164 8 org.qt-project.QtCore 0x00000001047d80d7 QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) + 791

   the last entry of debug.log 2019-06-02T14:22:58Z [default wallet] RescanFromTime: Rescanning last 253 blocks

   if I remove that showProgress call in the commit I did, the progress closes and no more crash. It happens fairly regular if you delete existing wallet.dat and run an import.

9. 
   fanquake commented at 4:09 pm on June 2, 2019: member

   Thanks @sidhujag, I’ve recreated the crash using master (c7cfd20a77ce57d200b3b9e5e0dfb0d63818abdc):

    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 70618 launched: '/Users/michael/github/bitcoin/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' (x86_64)
    62019-06-02 11:56:53.744068-0400 Bitcoin-Qt[70618:5085197] MessageTracer: Falling back to default whitelist
    7Process 70618 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.
   

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

    From the stack trace looks like it tries to close an invalid progress dialog. Considering current master, the progress dialog pointer isn’t reset to nullptr after deletion. Please test with #16135 instead.
11. 
    fanquake commented at 4:16 pm on June 2, 2019: member

    Checked that the same crash occurs using 0.18.0 (2472733a24a9364e4c6233ccd04166a26a68cc65).
12. 
    sidhujag commented at 8:43 pm on June 2, 2019: none

    Thanks @fanquake + @promag
13. sidhujag referenced this in commit 8b7943b647 on Jun 2, 2019
14. 
    fanquake commented at 9:13 pm on June 2, 2019: member

    @sidhujag Thanks for reporting the bug, and providing an initial fix. I’m going to close this in favor of #16135, if you could test the changes there that’d be great.
15. fanquake closed this on Jun 2, 2019

    ---

16. laanwj referenced this in commit 38523721ac on Jun 3, 2019
17. sidhujag deleted the branch on Jun 21, 2019
18. MarcoFalke deleted a comment on Jul 1, 2019
19. MarcoFalke deleted a comment on Jul 1, 2019
20. PastaPastaPasta referenced this in commit b8e50742ef on Jun 27, 2021
21. PastaPastaPasta referenced this in commit c65766fad2 on Jun 28, 2021
22. PastaPastaPasta referenced this in commit 78137e6017 on Jun 29, 2021
23. PastaPastaPasta referenced this in commit 98b4883d77 on Jul 1, 2021
24. PastaPastaPasta referenced this in commit f2511863ae on Jul 1, 2021
25. PastaPastaPasta referenced this in commit 93c1999c2f on Jul 12, 2021
26. DrahtBot locked this on Dec 16, 2021

Contributors
sidhujag MarcoFalke fanquake promag

Labels
Wallet RPC/REST/ZMQ