macOS, do not process actions during shutdown #751
Conversation
As the 'QMenuBar' is created without a parent window in MacOS, the app crashes when the user presses the shutdown button and, right after it, triggers any action in the menu bar. This happens because the QMenuBar is manually deleted in the BitcoinGUI destructor but the events attached to it children actions are not disconnected, so QActions events such us the 'QMenu::aboutToShow' could try to access null pointers. Instead of guarding every single QAction pointer inside the QMenu::aboutToShow slot, or manually disconnecting all registered events in the destructor, we can check if a shutdown was requested and discard the event. The 'node' field is a ref whose memory is held by the main application class, so it is safe to use here. Events are disconnected prior destructing the main application object. Furthermore, the 'MacDockIconHandler::dockIconClicked' signal can make the app crash during shutdown for the very same reason. The 'show()' call triggers the 'QApplication::focusWindowChanged' event, which is connected to the 'minimize_action' QAction, which is also part of the app menu bar, which could no longer exist.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
furszy
changed the title
furszy
force-pushed
the
2023_gui_fix_appbar_crash
branch
from
3d589b5
to
c08fe99
Compare
By moving the appMenuBar destruction responsibility to the QT framework, we ensure the disconnection of the submenus signals prior to the destruction of the main app window. The standalone menu bar may have served a purpose in earlier versions when it didn't contain actions that directly open specific screens within the main application window. However, at present, all the actions within the appMenuBar lead to the opening of screens within the main app window. So, the absence of a main app window makes these actions essentially pointless.
furszy
force-pushed
the
2023_gui_fix_appbar_crash
branch
from
c08fe99
to
bae209e
Compare
hebasto
changed the title
There was a problem hiding this comment.
ACK bae209e.
I had some doubts regarding the last commit due to Qt docs:
If you want all windows in a Mac application to share one menu bar, don't use this function to create it, because the menu bar created here will have this QMainWindow as its parent. Instead, you must create a menu bar that does not have a parent, which you can then share among all the Mac windows.
However, I tested this PR on macOS Monterey 12.6.9. It works flawlessly. The application menu is still available and works fine even the main window is closed and the "Node window" is opened.
|
utACK bae209e |
|
Tested with bae209e Tested on (Intel) macOS Ventura 13.6, was able to reproduce various crashes. I used the Load URL option in the File menu, which crashes without this PR. With this PR the dialog appears and then goes away when the app is done quitting, but no crash. Similiar result when opening the Information menu. Another way to trigger a crash is to hover over the Open Wallet menu option. This PR doesn't fix that, so maybe it's a different bug? The I didn't study the second commit deeply. It might be good to point to the PR where it was introduced and retry whatever bug was being fixed at the time. (in unrelated news, I'll be able to test Ventura for quite some time into the future, because my 2017 iMac doesn't support newer macOS versions) |
There was a problem hiding this comment.
Another way to trigger a crash is to hover over the Open Wallet menu option. This PR doesn't fix that, so maybe it's a different bug?
Yeah. Good catch. Just to move forward here, I can fix that one in a quick follow-up.
I didn't study the second commit deeply. It might be good to point to the PR where it was introduced and retry whatever bug was being fixed at the time.
The second commit actually fixes another set of crash causes.
Basically, can crash the app by running the shortcuts provided by the appMenuBar submenus during shutdown. For instance, executing actions like opening the information dialog (command + I) or the console dialog (command + T) lead to access null pointers.
This occurs because, in the present setup, we create a parentless appMenuBar whose submenus QActions are connected to qApp events (the app's global instance). However, at the BitcoinGUI destructor, we manually destruct this object without properly disconnecting the events. This leaves qApp events, such as focusWindowChanged, tied to submenus' QAction pointers, which causes the app to crash when it attempts to access them.
Just updated the PR description with this information too.
|
Going to merge this PR as it is a strict improvement. Follow-ups for other cases are welcome. |
|
Backported to 25.1 in bitcoin/bitcoin#28487. |
45a5fcb http: bugfix: track closed connection (stickies-v) 752a456 http: log connection instead of request count (stickies-v) ae86ada http: refactor: use encapsulated HTTPRequestTracker (stickies-v) f31899d gui: macOS, make appMenuBar part of the main app window (furszy) 64ffa94 gui: macOS, do not process dock icon actions during shutdown (furszy) e270f3f depends: fix unusable memory_resource in macos qt build (fanquake) a668394 build, macos: Fix `qt` package build with new Xcode 15 linker (Hennadii Stepanov) b3517cb test: Test loading wallets with conflicts without a chain (Andrew Chow) d63478c wallet: Check last block and conflict height are valid in MarkConflicted (Andrew Chow) 5e51a9c ci: Nuke Android APK task, Use credits for tsan (MarcoFalke) 910c362 test: ensure old fee_estimate.dat not read on restart and flushed (ismaelsadeeq) 37764d3 tx fees, policy: read stale fee estimates with a regtest-only option (ismaelsadeeq) 16bb916 tx fees, policy: do not read estimates of old fee_estimates.dat (ismaelsadeeq) c4dd598 tx fees, policy: periodically flush fee estimates to fee_estimates.dat (ismaelsadeeq) c36770c test: wallet, verify migration doesn't crash for an invalid script (furszy) 0d2a33e wallet: disallow migration of invalid or not-watched scripts (furszy) 2c51a07 Do not use std::vector = {} to release memory (Pieter Wuille) Pull request description: Further backports for the `25.x` branch. Currently: * #27622 * #27834 * #28125 * #28452 * #28542 * #28543 * #28551 * #28571 * bitcoin-core/gui#751 ACKs for top commit: hebasto: re-ACK 45a5fcb, only #28551 has been backported with since my recent [review](#28487 (review)). dergoegge: reACK 45a5fcb willcl-ark: reACK 45a5fcb Tree-SHA512: 0f5807aa364b7c2a2039fef11d5cd5e168372c3bf5b0e941350fcd92e7db4a1662801b97bb4f68e29788c77d24bbf97385a483c4501ca72d93fa25327d5694fa
…down bae209e gui: macOS, make appMenuBar part of the main app window (furszy) e14cc8f gui: macOS, do not process dock icon actions during shutdown (furszy) Pull request description: As the 'QMenuBar' is created without a parent window in MacOS, the app crashes when the user presses the shutdown button and, right after it, triggers any action in the menu bar. This happens because the QMenuBar is manually deleted in the BitcoinGUI destructor but the events attached to it children actions are not disconnected, so QActions events such us the 'QMenu::aboutToShow' could try to access null pointers. Instead of guarding every single QAction pointer inside the QMenu::aboutToShow slot, or manually disconnecting all registered events in the destructor, we can check if a shutdown was requested and discard the event. The 'node' field is a ref whose memory is held by the main application class, so it is safe to use here. Events are disconnected prior destructing the main application object. Furthermore, the 'MacDockIconHandler::dockIconClicked' signal can make the app crash during shutdown for the very same reason. The 'show()' call triggers the 'QApplication::focusWindowChanged' event, which is connected to the 'minimize_action' QAction, which is also part of the app menu bar, which could no longer exist. Another cause of crashes stems from the shortcuts provided by the `appMenuBar` submenus during shutdown. For instance, executing actions like opening the information dialog (command + I) or the console dialog (command + T) lead to access null pointers. The second commit addresses and resolves these issues. Basically, in the present setup, we create a parentless `appMenuBar` whose submenus `QActions` are connected to `qApp` events (the app's global instance). However, at the `BitcoinGUI` destructor, we manually destruct this object without properly disconnecting the events. This leaves `qApp` events, such as `focusWindowChanged`, tied to submenus' `QAction` pointers, which causes the application to crash when it attempts to access them. Important Note: This happened to me few times. The worst consequence was an inconsistent chain state during IBD. Which triggered a full "replay blocks" process on the next startup. Which was painfully slow. ACKs for top commit: RandyMcMillan: utACK bae209e hebasto: ACK bae209e. Tree-SHA512: 432e19c5f7e02c3165b7e7bd7f96f2a902bae5b5e439c2594db1c69d74ab6e0d4509d90f02db8c076f616e567e6a07492ede416ef651b5f749637398291b92fd
8b6470a gui: disable top bar menu actions during shutdown (furszy) 7066e89 gui: provide wallet controller context to wallet actions (furszy) Pull request description: Small follow-up to #751. Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list. Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers. ACKs for top commit: hebasto: ACK 8b6470a, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1). Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
As the 'QMenuBar' is created without a parent window in MacOS, the app crashes when the user presses the shutdown button and, right after it, triggers any action in the menu bar.
This happens because the QMenuBar is manually deleted in the BitcoinGUI destructor but the events attached to it children actions are not disconnected, so QActions events such us the 'QMenu::aboutToShow' could try to access null pointers.
Instead of guarding every single QAction pointer inside the QMenu::aboutToShow slot, or manually disconnecting all registered events in the destructor, we can check if a shutdown was requested and discard the event.
The 'node' field is a ref whose memory is held by the main application class, so it is safe to use here. Events are disconnected prior destructing the main application object.
Furthermore, the 'MacDockIconHandler::dockIconClicked' signal can make the app crash during shutdown for the very same reason. The 'show()' call triggers the 'QApplication::focusWindowChanged' event, which is connected to the 'minimize_action' QAction, which is also part of the app menu bar, which could no longer exist.
Another cause of crashes stems from the shortcuts provided by the
appMenuBarsubmenus during shutdown. For instance, executing actions like opening the information dialog (command + I) or the console dialog (command + T) lead to access null pointers. The second commit addresses and resolves these issues.Basically, in the present setup, we create a parentless
appMenuBarwhose submenusQActionsare connected toqAppevents (the app's global instance). However, at theBitcoinGUIdestructor, we manually destruct this object without properly disconnecting the events. This leavesqAppevents, such asfocusWindowChanged, tied to submenus'QActionpointers, which causes the app to crash when it attempts to access them.Important Note:
This happened to me few times. The worst consequence was an inconsistent chain state during IBD. Which triggered a full "replay blocks" process on the next startup. Which was painfully slow.