From 7b0a84477ae8ae8817c560cd687e3222087b9d58 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 27 May 2020 13:54:52 -0700 Subject: [PATCH] fix: weakly reference MenuModel from MenuController (#23778) --- .../ui/cocoa/electron_menu_controller.h | 8 +- .../ui/cocoa/electron_menu_controller.mm | 78 +++++++++++++++---- shell/browser/ui/electron_menu_model.h | 7 ++ 3 files changed, 73 insertions(+), 20 deletions(-) diff --git a/shell/browser/ui/cocoa/electron_menu_controller.h b/shell/browser/ui/cocoa/electron_menu_controller.h index 886fe7f81046..5cc94a2e526b 100644 --- a/shell/browser/ui/cocoa/electron_menu_controller.h +++ b/shell/browser/ui/cocoa/electron_menu_controller.h @@ -10,6 +10,7 @@ #include "base/callback.h" #include "base/mac/scoped_nsobject.h" +#include "base/memory/weak_ptr.h" #include "base/strings/string16.h" namespace electron { @@ -24,15 +25,13 @@ class ElectronMenuModel; // as it only maintains weak references. @interface ElectronMenuController : NSObject { @protected - electron::ElectronMenuModel* model_; // weak + base::WeakPtr model_; base::scoped_nsobject menu_; BOOL isMenuOpen_; BOOL useDefaultAccelerator_; base::OnceClosure closeCallback; } -@property(nonatomic, assign) electron::ElectronMenuModel* model; - // Builds a NSMenu from the pre-built model (must not be nil). Changes made // to the contents of the model after calling this will not be noticed. - (id)initWithModel:(electron::ElectronMenuModel*)model @@ -46,6 +45,9 @@ class ElectronMenuModel; // Programmatically close the constructed menu. - (void)cancel; +- (electron::ElectronMenuModel*)model; +- (void)setModel:(electron::ElectronMenuModel*)model; + // Access to the constructed menu if the complex initializer was used. If the // default initializer was used, then this will create the menu on first call. - (NSMenu*)menu; diff --git a/shell/browser/ui/cocoa/electron_menu_controller.mm b/shell/browser/ui/cocoa/electron_menu_controller.mm index 8212f5f4f9d9..d8da2457938a 100644 --- a/shell/browser/ui/cocoa/electron_menu_controller.mm +++ b/shell/browser/ui/cocoa/electron_menu_controller.mm @@ -8,6 +8,7 @@ #include #include "base/logging.h" +#include "base/mac/foundation_util.h" #include "base/strings/sys_string_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/task/post_task.h" @@ -87,6 +88,44 @@ NSMenu* MakeEmptySubmenu() { } // namespace +// This class stores a base::WeakPtr as an +// Objective-C object, which allows it to be stored in the representedObject +// field of an NSMenuItem. +@interface WeakPtrToElectronMenuModelAsNSObject : NSObject ++ (instancetype)weakPtrForModel:(electron::ElectronMenuModel*)model; ++ (electron::ElectronMenuModel*)getFrom:(id)instance; +- (instancetype)initWithModel:(electron::ElectronMenuModel*)model; +- (electron::ElectronMenuModel*)menuModel; +@end + +@implementation WeakPtrToElectronMenuModelAsNSObject { + base::WeakPtr _model; +} + ++ (instancetype)weakPtrForModel:(electron::ElectronMenuModel*)model { + return [[[WeakPtrToElectronMenuModelAsNSObject alloc] initWithModel:model] + autorelease]; +} + ++ (electron::ElectronMenuModel*)getFrom:(id)instance { + return + [base::mac::ObjCCastStrict(instance) + menuModel]; +} + +- (instancetype)initWithModel:(electron::ElectronMenuModel*)model { + if ((self = [super init])) { + _model = model->GetWeakPtr(); + } + return self; +} + +- (electron::ElectronMenuModel*)menuModel { + return _model.get(); +} + +@end + // Menu item is located for ease of removing it from the parent owner static base::scoped_nsobject recentDocumentsMenuItem_; @@ -95,12 +134,18 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; @implementation ElectronMenuController -@synthesize model = model_; +- (electron::ElectronMenuModel*)model { + return model_.get(); +} -- (id)initWithModel:(electron::ElectronMenuModel*)model - useDefaultAccelerator:(BOOL)use { +- (void)setModel:(electron::ElectronMenuModel*)model { + model_ = model->GetWeakPtr(); +} + +- (instancetype)initWithModel:(electron::ElectronMenuModel*)model + useDefaultAccelerator:(BOOL)use { if ((self = [super init])) { - model_ = model; + model_ = model->GetWeakPtr(); isMenuOpen_ = NO; useDefaultAccelerator_ = use; [self menu]; @@ -115,8 +160,7 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; // while its context menu is still open. [self cancel]; - model_ = nil; - + model_ = nullptr; [super dealloc]; } @@ -137,7 +181,7 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; itemWithTitle:@"Electron"] submenu] itemWithTitle:openTitle] retain]); } - model_ = model; + model_ = model->GetWeakPtr(); [menu_ removeAllItems]; const int count = model->GetItemCount(); @@ -153,7 +197,8 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; if (isMenuOpen_) { [menu_ cancelTracking]; isMenuOpen_ = NO; - model_->MenuWillClose(); + if (model_) + model_->MenuWillClose(); if (!closeCallback.is_null()) { base::PostTask(FROM_HERE, {BrowserThread::UI}, std::move(closeCallback)); } @@ -290,8 +335,8 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; // model. Setting the target to |self| allows this class to participate // in validation of the menu items. [item setTag:index]; - NSValue* modelObject = [NSValue valueWithPointer:model]; - [item setRepresentedObject:modelObject]; // Retains |modelObject|. + [item setRepresentedObject:[WeakPtrToElectronMenuModelAsNSObject + weakPtrForModel:model]]; ui::Accelerator accelerator; if (model->GetAcceleratorAtWithParams(index, useDefaultAccelerator_, &accelerator)) { @@ -331,9 +376,8 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; return NO; NSInteger modelIndex = [item tag]; - electron::ElectronMenuModel* model = - static_cast( - [[(id)item representedObject] pointerValue]); + electron::ElectronMenuModel* model = [WeakPtrToElectronMenuModelAsNSObject + getFrom:[(id)item representedObject]]; DCHECK(model); if (model) { BOOL checked = model->IsItemCheckedAt(modelIndex); @@ -352,8 +396,7 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; - (void)itemSelected:(id)sender { NSInteger modelIndex = [sender tag]; electron::ElectronMenuModel* model = - static_cast( - [[sender representedObject] pointerValue]); + [WeakPtrToElectronMenuModelAsNSObject getFrom:[sender representedObject]]; DCHECK(model); if (model) { NSEvent* event = [NSApp currentEvent]; @@ -369,7 +412,7 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; menu_.reset([[NSMenu alloc] initWithTitle:@""]); [menu_ setDelegate:self]; if (model_) - [self populateWithModel:model_]; + [self populateWithModel:model_.get()]; return menu_.get(); } @@ -379,7 +422,8 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; - (void)menuWillOpen:(NSMenu*)menu { isMenuOpen_ = YES; - model_->MenuWillShow(); + if (model_) + model_->MenuWillShow(); } - (void)menuDidClose:(NSMenu*)menu { diff --git a/shell/browser/ui/electron_menu_model.h b/shell/browser/ui/electron_menu_model.h index 4e02c72681e6..59db82712267 100644 --- a/shell/browser/ui/electron_menu_model.h +++ b/shell/browser/ui/electron_menu_model.h @@ -7,6 +7,7 @@ #include +#include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "base/observer_list_types.h" #include "ui/base/models/simple_menu_model.h" @@ -69,6 +70,10 @@ class ElectronMenuModel : public ui::SimpleMenuModel { void MenuWillClose() override; void MenuWillShow() override; + base::WeakPtr GetWeakPtr() { + return weak_factory_.GetWeakPtr(); + } + using SimpleMenuModel::GetSubmenuModelAt; ElectronMenuModel* GetSubmenuModelAt(int index); @@ -80,6 +85,8 @@ class ElectronMenuModel : public ui::SimpleMenuModel { std::map sublabels_; // command id -> sublabel base::ObserverList observers_; + base::WeakPtrFactory weak_factory_{this}; + DISALLOW_COPY_AND_ASSIGN(ElectronMenuModel); };