Skip to content

Conversation

@VerteDinde
Copy link
Member

Description of Change

Follow up to #48351

A previous refactor uncovered a long-standing bug in MenuMac::~MenuMac() 's lifetime cycle: When MenuMac::~MenuMac() was being destroyed by GC, during implicit destruction of menu_controller_, it could trigger callbacks to the model through observer notifications. Menu::~Menu() ran after, removing the observer too late.

This PR moves the observer cleanup from Menu::~Menu() to MenuMac::~MenuMac(), removing the observer before menu_controller_ is destroyed and preventing callbacks during the deallocation.

Checklist

Release Notes

Notes: Fixed an application crash on MacOS where the menu observer was not being properly removed before garbage collection.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Feb 3, 2026
@VerteDinde VerteDinde added semver/patch backwards-compatible bug fixes target/40-x-y PR should also be added to the "40-x-y" branch. target/41-x-y PR should also be added to the "41-x-y" branch. labels Feb 3, 2026
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a targeted fix

A followup here would be to move the ownership of MenuModel to the subclasses MenuMac and MenuViews, most of the operations on the model happen through the MenuController and MenuRunner platform specific classes which are both owned by the respective subclasses, the base class api::Menu should just have a virtual access to model.

@codebytere codebytere merged commit 431f77c into main Feb 4, 2026
106 of 107 checks passed
@release-clerk
Copy link

release-clerk bot commented Feb 4, 2026

Release Notes Persisted

Fixed an application crash on MacOS where the menu observer was not being properly removed before garbage collection.

@trop
Copy link
Contributor

trop bot commented Feb 4, 2026

I have automatically backported this PR to "40-x-y", please check out #49657

@trop trop bot added in-flight/40-x-y and removed target/40-x-y PR should also be added to the "40-x-y" branch. labels Feb 4, 2026
@trop
Copy link
Contributor

trop bot commented Feb 4, 2026

I have automatically backported this PR to "41-x-y", please check out #49658

@trop trop bot added in-flight/41-x-y and removed target/41-x-y PR should also be added to the "41-x-y" branch. labels Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants