macOS, do not process actions during shutdown #751

pull furszy wants to merge 2 commits into bitcoin-core:master from furszy:2023_gui_fix_appbar_crash changing 1 files +3 −7
  1. furszy commented at 2:34 pm on September 8, 2023: member

    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 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.

  2. gui: macOS, do not process dock icon actions during shutdown
    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.
    e14cc8fc69
  3. DrahtBot commented at 2:34 pm on September 8, 2023: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hebasto, RandyMcMillan
    Approach ACK hernanmarino

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. furszy renamed this:
    gui: macOS, do not process dock icon actions during shutdown
    gui: macOS, do not process actions during shutdown
    on Sep 8, 2023
  5. hernanmarino approved
  6. hernanmarino commented at 8:30 pm on September 8, 2023: contributor
    Approach ACK . I cannot test this (not my OS) but changes seem straightforward
  7. furszy force-pushed on Sep 12, 2023
  8. gui: macOS, make appMenuBar part of the main app window
    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.
    bae209e387
  9. in src/qt/bitcoingui.cpp:472 in c08fe9992b outdated
    468@@ -470,13 +469,8 @@ void BitcoinGUI::createActions()
    469 
    470 void BitcoinGUI::createMenuBar()
    471 {
    472-#ifdef Q_OS_MACOS
    473-    // Create a decoupled menu bar on Mac which stays even if the window is closed
    474-    appMenuBar = new QMenuBar();
    475-#else
    476     // Get the main window's menu bar on other platforms
    


    luke-jr commented at 4:03 pm on September 12, 2023:
    This comment no longer makes sense

    furszy commented at 5:37 pm on September 12, 2023:
    upps, thanks. Removed.
  10. furszy force-pushed on Sep 12, 2023
  11. Aminkavoos approved
  12. hebasto renamed this:
    gui: macOS, do not process actions during shutdown
    macOS, do not process actions during shutdown
    on Sep 20, 2023
  13. hebasto approved
  14. hebasto commented at 12:29 pm on September 22, 2023: member

    ACK bae209e3879fa099302d3b211362c49bbbfbdd14.

    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.

  15. hebasto commented at 12:29 pm on September 22, 2023: member
  16. RandyMcMillan commented at 3:25 am on October 2, 2023: contributor
    utACK bae209e
  17. Sjors commented at 8:08 am on October 2, 2023: member

    Tested with bae209e3879fa099302d3b211362c49bbbfbdd14

    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 m_node.shutdownRequested() seems a simple enough fix.

    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)

  18. furszy commented at 3:52 pm on October 2, 2023: member

    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.

  19. hebasto commented at 12:50 pm on October 3, 2023: member
    Going to merge this PR as it is a strict improvement. Follow-ups for other cases are welcome.
  20. hebasto merged this on Oct 3, 2023
  21. hebasto closed this on Oct 3, 2023

  22. furszy deleted the branch on Oct 3, 2023
  23. hebasto commented at 3:01 pm on October 3, 2023: member
  24. fanquake referenced this in commit 9f8d501cb8 on Oct 4, 2023
  25. Frank-GER referenced this in commit 4c482a2426 on Oct 13, 2023
  26. hebasto referenced this in commit 92704535f6 on Oct 16, 2023
  27. bitcoin-core locked this on Oct 2, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-30 16:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me