cocoa: weakly reference MenuModel from MenuController

Before this CL, MenuController's contract required that the provided
MenuModel outlived a MenuController instance. As of 10.15, it's
impossible to fulfill that contract: AppKit seems to like keeping
MenuController instances (via [NSMenuItem target]) alive until the
next autorelease pool drain, and there's no way for the C++ code that
creates & manages a MenuModel to know when that will happen. See bug
998773 for more details about this change in 10.15.

As such, this CL has MenuController hold a WeakPtr to its MenuModel,
rather than a raw pointer, and handles the MenuModel being destroyed
before the MenuController.

One other possible approach would be to make MenuController own the
MenuModel, but that would be a very large change to the ownership of
MenuModel, and in particular would conflict with the fact that Views
(in the //ui/views sense) are often used as MenuModels but are owned
by their parent View.

Bug: 998835
Change-Id: Ieb3b8928a40d07edf1b9a4d394fb4adc0410fde5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1804538Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697297}

ui/base/cocoa/menu_controller.h

@@ -25,9 +25,6 @@ UI_BASE_EXPORT
 @interface MenuControllerCocoa
     : NSObject<NSMenuDelegate, NSUserInterfaceValidations>
 
-// The Model passed in to -initWithModel:.
-@property(nonatomic, assign) ui::MenuModel* model;
-
 // Whether to activate selected menu items via a posted task. This may allow the
 // selection to be handled earlier, whilst the menu is fading out. If the posted
 // task wasn't processed by the time the action is normally sent, it will be
@@ -54,6 +51,9 @@ UI_BASE_EXPORT
 // Programmatically close the constructed menu.
 - (void)cancel;
 
+- (ui::MenuModel*)model;
+- (void)setModel:(ui::MenuModel*)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;

ui/base/cocoa/menu_controller.mm

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/cancelable_callback.h"
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "ui/base/accelerators/accelerator.h"
@@ -44,6 +45,42 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 
 }  // namespace
 
+// This class stores a base::WeakPtr<ui::MenuModel> as an Objective-C object,
+// which allows it to be stored in the representedObject field of an NSMenuItem.
+@interface WeakPtrToMenuModelAsNSObject : NSObject
++ (instancetype)weakPtrForModel:(ui::MenuModel*)model;
++ (ui::MenuModel*)getFrom:(id)instance;
+- (instancetype)initWithModel:(ui::MenuModel*)model;
+- (ui::MenuModel*)menuModel;
+@end
+
+@implementation WeakPtrToMenuModelAsNSObject {
+  base::WeakPtr<ui::MenuModel> model_;
+}
+
++ (instancetype)weakPtrForModel:(ui::MenuModel*)model {
+  return
+      [[[WeakPtrToMenuModelAsNSObject alloc] initWithModel:model] autorelease];
+}
+
++ (ui::MenuModel*)getFrom:(id)instance {
+  return [base::mac::ObjCCastStrict<WeakPtrToMenuModelAsNSObject>(instance)
+      menuModel];
+}
+
+- (instancetype)initWithModel:(ui::MenuModel*)model {
+  if ((self = [super init])) {
+    model_ = model->AsWeakPtr();
+  }
+  return self;
+}
+
+- (ui::MenuModel*)menuModel {
+  return model_.get();
+}
+
+@end
+
 // Internal methods.
 @interface MenuControllerCocoa ()
 // Called before the menu is to be displayed to update the state (enabled,
@@ -83,7 +120,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 @end
 
 @implementation MenuControllerCocoa {
-  ui::MenuModel* model_;  // Weak.
+  base::WeakPtr<ui::MenuModel> model_;
   base::scoped_nsobject<NSMenu> menu_;
   BOOL useWithPopUpButtonCell_;  // If YES, 0th item is blank
   BOOL isMenuOpen_;
@@ -91,10 +128,17 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   std::unique_ptr<base::CancelableClosure> postedItemSelectedTask_;
 }
 
-@synthesize model = model_;
 @synthesize useWithPopUpButtonCell = useWithPopUpButtonCell_;
 @synthesize postItemSelectedAsTask = postItemSelectedAsTask_;
 
+- (ui::MenuModel*)model {
+  return model_.get();
+}
+
+- (void)setModel:(ui::MenuModel*)model {
+  model_ = model->AsWeakPtr();
+}
+
 - (instancetype)init {
   self = [super init];
   return self;
@@ -103,7 +147,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 - (instancetype)initWithModel:(ui::MenuModel*)model
        useWithPopUpButtonCell:(BOOL)useWithCell {
   if ((self = [super init])) {
-    model_ = model;
+    model_ = model->AsWeakPtr();
     useWithPopUpButtonCell_ = useWithCell;
     [self menu];
   }
@@ -117,14 +161,15 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
   // while its context menu is still open.
   [self cancel];
 
-  model_ = NULL;
+  model_ = nullptr;
   [super dealloc];
 }
 
 - (void)cancel {
   if (isMenuOpen_) {
     [menu_ cancelTracking];
-    model_->MenuWillClose();
+    if (model_)
+      model_->MenuWillClose();
     isMenuOpen_ = NO;
   }
 }
@@ -190,8 +235,8 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
     // in validation of the menu items.
     [item setTag:index];
     [item setTarget:self];
-    NSValue* modelObject = [NSValue valueWithPointer:model];
-    [item setRepresentedObject:modelObject];  // Retains |modelObject|.
+    [item setRepresentedObject:[WeakPtrToMenuModelAsNSObject
+                                   weakPtrForModel:model]];
     // On the Mac, context menus never have accelerators. Menus constructed
     // for context use have useWithPopUpButtonCell_ set to NO.
     if (useWithPopUpButtonCell_) {
@@ -216,8 +261,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 
   NSInteger modelIndex = [item tag];
   ui::MenuModel* model =
-      static_cast<ui::MenuModel*>(
-          [[(id)item representedObject] pointerValue]);
+      [WeakPtrToMenuModelAsNSObject getFrom:[(id)item representedObject]];
   DCHECK(model);
   if (model) {
     BOOL checked = model->IsItemCheckedAt(modelIndex);
@@ -303,8 +347,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 
   NSInteger modelIndex = [sender tag];
   ui::MenuModel* model =
-      static_cast<ui::MenuModel*>(
-          [[sender representedObject] pointerValue]);
+      [WeakPtrToMenuModelAsNSObject getFrom:[sender representedObject]];
   DCHECK(model);
   if (model)
     model->ActivatedAt(modelIndex, uiEventFlags);
@@ -313,7 +356,7 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 
 - (NSMenu*)menu {
   if (!menu_ && model_) {
-    menu_.reset([[self menuFromModel:model_] retain]);
+    menu_.reset([[self menuFromModel:model_.get()] retain]);
     [menu_ setDelegate:self];
     // If this is to be used with a NSPopUpButtonCell, add an item at the 0th
     // position that's empty. Doing it after the menu has been constructed won't
@@ -334,13 +377,15 @@ bool MenuHasVisibleItems(const ui::MenuModel* model) {
 
 - (void)menuWillOpen:(NSMenu*)menu {
   isMenuOpen_ = YES;
-  model_->MenuWillShow();  // Note: |model_| may trigger -[self dealloc].
+  if (model_)
+    model_->MenuWillShow();  // Note: |model_| may trigger -[self dealloc].
 }
 
 - (void)menuDidClose:(NSMenu*)menu {
   if (isMenuOpen_) {
     isMenuOpen_ = NO;
-    model_->MenuWillClose();  // Note: |model_| may trigger -[self dealloc].
+    if (model_)
+      model_->MenuWillClose();  // Note: |model_| may trigger -[self dealloc].
   }
 }
 

ui/base/cocoa/menu_controller_unittest.mm

@@ -281,7 +281,6 @@ TEST_F(MenuControllerTest, BasicCreation) {
   NSString* title = [itemTwo title];
   EXPECT_EQ(ASCIIToUTF16("three"), base::SysNSStringToUTF16(title));
   EXPECT_EQ(2, [itemTwo tag]);
-  EXPECT_EQ([[itemTwo representedObject] pointerValue], &model);
 
   EXPECT_TRUE([[[menu menu] itemAtIndex:3] isSeparatorItem]);
 }
@@ -315,7 +314,6 @@ TEST_F(MenuControllerTest, Submenus) {
   NSString* title = [submenuItem title];
   EXPECT_EQ(ASCIIToUTF16("sub-two"), base::SysNSStringToUTF16(title));
   EXPECT_EQ(1, [submenuItem tag]);
-  EXPECT_EQ([[submenuItem representedObject] pointerValue], &submodel);
 
   // Make sure the item after the submenu is correct and its represented
   // object is back to the top model.
@@ -323,7 +321,6 @@ TEST_F(MenuControllerTest, Submenus) {
   title = [item title];
   EXPECT_EQ(ASCIIToUTF16("three"), base::SysNSStringToUTF16(title));
   EXPECT_EQ(2, [item tag]);
-  EXPECT_EQ([[item representedObject] pointerValue], &model);
 }
 
 TEST_F(MenuControllerTest, EmptySubmenu) {

ui/base/models/menu_model.h

@@ -5,6 +5,7 @@
 #ifndef UI_BASE_MODELS_MENU_MODEL_H_
 #define UI_BASE_MODELS_MENU_MODEL_H_
 
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "ui/base/models/menu_model_delegate.h"
 #include "ui/base/models/menu_separator_types.h"
@@ -24,7 +25,7 @@ class Accelerator;
 class ButtonMenuItemModel;
 
 // An interface implemented by an object that provides the content of a menu.
-class UI_BASE_EXPORT MenuModel {
+class UI_BASE_EXPORT MenuModel : public base::SupportsWeakPtr<MenuModel> {
  public:
   // The type of item.
   enum ItemType {