mirrors/electron
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: menu should not be garbage-collected when popuping (#21169)

Browse source 
* fix: retain menu when popuping

* test: menu should not be garbage-collected when popuping
This commit is contained in:
Cheng Zhao 2019-11-20 20:17:39 +09:00 committed by GitHub
parent ea23f18e94
commit 50f2d2b5ab
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 82 additions and 33 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
shell/browser/api/atom_api_menu.cc
View file

@ -5,6 +5,7 @@
#include "shell/browser/api/atom_api_menu.h"
#include <map>
#include <utility>
#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<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) {

8
shell/browser/api/atom_api_menu.h
View file

@ -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<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_;
@ -72,6 +71,11 @@ class Menu : public gin_helper::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);

6
shell/browser/api/atom_api_menu_mac.h
View file

@ -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<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;
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<AtomMenuController> menu_controller_;

24
shell/browser/api/atom_api_menu_mac.mm
View file

@ -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<NativeWindow>& 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>(
[[AtomMenuController alloc] initWithModel:model()
useDefaultAccelerator:NO]);
@ -102,7 +104,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;
@ -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

25
shell/browser/api/atom_api_menu_views.cc
View file

@ -5,6 +5,7 @@
#include "shell/browser/api/atom_api_menu_views.h"
#include <memory>
#include <utility>
#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<NativeWindowViews*>(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<MenuRunner>(model(), flags, close_callback);
std::make_unique<MenuRunner>(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

4
shell/browser/api/atom_api_menu_views.h
View file

@ -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_;

4
shell/browser/ui/cocoa/atom_menu_controller.h
View file

@ -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) electron::AtomMenuModel* model;
@ -38,7 +38,7 @@ class AtomMenuModel;
- (id)initWithModel:(electron::AtomMenuModel*)model
useDefaultAccelerator:(BOOL)use;
- (void)setCloseCallback:(const base::Callback<void()>&)callback;
- (void)setCloseCallback:(base::OnceClosure)callback;
// Populate current NSMenu with |model|.
- (void)populateWithModel:(electron::AtomMenuModel*)model;

8
shell/browser/ui/cocoa/atom_menu_controller.mm
View file

@ -118,8 +118,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:(electron::AtomMenuModel*)model {
@ -153,7 +153,7 @@ static base::scoped_nsobject<NSMenu> 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<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::PostTask(FROM_HERE, {BrowserThread::UI}, closeCallback);
base::PostTask(FROM_HERE, {BrowserThread::UI}, std::move(closeCallback));
}
}
}

25
spec-main/api-menu-spec.ts
View file

@ -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', () => {