From 50f2d2b5abfa1124dd49f41de8251952bc946307 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Wed, 20 Nov 2019 20:17:39 +0900 Subject: [PATCH] fix: menu should not be garbage-collected when popuping (#21169) * fix: retain menu when popuping * test: menu should not be garbage-collected when popuping --- shell/browser/api/atom_api_menu.cc | 11 ++++++++ shell/browser/api/atom_api_menu.h | 8 ++++-- shell/browser/api/atom_api_menu_mac.h | 6 ++--- shell/browser/api/atom_api_menu_mac.mm | 24 ++++++++++-------- shell/browser/api/atom_api_menu_views.cc | 25 ++++++++++++------- shell/browser/api/atom_api_menu_views.h | 4 +-- shell/browser/ui/cocoa/atom_menu_controller.h | 4 +-- .../browser/ui/cocoa/atom_menu_controller.mm | 8 +++--- spec-main/api-menu-spec.ts | 25 +++++++++++++++++++ 9 files changed, 82 insertions(+), 33 deletions(-) diff --git a/shell/browser/api/atom_api_menu.cc b/shell/browser/api/atom_api_menu.cc index 2199dbeec96c..75bb8848bbba 100644 --- a/shell/browser/api/atom_api_menu.cc +++ b/shell/browser/api/atom_api_menu.cc @@ -5,6 +5,7 @@ #include "shell/browser/api/atom_api_menu.h" #include +#include #include "shell/browser/native_window.h" #include "shell/common/gin_converters/accelerator_converter.h" @@ -217,6 +218,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 ref(isolate(), GetWrapper()); + return base::BindOnce( + [](base::OnceClosure callback, v8::Global ref) { + std::move(callback).Run(); + }, + std::move(callback), std::move(ref)); +} + // static void Menu::BuildPrototype(v8::Isolate* isolate, v8::Local prototype) { diff --git a/shell/browser/api/atom_api_menu.h b/shell/browser/api/atom_api_menu.h index 67e97aeafe2c..82e8a4161f6b 100644 --- a/shell/browser/api/atom_api_menu.h +++ b/shell/browser/api/atom_api_menu.h @@ -12,7 +12,6 @@ #include "gin/arguments.h" #include "shell/browser/api/atom_api_top_level_window.h" #include "shell/browser/ui/atom_menu_model.h" -#include "shell/common/gin_helper/locker.h" #include "shell/common/gin_helper/trackable_object.h" namespace electron { @@ -62,7 +61,7 @@ class Menu : public gin_helper::TrackableObject, 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 model_; @@ -72,6 +71,11 @@ class Menu : public gin_helper::TrackableObject, 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); diff --git a/shell/browser/api/atom_api_menu_mac.h b/shell/browser/api/atom_api_menu_mac.h index db7d9f4d2784..a3aee7445976 100644 --- a/shell/browser/api/atom_api_menu_mac.h +++ b/shell/browser/api/atom_api_menu_mac.h @@ -27,20 +27,20 @@ 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& 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; void ClosePopupOnUI(int32_t window_id); private: friend class Menu; - void OnClosed(int32_t window_id, base::Closure callback); + void OnClosed(int32_t window_id, base::OnceClosure callback); scoped_nsobject menu_controller_; diff --git a/shell/browser/api/atom_api_menu_mac.mm b/shell/browser/api/atom_api_menu_mac.mm index 7151c97f4990..5452f14476d6 100644 --- a/shell/browser/api/atom_api_menu_mac.mm +++ b/shell/browser/api/atom_api_menu_mac.mm @@ -38,15 +38,19 @@ 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; + // 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, callback); + positioning_item, std::move(callback_with_ref)); base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(popup)); } @@ -55,16 +59,14 @@ void MenuMac::PopupOnUI(const base::WeakPtr& native_window, int x, int y, int positioning_item, - base::Closure callback) { - gin_helper::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::BindRepeating( - &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 alloc] initWithModel:model() useDefaultAccelerator:NO]); @@ -102,7 +104,7 @@ void MenuMac::PopupOnUI(const base::WeakPtr& 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; @@ -140,9 +142,9 @@ void MenuMac::ClosePopupOnUI(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 diff --git a/shell/browser/api/atom_api_menu_views.cc b/shell/browser/api/atom_api_menu_views.cc index 6d71cafa0741..4f82def8dd1a 100644 --- a/shell/browser/api/atom_api_menu_views.cc +++ b/shell/browser/api/atom_api_menu_views.cc @@ -5,6 +5,7 @@ #include "shell/browser/api/atom_api_menu_views.h" #include +#include #include "shell/browser/native_window_views.h" #include "shell/browser/unresponsive_suppressor.h" @@ -24,10 +25,7 @@ void MenuViews::PopupAt(TopLevelWindow* window, int x, int y, int positioning_item, - const base::Closure& callback) { - gin_helper::Locker locker(isolate()); - v8::HandleScope handle_scope(isolate()); - + base::OnceClosure callback) { auto* native_window = static_cast(window->window()); if (!native_window) return; @@ -46,12 +44,21 @@ void MenuViews::PopupAt(TopLevelWindow* window, // Don't emit unresponsive event when showing menu. electron::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::BindRepeating( - &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(model(), flags, close_callback); + std::make_unique(model(), flags, std::move(close_callback)); menu_runners_[window_id]->RunMenuAt( native_window->widget(), nullptr, gfx::Rect(location, gfx::Size()), views::MenuAnchorPosition::kTopLeft, ui::MENU_SOURCE_MOUSE); @@ -71,9 +78,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 diff --git a/shell/browser/api/atom_api_menu_views.h b/shell/browser/api/atom_api_menu_views.h index f2e4c28103f5..e32bc8850b2d 100644 --- a/shell/browser/api/atom_api_menu_views.h +++ b/shell/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> menu_runners_; diff --git a/shell/browser/ui/cocoa/atom_menu_controller.h b/shell/browser/ui/cocoa/atom_menu_controller.h index e47402b97a52..7e3b9f181290 100644 --- a/shell/browser/ui/cocoa/atom_menu_controller.h +++ b/shell/browser/ui/cocoa/atom_menu_controller.h @@ -28,7 +28,7 @@ class AtomMenuModel; base::scoped_nsobject menu_; BOOL isMenuOpen_; BOOL useDefaultAccelerator_; - base::Callback closeCallback; + base::OnceClosure closeCallback; } @property(nonatomic, assign) electron::AtomMenuModel* model; @@ -38,7 +38,7 @@ class AtomMenuModel; - (id)initWithModel:(electron::AtomMenuModel*)model useDefaultAccelerator:(BOOL)use; -- (void)setCloseCallback:(const base::Callback&)callback; +- (void)setCloseCallback:(base::OnceClosure)callback; // Populate current NSMenu with |model|. - (void)populateWithModel:(electron::AtomMenuModel*)model; diff --git a/shell/browser/ui/cocoa/atom_menu_controller.mm b/shell/browser/ui/cocoa/atom_menu_controller.mm index d1e9484d0358..ade72786b797 100644 --- a/shell/browser/ui/cocoa/atom_menu_controller.mm +++ b/shell/browser/ui/cocoa/atom_menu_controller.mm @@ -118,8 +118,8 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; [super dealloc]; } -- (void)setCloseCallback:(const base::Callback&)callback { - closeCallback = callback; +- (void)setCloseCallback:(base::OnceClosure)callback { + closeCallback = std::move(callback); } - (void)populateWithModel:(electron::AtomMenuModel*)model { @@ -153,7 +153,7 @@ static base::scoped_nsobject recentDocumentsMenuSwap_; isMenuOpen_ = NO; model_->MenuWillClose(); if (!closeCallback.is_null()) { - base::PostTask(FROM_HERE, {BrowserThread::UI}, closeCallback); + base::PostTask(FROM_HERE, {BrowserThread::UI}, std::move(closeCallback)); } } } @@ -385,7 +385,7 @@ static base::scoped_nsobject 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::PostTask(FROM_HERE, {BrowserThread::UI}, closeCallback); + base::PostTask(FROM_HERE, {BrowserThread::UI}, std::move(closeCallback)); } } } diff --git a/spec-main/api-menu-spec.ts b/spec-main/api-menu-spec.ts index 584d4b02afa0..1dafc6d1a1a7 100644 --- a/spec-main/api-menu-spec.ts +++ b/spec-main/api-menu-spec.ts @@ -822,6 +822,31 @@ describe('Menu module', function () { menu.closePopup() }) }) + + it('prevents menu from getting garbage-collected when popuping', (done) => { + const menu = Menu.buildFromTemplate([{ role: 'paste' }]) + menu.popup({ window: w }) + + // Keep a weak reference to the menu. + const v8Util = process.electronBinding('v8_util') + const map = (v8Util as any).createIDWeakMap() as any + map.set(0, menu) + + setTimeout(() => { + // Do garbage collection, since |menu| is not referenced in this closure + // it would be gone after next call. + v8Util.requestGarbageCollectionForTesting() + setTimeout(() => { + // Try to receive menu from weak reference. + if (map.has(0)) { + map.get(0).closePopup() + done() + } else { + done('Menu is garbage-collected while popuping') + } + }) + }) + }) }) describe('Menu.setApplicationMenu', () => {