BitcoinApplication::initializeResult and BitcoinGUI::setWalletController are only called after the startup rescan is completed. While the rescan is in progress the window menus are already available.
This PR makes the Open Wallet menu disabled until BitcoinGUI::setWalletController is called.
Fixes #16087
Concept ACK.
This fixes the crashing case in #16087, however I think we could make a more extensive change, as even with this fix it’s still possible to cause a crash via the other menus.
i.e: if you open Window -> Console and try and execute any command (can be garbage) bitcoin-qt will crash:
0On branch pull/16118/local-merge
1
2Process 89287 launched: '/Users/michael/github/bitcoin/src/qt/bitcoin-qt' (x86_64)
32019-05-29 09:57:56.276746-0400 bitcoin-qt[89287:939924] MessageTracer: Falling back to default whitelist
4Process 89287 stopped
5* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
6 frame [#0](/bitcoin-bitcoin/0/): 0x00000001015158ba QtWidgets`QCompleter::popup() const + 10
7QtWidgets`QCompleter::popup:
8-> 0x1015158ba <+10>: movq 0x8(%rdi), %rbx
9 0x1015158be <+14>: movq 0x88(%rbx), %rax
10 0x1015158c5 <+21>: testq %rax, %rax
11 0x1015158c8 <+24>: jne 0x101515942 ; <+146>
12Target 0: (bitcoin-qt) stopped.
tACK https://github.com/bitcoin/bitcoin/pull/16122/commits/2d8ad2f99710a8981e33fe2d6ce834b0076c4e80 on macOS
Using master (https://github.com/bitcoin/bitcoin/commit/c7cfd20a77ce57d200b3b9e5e0dfb0d63818abdc) you can trigger a crash by mousing over the File -> Open Wallet menu during startup:
0./autogen.sh && ./configure && make check -j6 && make deploy
1lldb Bitcoin-Qt.app
2(lldb) target create "Bitcoin-Qt.app"
3Current executable set to 'Bitcoin-Qt.app' (x86_64).
4(lldb) run
5Process 35368 launched: '/Users/michael/github/bitcoin/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt' (x86_64)
62019-05-31 09:12:37.511797-0400 Bitcoin-Qt[35368:3580703] MessageTracer: Falling back to default whitelist
7Process 35368 stopped
8* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
9 frame [#0](/bitcoin-bitcoin/0/): 0x000000010192fb8e QtCore`QMutex::lock() + 14
10QtCore`QMutex::lock:
11-> 0x10192fb8e <+14>: lock
12 0x10192fb8f <+15>: cmpxchgq %rcx, (%rdi)
13 0x10192fb93 <+19>: je 0x10192fba8 ; <+40>
14 0x10192fb95 <+21>: movq %rax, %rbx
15Target 0: (Bitcoin-Qt) stopped.
Using this PR (https://github.com/bitcoin/bitcoin/pull/16118/commits/75485ef0962a53946f17b761c4445627b07e6eff) the File menu items are disabled during startup:
0merge 16118
1make check -j6 && make deploy
2lldb Bitcoin-Qt.app
The other crash I mentioned above is being fixed separately in #16122.
@promag, you mentioned that this change caused a bug listing wallets in #16230 (fixed in #16231), that you, me, jonas, and fanquake all didn’t notice during review.
Could you explain how this PR caused the bug? Also, it’d be good to know if you have any suggestions for things we should look for during reviews, or better ways to structure code to prevent initialization bugs in the future.
Like you said above, and considering this was already a fix to another problem, at the time I didn’t pay attention to remaining functionality. I guess other reviewers made the same mistake. Ideally we should have tests to prevent regressions like you say in #16075.
With this change the following
https://github.com/bitcoin/bitcoin/blob/e2182b02b5af13f0de38cf8b08bb81723387c570/src/qt/bitcoingui.cpp#L371
is worthless as the menu is only created and set later on setWalletController.
I’m a bit surprised it works because (haven’t checked for runtime warnings though) ->menu() is null. The result is that the menu is never updated.
Not sure if there’s a way to abort if QObject::connect() is called with nullptr.
m_open_wallet_action->setMenu()), a good thing to do would be to scroll down and make sure the uninitialized object (m_open_wallet_action->menu()) is not used in the meantime.
@ryanofsky actually running with -debug it shows a warning:
02019-06-18T15:16:13Z GUI: QObject::connect(QMenu, Unknown): invalid null parameter
@ryanofsky So after some googling a suggestion is to run like:
0QT_FATAL_WARNINGS=1 lldb src/qt/bitcoin-qt -- --regtest -debug
1...
2bt
3...
4* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = signal SIGABRT
5 * frame [#0](/bitcoin-bitcoin/0/): 0x00007fff653d823e libsystem_kernel.dylib`__pthread_kill + 10
6 frame [#1](/bitcoin-bitcoin/1/): 0x00007fff6548ec1c libsystem_pthread.dylib`pthread_kill + 285
7 frame [#2](/bitcoin-bitcoin/2/): 0x00007fff653411c9 libsystem_c.dylib`abort + 127
8 frame [#3](/bitcoin-bitcoin/3/): 0x0000000101c2e2d9 QtCore`___lldb_unnamed_symbol169$$QtCore + 9
9 frame [#4](/bitcoin-bitcoin/4/): 0x0000000101c2ee0b QtCore`QMessageLogger::warning(char const*, ...) const + 241
10 frame [#5](/bitcoin-bitcoin/5/): 0x0000000101e328ff QtCore`QObjectPrivate::connectImpl(QObject const*, int, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) + 863
11 frame [#6](/bitcoin-bitcoin/6/): 0x0000000101e32507 QtCore`QObject::connectImpl(QObject const*, void**, QObject const*, void**, QtPrivate::QSlotObjectBase*, Qt::ConnectionType, int const*, QMetaObject const*) + 359
12 frame [#7](/bitcoin-bitcoin/7/): 0x000000010001af79 bitcoin-qt`BitcoinGUI::createActions() [inlined] std::__1::enable_if<(QtPrivate::FunctionPointer<BitcoinGUI::createActions()::$_14>::ArgumentCount) == (-(1)), QMetaObject::Connection>::type QObject::connect<void (sender=0x0000000000000000, context=0x0000000000000000, slot=<unavailable>, type=DirectConnection)(), BitcoinGUI::createActions()::$_14>(QtPrivate::FunctionPointer<void (QMenu::*)()>::Object const*, void (QMenu::*)(), QObject const*, BitcoinGUI::createActions()::$_14, Qt::ConnectionType) at qobject.h:329:16 [opt]
13 frame [#8](/bitcoin-bitcoin/8/): 0x000000010001af1f bitcoin-qt`BitcoinGUI::createActions() [inlined] std::__1::enable_if<(QtPrivate::FunctionPointer<BitcoinGUI::createActions()::$_14>::ArgumentCount) == (-(1)), QMetaObject::Connection>::type QObject::connect<void (sender=0x0000000000000000, signal=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00, slot=<unavailable>)(), BitcoinGUI::createActions()::$_14>(QtPrivate::FunctionPointer<void (QMenu::*)()>::Object const*, void (QMenu::*)(), BitcoinGUI::createActions()::$_14) at qobject.h:302 [opt]
14 frame [#9](/bitcoin-bitcoin/9/): 0x000000010001af1f bitcoin-qt`BitcoinGUI::createActions(this=0x0000000102b4d700) at bitcoingui.cpp:371 [opt]
15 frame [#10](/bitcoin-bitcoin/10/): 0x000000010001732c bitcoin-qt`BitcoinGUI::BitcoinGUI(this=0x0000000102b4d700, node=0x00000001029d7a40, _platformStyle=0x0000000102d81e70, networkStyle=<unavailable>, parent=<unavailable>) at bitcoingui.cpp:120:5 [opt]
16 frame [#11](/bitcoin-bitcoin/11/): 0x000000010000abe4 bitcoin-qt`BitcoinApplication::createWindow(this=0x00007ffeefbff660, networkStyle=<unavailable>) at bitcoin.cpp:239:18 [opt]
17 frame [#12](/bitcoin-bitcoin/12/): 0x000000010000d2cd bitcoin-qt`GuiMain(argc=<unavailable>, argv=<unavailable>) at bitcoin.cpp:569:13 [opt]
18 frame [#13](/bitcoin-bitcoin/13/): 0x00007fff65298ed9 libdyld.dylib`start + 1
Which clearly shows the problem. Going to add this to doc/productivity.md.
Going to add this to doc/productivity.md.
Added to Developer Notes for Qt Code :)