Fix the cyclic reference in menu delegate (#11967)

* Fix the cyclic reference in menu delegate

* Fix menu tests due to delegate change

atom/browser/api/atom_api_menu.cc

@@ -47,15 +47,21 @@ void Menu::AfterInit(v8::Isolate* isolate) {
 }
 
 bool Menu::IsCommandIdChecked(int command_id) const {
-  return is_checked_.Run(command_id);
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+  return is_checked_.Run(GetWrapper(), command_id);
 }
 
 bool Menu::IsCommandIdEnabled(int command_id) const {
-  return is_enabled_.Run(command_id);
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+  return is_enabled_.Run(GetWrapper(), command_id);
 }
 
 bool Menu::IsCommandIdVisible(int command_id) const {
-  return is_visible_.Run(command_id);
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+  return is_visible_.Run(GetWrapper(), command_id);
 }
 
 bool Menu::GetAcceleratorForCommandIdWithParams(
@@ -65,18 +71,23 @@ bool Menu::GetAcceleratorForCommandIdWithParams(
   v8::Locker locker(isolate());
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Value> val = get_accelerator_.Run(
-      command_id, use_default_accelerator);
+      GetWrapper(), command_id, use_default_accelerator);
   return mate::ConvertFromV8(isolate(), val, accelerator);
 }
 
 void Menu::ExecuteCommand(int command_id, int flags) {
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
   execute_command_.Run(
+      GetWrapper(),
       mate::internal::CreateEventFromFlags(isolate(), flags),
       command_id);
 }
 
 void Menu::MenuWillShow(ui::SimpleMenuModel* source) {
-  menu_will_show_.Run();
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+  menu_will_show_.Run(GetWrapper());
 }
 
 void Menu::InsertItemAt(

atom/browser/api/atom_api_menu.h

@@ -93,12 +93,14 @@ class Menu : public mate::TrackableObject<Menu>,
   bool IsVisibleAt(int index) const;
 
   // Stored delegate methods.
-  base::Callback<bool(int)> is_checked_;
-  base::Callback<bool(int)> is_enabled_;
-  base::Callback<bool(int)> is_visible_;
-  base::Callback<v8::Local<v8::Value>(int, bool)> get_accelerator_;
-  base::Callback<void(v8::Local<v8::Value>, int)> execute_command_;
-  base::Callback<void()> menu_will_show_;
+  base::Callback<bool(v8::Local<v8::Value>, int)> is_checked_;
+  base::Callback<bool(v8::Local<v8::Value>, int)> is_enabled_;
+  base::Callback<bool(v8::Local<v8::Value>, int)> is_visible_;
+  base::Callback<v8::Local<v8::Value>(v8::Local<v8::Value>, int, bool)>
+      get_accelerator_;
+  base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>, int)>
+      execute_command_;
+  base::Callback<void(v8::Local<v8::Value>)> menu_will_show_;
 
   DISALLOW_COPY_AND_ASSIGN(Menu);
 };

atom/browser/api/event_emitter.h

@@ -42,8 +42,10 @@ class EventEmitter : public Wrappable<T> {
 
   // Make the convinient methods visible:
   // https://isocpp.org/wiki/faq/templates#nondependent-name-lookup-members
-  v8::Local<v8::Object> GetWrapper() { return Wrappable<T>::GetWrapper(); }
   v8::Isolate* isolate() const { return Wrappable<T>::isolate(); }
+  v8::Local<v8::Object> GetWrapper() const {
+    return Wrappable<T>::GetWrapper();
+  }
 
   // this.emit(name, event, args...);
   template<typename... Args>

lib/browser/api/menu.js

@@ -11,36 +11,39 @@ let groupIdIndex = 0
 
 Object.setPrototypeOf(Menu.prototype, EventEmitter.prototype)
 
+// Menu Delegate.
+// This object should hold no reference to |Menu| to avoid cyclic reference.
+const delegate = {
+  isCommandIdChecked: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].checked : undefined,
+  isCommandIdEnabled: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].enabled : undefined,
+  isCommandIdVisible: (menu, id) => menu.commandsMap[id] ? menu.commandsMap[id].visible : undefined,
+  getAcceleratorForCommandId: (menu, id, useDefaultAccelerator) => {
+    const command = menu.commandsMap[id]
+    if (!command) return
+    if (command.accelerator) return command.accelerator
+    if (useDefaultAccelerator) return command.getDefaultRoleAccelerator()
+  },
+  executeCommand: (menu, event, id) => {
+    const command = menu.commandsMap[id]
+    if (!command) return
+    command.click(event, BrowserWindow.getFocusedWindow(), webContents.getFocusedWebContents())
+  },
+  menuWillShow: (menu) => {
+    // Ensure radio groups have at least one menu item seleted
+    for (const id in menu.groupsMap) {
+      const found = menu.groupsMap[id].find(item => item.checked) || null
+      if (!found) v8Util.setHiddenValue(menu.groupsMap[id][0], 'checked', true)
+    }
+  }
+}
+
 /* Instance Methods */
 
 Menu.prototype._init = function () {
   this.commandsMap = {}
   this.groupsMap = {}
   this.items = []
-  this.delegate = {
-    isCommandIdChecked: id => this.commandsMap[id] ? this.commandsMap[id].checked : undefined,
-    isCommandIdEnabled: id => this.commandsMap[id] ? this.commandsMap[id].enabled : undefined,
-    isCommandIdVisible: id => this.commandsMap[id] ? this.commandsMap[id].visible : undefined,
-    getAcceleratorForCommandId: (id, useDefaultAccelerator) => {
-      const command = this.commandsMap[id]
-      if (!command) return
-      if (command.accelerator) return command.accelerator
-      if (useDefaultAccelerator) return command.getDefaultRoleAccelerator()
-    },
-    getIconForCommandId: id => this.commandsMap[id] ? this.commandsMap[id].icon : undefined,
-    executeCommand: (event, id) => {
-      const command = this.commandsMap[id]
-      if (!command) return
-      command.click(event, BrowserWindow.getFocusedWindow(), webContents.getFocusedWebContents())
-    },
-    menuWillShow: () => {
-      // Ensure radio groups have at least one menu item seleted
-      for (const id in this.groupsMap) {
-        const found = this.groupsMap[id].find(item => item.checked) || null
-        if (!found) v8Util.setHiddenValue(this.groupsMap[id][0], 'checked', true)
-      }
-    }
-  }
+  this.delegate = delegate
 }
 
 Menu.prototype.popup = function (window, x, y, positioningItem) {
@@ -150,7 +153,7 @@ Menu.prototype.insert = function (pos, item) {
 }
 
 Menu.prototype._callMenuWillShow = function () {
-  if (this.delegate) this.delegate.menuWillShow()
+  if (this.delegate) this.delegate.menuWillShow(this)
   this.items.forEach(item => {
     if (item.submenu) item.submenu._callMenuWillShow()
   })

spec/api-menu-spec.js

@@ -430,7 +430,7 @@ describe('Menu module', () => {
           }
         }
       ])
-      menu.delegate.executeCommand({}, menu.items[0].commandId)
+      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId)
     })
   })
 
@@ -443,7 +443,7 @@ describe('Menu module', () => {
         }
       ])
       assert.equal(menu.items[0].checked, false)
-      menu.delegate.executeCommand({}, menu.items[0].commandId)
+      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
     })
 
@@ -454,9 +454,9 @@ describe('Menu module', () => {
           type: 'radio'
         }
       ])
-      menu.delegate.executeCommand({}, menu.items[0].commandId)
+      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
-      menu.delegate.executeCommand({}, menu.items[0].commandId)
+      menu.delegate.executeCommand(menu, {}, menu.items[0].commandId)
       assert.equal(menu.items[0].checked, true)
     })
 
@@ -476,7 +476,7 @@ describe('Menu module', () => {
         })
       }
       const menu = Menu.buildFromTemplate(template)
-      menu.delegate.menuWillShow()
+      menu.delegate.menuWillShow(menu)
       assert.equal(menu.items[0].checked, true)
       assert.equal(menu.items[12].checked, true)
     })

vendor/native_mate

@@ -1 +1 @@
-Subproject commit a38fb5df41be48fa98330f54033ff14f1558bac5
+Subproject commit 99d9e262eb36cb9dc8e83f61e026d2a7ad1e96ab