fix: weakly reference MenuModel from MenuController (#23806)

Co-authored-by: Shelley Vohr <[email protected]>

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 <NSMenuDelegate> {
  @protected
-  electron::ElectronMenuModel* model_;  // weak
+  base::WeakPtr<electron::ElectronMenuModel> model_;
   base::scoped_nsobject<NSMenu> 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;

shell/browser/ui/cocoa/electron_menu_controller.mm

@@ -8,6 +8,7 @@
 #include <utility>
 
 #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<electron::ElectronMenuModel> 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<electron::ElectronMenuModel> _model;
+}
+
++ (instancetype)weakPtrForModel:(electron::ElectronMenuModel*)model {
+  return [[[WeakPtrToElectronMenuModelAsNSObject alloc] initWithModel:model]
+      autorelease];
+}
+
++ (electron::ElectronMenuModel*)getFrom:(id)instance {
+  return
+      [base::mac::ObjCCastStrict<WeakPtrToElectronMenuModelAsNSObject>(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<NSMenuItem> recentDocumentsMenuItem_;
 
@@ -95,12 +134,18 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
 
 @implementation ElectronMenuController
 
-@synthesize model = model_;
+- (electron::ElectronMenuModel*)model {
+  return model_.get();
+}
+
+- (void)setModel:(electron::ElectronMenuModel*)model {
+  model_ = model->GetWeakPtr();
+}
 
-- (id)initWithModel:(electron::ElectronMenuModel*)model
-    useDefaultAccelerator:(BOOL)use {
+- (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<NSMenu> recentDocumentsMenuSwap_;
   // while its context menu is still open.
   [self cancel];
 
-  model_ = nil;
-
+  model_ = nullptr;
   [super dealloc];
 }
 
@@ -137,7 +181,7 @@ static base::scoped_nsobject<NSMenu> 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<NSMenu> 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<NSMenu> 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<NSMenu> recentDocumentsMenuSwap_;
     return NO;
 
   NSInteger modelIndex = [item tag];
-  electron::ElectronMenuModel* model =
-      static_cast<electron::ElectronMenuModel*>(
-          [[(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<NSMenu> recentDocumentsMenuSwap_;
 - (void)itemSelected:(id)sender {
   NSInteger modelIndex = [sender tag];
   electron::ElectronMenuModel* model =
-      static_cast<electron::ElectronMenuModel*>(
-          [[sender representedObject] pointerValue]);
+      [WeakPtrToElectronMenuModelAsNSObject getFrom:[sender representedObject]];
   DCHECK(model);
   if (model) {
     NSEvent* event = [NSApp currentEvent];
@@ -369,7 +412,7 @@ static base::scoped_nsobject<NSMenu> 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<NSMenu> recentDocumentsMenuSwap_;
 
 - (void)menuWillOpen:(NSMenu*)menu {
   isMenuOpen_ = YES;
-  model_->MenuWillShow();
+  if (model_)
+    model_->MenuWillShow();
 }
 
 - (void)menuDidClose:(NSMenu*)menu {

shell/browser/ui/electron_menu_model.h

@@ -7,6 +7,7 @@
 
 #include <map>
 
+#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<ElectronMenuModel> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
   using SimpleMenuModel::GetSubmenuModelAt;
   ElectronMenuModel* GetSubmenuModelAt(int index);
 
@@ -80,6 +85,8 @@ class ElectronMenuModel : public ui::SimpleMenuModel {
   std::map<int, base::string16> sublabels_;  // command id -> sublabel
   base::ObserverList<Observer> observers_;
 
+  base::WeakPtrFactory<ElectronMenuModel> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(ElectronMenuModel);
 };