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

john-moffett · 2022-11-15T17:39:20Z

This is a PR to address bitcoin/bitcoin#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.

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.

jarolrod · 2022-11-17T07:20:16Z

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

hebasto · 2022-11-17T13:36:48Z

Concept ACK.

DrahtBot · 2022-11-17T13:36:50Z

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

hebasto · 2022-11-17T14:06:53Z

@john-moffett

Thank you for your such a nice first contribution!

Tested 8a5014c on macOS Ventura (M1). It fixes bitcoin/bitcoin#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:

gui/src/qt/walletcontroller.cpp

Lines 155 to 158 in 82fe672

    
           connect(qApp, &QApplication::focusWindowChanged, wallet_model, [this, wallet_model]() { 
        
               if (!QApplication::activeModalWidget()) { 
        
                   removeAndDeleteWallet(wallet_model); 
        
               }

hebasto

ACK 8a5014c

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. Github-Pull: bitcoin-core/gui#680 Rebased-From: 8a5014c

hebasto · 2022-11-17T14:46:47Z

Backported to:

22.x in [22.x] GUI backports bitcoin/bitcoin#26521
23.x in [23.x] GUI backports bitcoin/bitcoin#26522
24.x in [24.x] GUI backports bitcoin/bitcoin#26523

john-moffett · 2022-11-17T18:13:43Z

Thank you, @hebasto ! I did indeed link to the wrong overload 😬. 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.

39af5f2 Fixes #26490 by preventing notifications (John Moffett) Pull request description: Backports: - bitcoin-core/gui#680 ACKs for top commit: jarolrod: ACK 39af5f2 Tree-SHA512: 1c84703395cc750611922aa2c59dc2e2f2e98c92845859f9f57e30ee5371c9a31690318230ab514f2d9b71e75a716a30280aa1e585b6da31e45fd970017bc74e

…tain notifications after main window is destroyed 8a5014c Fixes bitcoin#26490 by preventing notifications (John Moffett) Pull request description: This is a PR to address bitcoin#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](https://doc.qt.io/qt-5/qobject.html#connect-1) 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. ACKs for top commit: hebasto: ACK 8a5014c Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090

e54a4de Fixes #26490 by preventing notifications (John Moffett) Pull request description: Backports: - bitcoin-core/gui#680 ACKs for top commit: jarolrod: ACK e54a4de Tree-SHA512: b81c73ece3f3c1e1d1c81bd0bb5b80a47488bca1fa43bbed25bab859cd063cd9b3acc1cff76f07961c3bd01276fab2fad8ea10b9d06e18965a198e78ff1f6705

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. Github-Pull: bitcoin-core/gui#680 Rebased-From: 8a5014c

272fa25 Fixes #26490 by preventing notifications (John Moffett) 7b7bbc1 Disallow encryption of watchonly wallets (Andrew Chow) Pull request description: Backports: - bitcoin-core/gui#631 - bitcoin-core/gui#680 ACKs for top commit: jarolrod: ACK 272fa25 Tree-SHA512: 4c285327464240ace3884d9653cc46d8e7b60b888f3b096ceb4fd5000d084ea8d97f1ef86ca1dea8dc7d3be8cdd2da19eece2b8c5b7351c5961b50b78fcd4c4d

d9bd628 doc: add release notes for 22.1rc2 (fanquake) 6523107 doc: Update manual pages for 22.1rc2 (fanquake) 6af7af6 build: Bump version to 22.1rc2 (fanquake) Pull request description: Bump the version to 22.1rc2. Regenerate the man pages. Add WIP 22.1 release notes. Changes since rc1: - bitcoin-core/gui#631 - bitcoin-core/gui#680 ACKs for top commit: stickies-v: ACK [d9bd628](d9bd628) jarolrod: ACK d9bd628 Tree-SHA512: 70b1723fd5f77a93763ffc153b18c5d6c11c8294828406bd5e93daf9e8aac5e62306280ef6601508b4d22e1cce5136687afc826be6d159816071549849c40f91

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. Github-Pull: bitcoin-core/gui#680 Rebased-From: 8a5014c (cherry picked from commit 272fa25)

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. Github-Pull: bitcoin-core/gui#680 Rebased-From: 8a5014c

…tain notifications after main window is destroyed 8a5014c Fixes bitcoin#26490 by preventing notifications (John Moffett) Pull request description: This is a PR to address bitcoin#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](https://doc.qt.io/qt-5/qobject.html#connect-1) 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. ACKs for top commit: hebasto: ACK 8a5014c Tree-SHA512: 3dff0a252fe0e93dd68cf5503135ecf6a72bcf385ba38407d6021ab77cca323f8bbe58aeca90ec124aa2a22ab9d35b706946179ac3b5d171c96a7010de51a090

jarolrod added Bug Something isn't working macOS labels Nov 15, 2022

hebasto approved these changes Nov 17, 2022

View reviewed changes

hebasto added Needs backport (22.x) labels Nov 17, 2022

hebasto merged commit fb01af6 into bitcoin-core:master Nov 17, 2022

hebasto mentioned this pull request Nov 17, 2022

[22.x] GUI backports bitcoin/bitcoin#26521

Merged

hebasto mentioned this pull request Nov 17, 2022

[23.x] GUI backports bitcoin/bitcoin#26522

Merged

hebasto mentioned this pull request Nov 17, 2022

[24.x] GUI backports bitcoin/bitcoin#26523

Merged

hebasto removed Needs backport (22.x) labels Nov 17, 2022

fanquake mentioned this pull request Nov 21, 2022

[22.x] Bump version to 22.1rc2 & add release notes bitcoin/bitcoin#26547

Merged

hebasto mentioned this pull request Nov 24, 2022

macOS reports error on shutdown of GUI bitcoin/bitcoin#26490

Closed

PastaPastaPasta mentioned this pull request Jan 14, 2023

fix: resolve crash on close bug in macos dashpay/dash#5162

Merged

5 tasks

hebasto mentioned this pull request Oct 7, 2023

Fix wallet list hover crash on shutdown #765

Merged

bitcoin-core locked and limited conversation to collaborators Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

john-moffett commented Nov 15, 2022 •

edited

Loading

Uh oh!

jarolrod commented Nov 17, 2022

Uh oh!

hebasto commented Nov 17, 2022

Uh oh!

DrahtBot commented Nov 17, 2022 •

edited

Loading

Uh oh!

hebasto commented Nov 17, 2022 •

edited

Loading

Uh oh!

hebasto left a comment

Uh oh!

hebasto commented Nov 17, 2022

Uh oh!

john-moffett commented Nov 17, 2022

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

john-moffett commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Nov 17, 2022

Uh oh!

hebasto commented Nov 17, 2022

Uh oh!

DrahtBot commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

hebasto commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 17, 2022

Uh oh!

john-moffett commented Nov 17, 2022

Uh oh!

Uh oh!

john-moffett commented Nov 15, 2022 •

edited

Loading

DrahtBot commented Nov 17, 2022 •

edited

Loading

hebasto commented Nov 17, 2022 •

edited

Loading