Fixes MacOS 13 segfault by preventing certain notifications after main window is destroyed #680

pull john-moffett wants to merge 1 commits into bitcoin-core:master from john-moffett:202211MacSegfaultOnQuit changing 1 files +2 −2
  1. john-moffett commented at 5:39 pm on November 15, 2022: contributor

    This is a PR to address https://github.com/bitcoin/bitcoin/issues/26490

    The menu bar currently subscribes to window focus change notifications to enable or disable certain menu options in response to the window status.

    Notifications are automatically unsubscribed (disconnected in Qt parlance) if the sender is deleted – in this case, the sender is the QTApplication object (qApp). However, MacOS 13 sends a window focus change notification after the main window has been destroyed but before qApp has been fully destroyed.

    Since the menu bar is deleted in the main window’s destructor, it no longer exists when it receives these notifications (in two different places via lambda expressions). The solution is to pass the main window (this) as context when subscribing to the notifications. In this overloaded version of connect, Qt automatically unsubscribes to notifications if the sender OR context (here the main window object) is destroyed. Since the spurious notifications are sent after the main window object is destroyed, this change prevents them from being sent.

    Tested on Mac OS 13 and 12 only.

  2. Fixes bitcoin#26490 by preventing notifications 
    MacOS 13 sends a window focus change notification after the main
window has been destroyed but before the QTApplication has been
destroyed. This results in the menu bar receiving a notification
despite it no longer existing. The solution is to pass the main
window as context when subscribing to the notifications. Qt
automatically unsubscribes to notifications if the sender OR
context is destroyed.
    8a5014cd8a
  3. jarolrod added the label Bug on Nov 15, 2022
  4. jarolrod added the label macOS on Nov 15, 2022
  5. jarolrod commented at 7:20 am on November 17, 2022: member

    I have confirmed that the issue is isolated to macOS 13 and that this PR fixes the issue.

    Normally we don’t reference an issue in the commit title, and I believe this can lead to some issues and unwanted notifications. A better commit title would be: qt: pass main window as context when subscribing to notifications. Then in the commit description you can add at the bottom of what you’ve already written: closes bitcoin#26490

  6. hebasto commented at 1:36 pm on November 17, 2022: member
    Concept ACK.
  7. DrahtBot commented at 1:36 pm on November 17, 2022: 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 Count Reviewers
    ACK 1 hebasto
  8. hebasto commented at 2:06 pm on November 17, 2022: member

    @john-moffett

    Thank you for your such a nice first contribution!

    Tested 8a5014cd8a05b3ab86ae34a47653a82ce11bdf17 on macOS Ventura (M1). It fixes https://github.com/bitcoin/bitcoin/issues/26490 indeed.

    The solution is to pass the main window (this) as context when subscribing to the notifications. In this overloaded version of connect, Qt automatically unsubscribes to notifications if the sender OR context (here the main window object) is destroyed.

    I believe you was going to point at https://doc.qt.io/qt-5/qobject.html#connect-5 :)

    Indeed,

    The connection will automatically disconnect if the sender or the context is destroyed.

    Therefore, I think in our code we should avoid connections without specifying a context explicitly. For example:https://github.com/bitcoin-core/gui/blob/82fe672ea078371cfa7a504cba87e52a782235fa/src/qt/walletcontroller.cpp#L155-L158

  9. hebasto approved
  10. hebasto commented at 2:10 pm on November 17, 2022: member
    ACK 8a5014cd8a05b3ab86ae34a47653a82ce11bdf17
  11. hebasto added the label Needs backport (22.x) on Nov 17, 2022
  12. hebasto added the label Needs backport (23.x) on Nov 17, 2022
  13. hebasto added the label Needs backport (24.x) on Nov 17, 2022
  14. hebasto merged this on Nov 17, 2022
  15. hebasto closed this on Nov 17, 2022
  16. hebasto referenced this in commit fc93446117 on Nov 17, 2022
  17. hebasto cross-referenced this on Nov 17, 2022 from issue [22.x] GUI backports by hebasto
  18. hebasto referenced this in commit e54a4deff8 on Nov 17, 2022
  19. hebasto cross-referenced this on Nov 17, 2022 from issue [23.x] GUI backports by hebasto
  20. hebasto referenced this in commit 39af5f2164 on Nov 17, 2022
  21. hebasto cross-referenced this on Nov 17, 2022 from issue [24.x] GUI backports by hebasto
  23. hebasto removed the label Needs backport (22.x) on Nov 17, 2022
  24. hebasto removed the label Needs backport (23.x) on Nov 17, 2022
  25. hebasto removed the label Needs backport (24.x) on Nov 17, 2022
  26. john-moffett commented at 6:13 pm on November 17, 2022: contributor
    Thank you, @hebasto ! I did indeed link to the wrong overload :grimacing:. Thank you for the correction. I agree that context ought to be passed to connections that use lambda expressions. I’ll take a look to see if there are any other potentially vulnerable places in the code.
  27. fanquake referenced this in commit c1061be14a on Nov 18, 2022
  28. sidhujag referenced this in commit 1d3cf7bf86 on Nov 18, 2022
  29. fanquake referenced this in commit d14dc8e2c6 on Nov 21, 2022
  30. hebasto referenced this in commit 272fa25304 on Nov 21, 2022
  31. fanquake cross-referenced this on Nov 21, 2022 from issue [22.x] Bump version to 22.1rc2 & add release notes by fanquake
  32. fanquake referenced this in commit c192b86c0b on Nov 22, 2022
  33. fanquake referenced this in commit 9182b2fbae on Nov 23, 2022
  34. hebasto cross-referenced this on Nov 24, 2022 from issue macOS reports error on shutdown of GUI by xjmzx
  35. psgreco referenced this in commit 6f9b84ddf8 on Dec 7, 2022
  36. psgreco referenced this in commit c7632f6d49 on Dec 7, 2022
  37. psgreco referenced this in commit 0df78e4e73 on Dec 7, 2022
  38. psgreco referenced this in commit 7b99ac7a69 on Dec 7, 2022
  39. vertiond referenced this in commit fda1078d5e on Dec 10, 2022
  40. PastaPastaPasta referenced this in commit cb8cc71b8d on Jan 14, 2023
  41. PastaPastaPasta cross-referenced this on Jan 14, 2023 from issue fix: resolve crash on close bug in macos by PastaPastaPasta
  42. UdjinM6 referenced this in commit 512503ca13 on Jan 22, 2023
  43. hebasto cross-referenced this on Oct 7, 2023 from issue Fix wallet list hover crash on shutdown by furszy
  44. bitcoin-core locked this on Nov 17, 2023


john-moffett jarolrod hebasto DrahtBot

Labels
Bug macOS

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-10-23 00:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me