fix: retain menu when popuping (#21227)

atom/browser/api/atom_api_menu.cc

@@ -4,10 +4,12 @@
 
 #include "atom/browser/api/atom_api_menu.h"
 
+#include <utility>
+
 #include "atom/browser/native_window.h"
 #include "atom/common/native_mate_converters/accelerator_converter.h"
-#include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/image_converter.h"
+#include "atom/common/native_mate_converters/once_callback.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "native_mate/constructor.h"
 #include "native_mate/dictionary.h"
@@ -189,6 +191,16 @@ void Menu::OnMenuWillShow() {
   Emit("menu-will-show");
 }
 
+base::OnceClosure Menu::BindSelfToClosure(base::OnceClosure callback) {
+  // return ((callback, ref) => { callback() }).bind(null, callback, this)
+  v8::Global<v8::Value> ref(isolate(), GetWrapper());
+  return base::BindOnce(
+      [](base::OnceClosure callback, v8::Global<v8::Value> ref) {
+        std::move(callback).Run();
+      },
+      std::move(callback), std::move(ref));
+}
+
 // static
 void Menu::BuildPrototype(v8::Isolate* isolate,
                           v8::Local<v8::FunctionTemplate> prototype) {

atom/browser/api/atom_api_menu.h

@@ -60,7 +60,7 @@ class Menu : public mate::TrackableObject<Menu>,
                        int x,
                        int y,
                        int positioning_item,
-                       const base::Closure& callback) = 0;
+                       base::OnceClosure callback) = 0;
   virtual void ClosePopupAt(int32_t window_id) = 0;
 
   std::unique_ptr<AtomMenuModel> model_;
@@ -70,6 +70,11 @@ class Menu : public mate::TrackableObject<Menu>,
   void OnMenuWillClose() override;
   void OnMenuWillShow() override;
 
+ protected:
+  // Returns a new callback which keeps references of the JS wrapper until the
+  // passed |callback| is called.
+  base::OnceClosure BindSelfToClosure(base::OnceClosure callback);
+
  private:
   void InsertItemAt(int index, int command_id, const base::string16& label);
   void InsertSeparatorAt(int index);

atom/browser/api/atom_api_menu_mac.h

@@ -27,19 +27,19 @@ class MenuMac : public Menu {
                int x,
                int y,
                int positioning_item,
-               const base::Closure& callback) override;
+               base::OnceClosure callback) override;
   void PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
                  int32_t window_id,
                  int x,
                  int y,
                  int positioning_item,
-                 base::Closure callback);
+                 base::OnceClosure callback);
   void ClosePopupAt(int32_t window_id) override;
 
  private:
   friend class Menu;
 
-  void OnClosed(int32_t window_id, base::Closure callback);
+  void OnClosed(int32_t window_id, base::OnceClosure callback);
 
   scoped_nsobject<AtomMenuController> menu_controller_;
 

atom/browser/api/atom_api_menu_mac.mm

@@ -37,15 +37,20 @@ void MenuMac::PopupAt(TopLevelWindow* window,
                       int x,
                       int y,
                       int positioning_item,
-                      const base::Closure& callback) {
+                      base::OnceClosure callback) {
   NativeWindow* native_window = window->window();
   if (!native_window)
     return;
 
-  auto popup = base::Bind(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(),
-                          native_window->GetWeakPtr(), window->weak_map_id(), x,
-                          y, positioning_item, callback);
-  base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI}, popup);
+  // Make sure the Menu object would not be garbage-collected until the callback
+  // has run.
+  base::OnceClosure callback_with_ref = BindSelfToClosure(std::move(callback));
+
+  auto popup =
+      base::BindOnce(&MenuMac::PopupOnUI, weak_factory_.GetWeakPtr(),
+                     native_window->GetWeakPtr(), window->weak_map_id(), x, y,
+                     positioning_item, std::move(callback_with_ref));
+  base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI}, std::move(popup));
 }
 
 void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
@@ -53,16 +58,14 @@ void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
                         int x,
                         int y,
                         int positioning_item,
-                        base::Closure callback) {
-  mate::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-
+                        base::OnceClosure callback) {
   if (!native_window)
     return;
   NSWindow* nswindow = native_window->GetNativeWindow().GetNativeNSWindow();
 
-  auto close_callback = base::Bind(
-      &MenuMac::OnClosed, weak_factory_.GetWeakPtr(), window_id, callback);
+  base::OnceClosure close_callback =
+      base::BindOnce(&MenuMac::OnClosed, weak_factory_.GetWeakPtr(), window_id,
+                     std::move(callback));
   popup_controllers_[window_id] = base::scoped_nsobject<AtomMenuController>(
       [[AtomMenuController alloc] initWithModel:model()
                           useDefaultAccelerator:NO]);
@@ -100,7 +103,7 @@ void MenuMac::PopupOnUI(const base::WeakPtr<NativeWindow>& native_window,
   if (rightmostMenuPoint > screenRight)
     position.x = position.x - [menu size].width;
 
-  [popup_controllers_[window_id] setCloseCallback:close_callback];
+  [popup_controllers_[window_id] setCloseCallback:std::move(close_callback)];
   // Make sure events can be pumped while the menu is up.
   base::MessageLoopCurrent::ScopedNestableTaskAllower allow;
 
@@ -131,9 +134,9 @@ void MenuMac::ClosePopupAt(int32_t window_id) {
   }
 }
 
-void MenuMac::OnClosed(int32_t window_id, base::Closure callback) {
+void MenuMac::OnClosed(int32_t window_id, base::OnceClosure callback) {
   popup_controllers_.erase(window_id);
-  callback.Run();
+  std::move(callback).Run();
 }
 
 // static

atom/browser/api/atom_api_menu_views.cc

@@ -5,6 +5,7 @@
 #include "atom/browser/api/atom_api_menu_views.h"
 
 #include <memory>
+#include <utility>
 
 #include "atom/browser/native_window_views.h"
 #include "atom/browser/unresponsive_suppressor.h"
@@ -25,10 +26,7 @@ void MenuViews::PopupAt(TopLevelWindow* window,
                         int x,
                         int y,
                         int positioning_item,
-                        const base::Closure& callback) {
-  mate::Locker locker(isolate());
-  v8::HandleScope handle_scope(isolate());
-
+                        base::OnceClosure callback) {
   auto* native_window = static_cast<NativeWindowViews*>(window->window());
   if (!native_window)
     return;
@@ -47,12 +45,21 @@ void MenuViews::PopupAt(TopLevelWindow* window,
   // Don't emit unresponsive event when showing menu.
   atom::UnresponsiveSuppressor suppressor;
 
+  // Make sure the Menu object would not be garbage-collected until the callback
+  // has run.
+  base::OnceClosure callback_with_ref = BindSelfToClosure(std::move(callback));
+
   // Show the menu.
+  //
+  // Note that while views::MenuRunner accepts RepeatingCallback as close
+  // callback, it is fine passing OnceCallback to it because we reset the
+  // menu runner immediately when the menu is closed.
   int32_t window_id = window->weak_map_id();
-  auto close_callback = base::Bind(
-      &MenuViews::OnClosed, weak_factory_.GetWeakPtr(), window_id, callback);
+  auto close_callback = base::AdaptCallbackForRepeating(
+      base::BindOnce(&MenuViews::OnClosed, weak_factory_.GetWeakPtr(),
+                     window_id, std::move(callback_with_ref)));
   menu_runners_[window_id] =
-      std::make_unique<MenuRunner>(model(), flags, close_callback);
+      std::make_unique<MenuRunner>(model(), flags, std::move(close_callback));
   menu_runners_[window_id]->RunMenuAt(
       native_window->widget(), NULL, gfx::Rect(location, gfx::Size()),
       views::MENU_ANCHOR_TOPLEFT, ui::MENU_SOURCE_MOUSE);
@@ -72,9 +79,9 @@ void MenuViews::ClosePopupAt(int32_t window_id) {
   }
 }
 
-void MenuViews::OnClosed(int32_t window_id, base::Closure callback) {
+void MenuViews::OnClosed(int32_t window_id, base::OnceClosure callback) {
   menu_runners_.erase(window_id);
-  callback.Run();
+  std::move(callback).Run();
 }
 
 // static

atom/browser/api/atom_api_menu_views.h

@@ -27,11 +27,11 @@ class MenuViews : public Menu {
                int x,
                int y,
                int positioning_item,
-               const base::Closure& callback) override;
+               base::OnceClosure callback) override;
   void ClosePopupAt(int32_t window_id) override;
 
  private:
-  void OnClosed(int32_t window_id, base::Closure callback);
+  void OnClosed(int32_t window_id, base::OnceClosure callback);
 
   // window ID -> open context menu
   std::map<int32_t, std::unique_ptr<views::MenuRunner>> menu_runners_;

atom/browser/api/atom_api_web_contents.cc

@@ -35,7 +35,6 @@
 #include "atom/common/color_util.h"
 #include "atom/common/mouse_util.h"
 #include "atom/common/native_mate_converters/blink_converter.h"
-#include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/content_converter.h"
 #include "atom/common/native_mate_converters/file_path_converter.h"
 #include "atom/common/native_mate_converters/gfx_converter.h"
@@ -43,6 +42,7 @@
 #include "atom/common/native_mate_converters/image_converter.h"
 #include "atom/common/native_mate_converters/net_converter.h"
 #include "atom/common/native_mate_converters/network_converter.h"
+#include "atom/common/native_mate_converters/once_callback.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/options_switches.h"

atom/browser/ui/cocoa/atom_menu_controller.h

@@ -28,7 +28,7 @@ class AtomMenuModel;
   base::scoped_nsobject<NSMenu> menu_;
   BOOL isMenuOpen_;
   BOOL useDefaultAccelerator_;
-  base::Callback<void()> closeCallback;
+  base::OnceClosure closeCallback;
 }
 
 @property(nonatomic, assign) atom::AtomMenuModel* model;
@@ -37,7 +37,7 @@ class AtomMenuModel;
 // to the contents of the model after calling this will not be noticed.
 - (id)initWithModel:(atom::AtomMenuModel*)model useDefaultAccelerator:(BOOL)use;
 
-- (void)setCloseCallback:(const base::Callback<void()>&)callback;
+- (void)setCloseCallback:(base::OnceClosure)callback;
 
 // Populate current NSMenu with |model|.
 - (void)populateWithModel:(atom::AtomMenuModel*)model;

atom/browser/ui/cocoa/atom_menu_controller.mm

@@ -117,8 +117,8 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
   [super dealloc];
 }
 
-- (void)setCloseCallback:(const base::Callback<void()>&)callback {
-  closeCallback = callback;
+- (void)setCloseCallback:(base::OnceClosure)callback {
+  closeCallback = std::move(callback);
 }
 
 - (void)populateWithModel:(atom::AtomMenuModel*)model {
@@ -150,7 +150,8 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
     isMenuOpen_ = NO;
     model_->MenuWillClose();
     if (!closeCallback.is_null()) {
-      base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI}, closeCallback);
+      base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI},
+                               std::move(closeCallback));
     }
   }
 }
@@ -371,7 +372,8 @@ static base::scoped_nsobject<NSMenu> recentDocumentsMenuSwap_;
     // Post async task so that itemSelected runs before the close callback
     // deletes the controller from the map which deallocates it
     if (!closeCallback.is_null()) {
-      base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI}, closeCallback);
+      base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI},
+                               std::move(closeCallback));
     }
   }
 }

atom/common/native_mate_converters/callback.h

@@ -138,20 +138,6 @@ struct NativeFunctionInvoker<ReturnType(ArgTypes...)> {
 
 }  // namespace internal
 
-template <typename Sig>
-struct Converter<base::OnceCallback<Sig>> {
-  static bool FromV8(v8::Isolate* isolate,
-                     v8::Local<v8::Value> val,
-                     base::OnceCallback<Sig>* out) {
-    if (!val->IsFunction())
-      return false;
-
-    *out = base::BindOnce(&internal::V8FunctionInvoker<Sig>::Go, isolate,
-                          internal::SafeV8Function(isolate, val));
-    return true;
-  }
-};
-
 template <typename Sig>
 struct Converter<base::RepeatingCallback<Sig>> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,

atom/common/native_mate_converters/once_callback.h

@@ -0,0 +1,87 @@
+// Copyright (c) 2019 GitHub, Inc. All rights reserved.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_COMMON_NATIVE_MATE_CONVERTERS_ONCE_CALLBACK_H_
+#define ATOM_COMMON_NATIVE_MATE_CONVERTERS_ONCE_CALLBACK_H_
+
+#include <utility>
+
+#include "atom/common/native_mate_converters/callback.h"
+
+namespace mate {
+
+namespace internal {
+
+// Manages the OnceCallback with ref-couting.
+template <typename Sig>
+class RefCountedOnceCallback
+    : public base::RefCounted<RefCountedOnceCallback<Sig>> {
+ public:
+  explicit RefCountedOnceCallback(base::OnceCallback<Sig> callback)
+      : callback_(std::move(callback)) {}
+
+  base::OnceCallback<Sig> GetCallback() { return std::move(callback_); }
+
+ private:
+  friend class base::RefCounted<RefCountedOnceCallback<Sig>>;
+  ~RefCountedOnceCallback() = default;
+
+  base::OnceCallback<Sig> callback_;
+};
+
+// Invokes the OnceCallback.
+template <typename Sig>
+struct InvokeOnceCallback {};
+
+template <typename... ArgTypes>
+struct InvokeOnceCallback<void(ArgTypes...)> {
+  static void Go(
+      scoped_refptr<RefCountedOnceCallback<void(ArgTypes...)>> holder,
+      ArgTypes... args) {
+    base::OnceCallback<void(ArgTypes...)> callback = holder->GetCallback();
+    DCHECK(!callback.is_null());
+    std::move(callback).Run(std::move(args)...);
+  }
+};
+
+template <typename ReturnType, typename... ArgTypes>
+struct InvokeOnceCallback<ReturnType(ArgTypes...)> {
+  static ReturnType Go(
+      scoped_refptr<RefCountedOnceCallback<ReturnType(ArgTypes...)>> holder,
+      ArgTypes... args) {
+    base::OnceCallback<void(ArgTypes...)> callback = holder->GetCallback();
+    DCHECK(!callback.is_null());
+    return std::move(callback).Run(std::move(args)...);
+  }
+};
+
+}  // namespace internal
+
+template <typename Sig>
+struct Converter<base::OnceCallback<Sig>> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   base::OnceCallback<Sig> val) {
+    // Reuse the converter of base::RepeatingCallback by storing the callback
+    // with a RefCounted.
+    auto holder = base::MakeRefCounted<internal::RefCountedOnceCallback<Sig>>(
+        std::move(val));
+    return Converter<base::RepeatingCallback<Sig>>::ToV8(
+        isolate,
+        base::BindRepeating(&internal::InvokeOnceCallback<Sig>::Go, holder));
+  }
+
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     base::OnceCallback<Sig>* out) {
+    if (!val->IsFunction())
+      return false;
+    *out = base::BindOnce(&internal::V8FunctionInvoker<Sig>::Go, isolate,
+                          internal::SafeV8Function(isolate, val));
+    return true;
+  }
+};
+
+}  // namespace mate
+
+#endif  // ATOM_COMMON_NATIVE_MATE_CONVERTERS_ONCE_CALLBACK_H_

