From 53bf94fdf4f46faaa91a669c7222334b73838dee Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Tue, 14 Apr 2026 14:52:13 -0500 Subject: [PATCH] refactor: move `electron::api::GlobalShortcut` to cppgc (#50192) * refactor: migrate electron::api::GlobalShortcut to cppgc * refactor: lazy-create electron::api::GlobalShortcut copy the lazy-create idom used by electron::api::Screen * refactor: use gin::WeakCellFactory in GlobalCallbacks * fix: make a copy of `callback` before running it safeguard against the callback changing the map, invalidating `cb` * chore: reduce unnecessary diffs with main * fixup! refactor: use gin::WeakCellFactory in GlobalCallbacks fix: must Trace() the weak cell factory * fix: destruction order - Setup isolate dispose observer to run destruction sequences and remove self persistent reference - Skip NOTREACHED check during destruction, it can happen as a result of plaform listeners scheduling callbacks when Unregister is invoked. - Fix the order of unregistration in GlobalShortcut::Unregister - Add GlobalShortcut::UnregisterAllInternal to avoid any callsites that can re-enter V8 * fix: crash during gc from incorrect cppgc object headers * chore: update patches * chore: cleanup * chore: fix lint --------- Co-authored-by: deepak1556 --- lib/browser/api/global-shortcut.ts | 41 ++++++- .../api/electron_api_global_shortcut.cc | 104 +++++++++++++----- .../api/electron_api_global_shortcut.h | 43 +++++--- .../gin_helper/wrappable_pointer_tags.h | 1 + typings/internal-ambient.d.ts | 2 +- 5 files changed, 140 insertions(+), 51 deletions(-) diff --git a/lib/browser/api/global-shortcut.ts b/lib/browser/api/global-shortcut.ts index b5efaa7240..48c0751422 100644 --- a/lib/browser/api/global-shortcut.ts +++ b/lib/browser/api/global-shortcut.ts @@ -1,2 +1,39 @@ -const { globalShortcut } = process._linkedBinding('electron_browser_global_shortcut'); -export default globalShortcut; +const { createGlobalShortcut } = process._linkedBinding('electron_browser_global_shortcut'); + +let globalShortcut: Electron.GlobalShortcut; + +const createGlobalShortcutIfNeeded = () => { + if (globalShortcut === undefined) { + globalShortcut = createGlobalShortcut(); + } +}; + +export default new Proxy( + {}, + { + get: (_target, property: keyof Electron.GlobalShortcut) => { + createGlobalShortcutIfNeeded(); + const value = globalShortcut[property]; + if (typeof value === 'function') { + return value.bind(globalShortcut); + } + return value; + }, + set: (_target, property: string, value: unknown) => { + createGlobalShortcutIfNeeded(); + return Reflect.set(globalShortcut, property, value); + }, + ownKeys: () => { + createGlobalShortcutIfNeeded(); + return Reflect.ownKeys(globalShortcut); + }, + has: (_target, property: string) => { + createGlobalShortcutIfNeeded(); + return property in globalShortcut; + }, + getOwnPropertyDescriptor: (_target, property: string) => { + createGlobalShortcutIfNeeded(); + return Reflect.getOwnPropertyDescriptor(globalShortcut, property); + } + } +); diff --git a/shell/browser/api/electron_api_global_shortcut.cc b/shell/browser/api/electron_api_global_shortcut.cc index e2ccf35ae7..ecc6d9560b 100644 --- a/shell/browser/api/electron_api_global_shortcut.cc +++ b/shell/browser/api/electron_api_global_shortcut.cc @@ -15,14 +15,17 @@ #include "electron/shell/browser/electron_browser_context.h" #include "electron/shell/common/electron_constants.h" #include "extensions/common/command.h" -#include "gin/dictionary.h" #include "gin/object_template_builder.h" +#include "gin/persistent.h" #include "shell/browser/api/electron_api_system_preferences.h" #include "shell/browser/browser.h" #include "shell/common/gin_converters/accelerator_converter.h" #include "shell/common/gin_converters/callback_converter.h" -#include "shell/common/gin_helper/handle.h" +#include "shell/common/gin_helper/dictionary.h" +#include "shell/common/gin_helper/wrappable_pointer_tags.h" #include "shell/common/node_includes.h" +#include "v8/include/cppgc/allocation.h" +#include "v8/include/v8-cppgc.h" #if BUILDFLAG(IS_MAC) #include "base/mac/mac_util.h" @@ -50,21 +53,28 @@ bool MapHasMediaKeys( namespace electron::api { -gin::DeprecatedWrapperInfo GlobalShortcut::kWrapperInfo = { - gin::kEmbedderNativeGin}; +const gin::WrapperInfo GlobalShortcut::kWrapperInfo = + electron::MakeWrapperInfo(electron::kElectronGlobalShortcut); -GlobalShortcut::GlobalShortcut() = default; +GlobalShortcut::GlobalShortcut(v8::Isolate* isolate) { + gin::PerIsolateData* data = gin::PerIsolateData::From(isolate); + data->AddDisposeObserver(this); +} + +GlobalShortcut::~GlobalShortcut() = default; + +void GlobalShortcut::Dispose() { + is_disposed_ = true; -GlobalShortcut::~GlobalShortcut() { auto* instance = ui::GlobalAcceleratorListener::GetInstance(); if (instance && instance->IsRegistrationHandledExternally()) { // Eagerly cancel callbacks so PruneStaleCommands() can clear them before // the WeakPtrFactory destructor runs. - weak_ptr_factory_.InvalidateWeakPtrs(); + weak_factory_.Invalidate(); instance->PruneStaleCommands(); } - UnregisterAll(); + UnregisterAllInternal(); } void GlobalShortcut::OnKeyPressed(const ui::Accelerator& accelerator) { @@ -73,7 +83,9 @@ void GlobalShortcut::OnKeyPressed(const ui::Accelerator& accelerator) { } else { // This should never occur, because if it does, // ui::GlobalAcceleratorListener notifies us with wrong accelerator. - NOTREACHED(); + if (!is_disposed_) { + NOTREACHED(); + } } } @@ -84,7 +96,9 @@ void GlobalShortcut::ExecuteCommand(const extensions::ExtensionId& extension_id, } else { // This should never occur, because if it does, GlobalAcceleratorListener // notifies us with wrong command. - NOTREACHED(); + if (!is_disposed_) { + NOTREACHED(); + } } } @@ -112,11 +126,14 @@ bool GlobalShortcut::RegisterAll( bool GlobalShortcut::Register(const ui::Accelerator& accelerator, const base::RepeatingClosure& callback) { + v8::Isolate* const isolate = JavascriptEnvironment::GetIsolate(); + if (!electron::Browser::Get()->is_ready()) { - gin_helper::ErrorThrower(JavascriptEnvironment::GetIsolate()) - .ThrowError("globalShortcut cannot be used before the app is ready"); + gin_helper::ErrorThrower(isolate).ThrowError( + "globalShortcut cannot be used before the app is ready"); return false; } + #if BUILDFLAG(IS_MAC) if (accelerator.IsMediaKey()) { if (RegisteringMediaKeyForUntrustedClient(accelerator)) @@ -170,8 +187,10 @@ bool GlobalShortcut::Register(const ui::Accelerator& accelerator, const std::string fake_extension_id = command_str + "+" + profile_id; instance->OnCommandsChanged( fake_extension_id, profile_id, commands, gfx::kNullAcceleratedWidget, - base::BindRepeating(&GlobalShortcut::ExecuteCommand, - weak_ptr_factory_.GetWeakPtr())); + base::BindRepeating( + &GlobalShortcut::ExecuteCommand, + gin::WrapPersistent(weak_factory_.GetWeakCell( + isolate->GetCppHeap()->GetAllocationHandle())))); command_callback_map_[command_str] = callback; return true; } else { @@ -189,19 +208,24 @@ void GlobalShortcut::Unregister(const ui::Accelerator& accelerator) { .ThrowError("globalShortcut cannot be used before the app is ready"); return; } - if (accelerator_callback_map_.erase(accelerator) == 0) + if (!accelerator_callback_map_.contains(accelerator)) return; + if (ui::GlobalAcceleratorListener::GetInstance()) { + ui::GlobalAcceleratorListener::GetInstance()->UnregisterAccelerator( + accelerator, this); + } + + // Remove from local callback map after unregistering from UI listener to + // avoid reentrancy races where in-flight key notifications arrive while the + // platform listener is stopping. + accelerator_callback_map_.erase(accelerator); + #if BUILDFLAG(IS_MAC) if (accelerator.IsMediaKey() && !MapHasMediaKeys(accelerator_callback_map_)) { ui::GlobalAcceleratorListener::SetShouldUseInternalMediaKeyHandling(true); } #endif - - if (ui::GlobalAcceleratorListener::GetInstance()) { - ui::GlobalAcceleratorListener::GetInstance()->UnregisterAccelerator( - accelerator, this); - } } void GlobalShortcut::UnregisterSome( @@ -226,10 +250,15 @@ void GlobalShortcut::UnregisterAll() { .ThrowError("globalShortcut cannot be used before the app is ready"); return; } - accelerator_callback_map_.clear(); + UnregisterAllInternal(); +} + +void GlobalShortcut::UnregisterAllInternal() { if (ui::GlobalAcceleratorListener::GetInstance()) { ui::GlobalAcceleratorListener::GetInstance()->UnregisterAccelerators(this); } + accelerator_callback_map_.clear(); + command_callback_map_.clear(); } void GlobalShortcut::SetSuspended(bool suspend) { @@ -257,16 +286,15 @@ bool GlobalShortcut::IsSuspended() { } // static -gin_helper::Handle GlobalShortcut::Create( - v8::Isolate* isolate) { - return gin_helper::CreateHandle(isolate, new GlobalShortcut()); +GlobalShortcut* GlobalShortcut::Create(v8::Isolate* isolate) { + return cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), isolate); } // static gin::ObjectTemplateBuilder GlobalShortcut::GetObjectTemplateBuilder( v8::Isolate* isolate) { - return gin_helper::DeprecatedWrappable< - GlobalShortcut>::GetObjectTemplateBuilder(isolate) + return gin::Wrappable::GetObjectTemplateBuilder(isolate) .SetMethod("registerAll", &GlobalShortcut::RegisterAll) .SetMethod("register", &GlobalShortcut::Register) .SetMethod("isRegistered", &GlobalShortcut::IsRegistered) @@ -276,8 +304,23 @@ gin::ObjectTemplateBuilder GlobalShortcut::GetObjectTemplateBuilder( .SetMethod("isSuspended", &GlobalShortcut::IsSuspended); } -const char* GlobalShortcut::GetTypeName() { - return "GlobalShortcut"; +void GlobalShortcut::Trace(cppgc::Visitor* visitor) const { + gin::Wrappable::Trace(visitor); + visitor->Trace(weak_factory_); +} + +const gin::WrapperInfo* GlobalShortcut::wrapper_info() const { + return &kWrapperInfo; +} + +const char* GlobalShortcut::GetHumanReadableName() const { + return "Electron / GlobalShortcut"; +} + +void GlobalShortcut::OnBeforeMicrotasksRunnerDispose(v8::Isolate* isolate) { + gin::PerIsolateData* data = gin::PerIsolateData::From(isolate); + data->RemoveDisposeObserver(this); + Dispose(); } } // namespace electron::api @@ -289,8 +332,9 @@ void Initialize(v8::Local exports, v8::Local context, void* priv) { v8::Isolate* const isolate = electron::JavascriptEnvironment::GetIsolate(); - gin::Dictionary dict{isolate, exports}; - dict.Set("globalShortcut", electron::api::GlobalShortcut::Create(isolate)); + gin_helper::Dictionary dict{isolate, exports}; + dict.SetMethod("createGlobalShortcut", + base::BindRepeating(&electron::api::GlobalShortcut::Create)); } } // namespace diff --git a/shell/browser/api/electron_api_global_shortcut.h b/shell/browser/api/electron_api_global_shortcut.h index b7bff4c2e4..b4584b27ce 100644 --- a/shell/browser/api/electron_api_global_shortcut.h +++ b/shell/browser/api/electron_api_global_shortcut.h @@ -11,42 +11,47 @@ #include "base/functional/callback_forward.h" #include "base/memory/weak_ptr.h" #include "extensions/common/extension_id.h" -#include "shell/common/gin_helper/wrappable.h" +#include "gin/per_isolate_data.h" +#include "gin/weak_cell.h" +#include "gin/wrappable.h" #include "ui/base/accelerators/accelerator.h" #include "ui/base/accelerators/global_accelerator_listener/global_accelerator_listener.h" -namespace gin_helper { -template -class Handle; -} // namespace gin_helper - namespace electron::api { -class GlobalShortcut final - : private ui::GlobalAcceleratorListener::Observer, - public gin_helper::DeprecatedWrappable { +class GlobalShortcut final : public gin::Wrappable, + private ui::GlobalAcceleratorListener::Observer, + public gin::PerIsolateData::DisposeObserver { public: - static gin_helper::Handle Create(v8::Isolate* isolate); + static GlobalShortcut* Create(v8::Isolate* isolate); - // gin_helper::Wrappable - static gin::DeprecatedWrapperInfo kWrapperInfo; + // gin::Wrappable + static const gin::WrapperInfo kWrapperInfo; + void Trace(cppgc::Visitor* visitor) const override; + const gin::WrapperInfo* wrapper_info() const override; + const char* GetHumanReadableName() const override; gin::ObjectTemplateBuilder GetObjectTemplateBuilder( v8::Isolate* isolate) override; - const char* GetTypeName() override; + + // gin::PerIsolateData::DisposeObserver + void OnBeforeDispose(v8::Isolate* isolate) override {} + void OnBeforeMicrotasksRunnerDispose(v8::Isolate* isolate) override; + void OnDisposed() override {} + + // Make public for cppgc::MakeGarbageCollected. + explicit GlobalShortcut(v8::Isolate* isolate); + ~GlobalShortcut() override; // disable copy GlobalShortcut(const GlobalShortcut&) = delete; GlobalShortcut& operator=(const GlobalShortcut&) = delete; - protected: - GlobalShortcut(); - ~GlobalShortcut() override; - private: typedef std::map AcceleratorCallbackMap; typedef std::map CommandCallbackMap; + void Dispose(); bool RegisterAll(const std::vector& accelerators, const base::RepeatingClosure& callback); bool Register(const ui::Accelerator& accelerator, @@ -55,6 +60,7 @@ class GlobalShortcut final void Unregister(const ui::Accelerator& accelerator); void UnregisterSome(const std::vector& accelerators); void UnregisterAll(); + void UnregisterAllInternal(); void SetSuspended(bool suspend); bool IsSuspended(); @@ -65,8 +71,9 @@ class GlobalShortcut final AcceleratorCallbackMap accelerator_callback_map_; CommandCallbackMap command_callback_map_; + bool is_disposed_ = false; - base::WeakPtrFactory weak_ptr_factory_{this}; + gin::WeakCellFactory weak_factory_{this}; }; } // namespace electron::api diff --git a/shell/common/gin_helper/wrappable_pointer_tags.h b/shell/common/gin_helper/wrappable_pointer_tags.h index e7ba3d93c6..edb08777ac 100644 --- a/shell/common/gin_helper/wrappable_pointer_tags.h +++ b/shell/common/gin_helper/wrappable_pointer_tags.h @@ -16,6 +16,7 @@ enum ElectronWrappablePointerTag : uint16_t { kElectronDataPipeHolder, // electron::api::DataPipeHolder kElectronDebugger, // electron::api::Debugger kElectronEvent, // gin_helper::internal::Event + kElectronGlobalShortcut, // electron::api::GlobalShortcut kElectronExtensions, // electron::api::Extensions kElectronMenu, // electron::api::Menu kElectronNetLog, // electron::api::NetLog diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index 133e4f11df..4ffe77e3d5 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -242,7 +242,7 @@ declare namespace NodeJS { _linkedBinding(name: 'electron_browser_crash_reporter'): CrashReporterBinding; _linkedBinding(name: 'electron_browser_desktop_capturer'): { createDesktopCapturer(): ElectronInternal.DesktopCapturer; isDisplayMediaSystemPickerAvailable(): boolean; }; _linkedBinding(name: 'electron_browser_event_emitter'): { setEventEmitterPrototype(prototype: Object): void; }; - _linkedBinding(name: 'electron_browser_global_shortcut'): { globalShortcut: Electron.GlobalShortcut }; + _linkedBinding(name: 'electron_browser_global_shortcut'): { createGlobalShortcut(): Electron.GlobalShortcut }; _linkedBinding(name: 'electron_browser_image_view'): { ImageView: any }; _linkedBinding(name: 'electron_browser_in_app_purchase'): { inAppPurchase: Electron.InAppPurchase }; _linkedBinding(name: 'electron_browser_message_port'): { createPair(): { port1: Electron.MessagePortMain, port2: Electron.MessagePortMain }; };