filenames.gni

@@ -612,6 +612,7 @@ filenames = {
     "atom/common/native_mate_converters/net_converter.h",
     "atom/common/native_mate_converters/network_converter.cc",
     "atom/common/native_mate_converters/network_converter.h",
+    "atom/common/native_mate_converters/once_callback.h",
     "atom/common/native_mate_converters/string16_converter.h",
     "atom/common/native_mate_converters/ui_base_types_converter.h",
     "atom/common/native_mate_converters/v8_value_converter.cc",

native_mate/native_mate/function_template.h

@@ -5,6 +5,8 @@
 #ifndef NATIVE_MATE_FUNCTION_TEMPLATE_H_
 #define NATIVE_MATE_FUNCTION_TEMPLATE_H_
 
+#include <utility>
+
 #include "base/callback.h"
 #include "base/logging.h"
 #include "native_mate/arguments.h"
@@ -193,7 +195,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
   void DispatchToCallback(base::Callback<ReturnType(ArgTypes...)> callback) {
     v8::MicrotasksScope script_scope(
         args_->isolate(), v8::MicrotasksScope::kRunMicrotasks);
-    args_->Return(callback.Run(ArgumentHolder<indices, ArgTypes>::value...));
+    args_->Return(callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...));
   }
 
   // In C++, you can declare the function foo(void), but you can't pass a void
@@ -202,7 +204,7 @@ class Invoker<IndicesHolder<indices...>, ArgTypes...>
   void DispatchToCallback(base::Callback<void(ArgTypes...)> callback) {
     v8::MicrotasksScope script_scope(
         args_->isolate(), v8::MicrotasksScope::kRunMicrotasks);
-    callback.Run(ArgumentHolder<indices, ArgTypes>::value...);
+    callback.Run(std::move(ArgumentHolder<indices, ArgTypes>::value)...);
   }
 
  private: