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 <hop2deep@gmail.com>

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> GlobalShortcut::Create(
-    v8::Isolate* isolate) {
-  return gin_helper::CreateHandle(isolate, new GlobalShortcut());
+GlobalShortcut* GlobalShortcut::Create(v8::Isolate* isolate) {
+  return cppgc::MakeGarbageCollected<GlobalShortcut>(
+      isolate->GetCppHeap()->GetAllocationHandle(), isolate);
 }
 
 // static
 gin::ObjectTemplateBuilder GlobalShortcut::GetObjectTemplateBuilder(
     v8::Isolate* isolate) {
-  return gin_helper::DeprecatedWrappable<
-             GlobalShortcut>::GetObjectTemplateBuilder(isolate)
+  return gin::Wrappable<GlobalShortcut>::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<GlobalShortcut>::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<v8::Object> exports,
                 v8::Local<v8::Context> 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

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 <typename T>
-class Handle;
-}  // namespace gin_helper
-
 namespace electron::api {
 
-class GlobalShortcut final
-    : private ui::GlobalAcceleratorListener::Observer,
-      public gin_helper::DeprecatedWrappable<GlobalShortcut> {
+class GlobalShortcut final : public gin::Wrappable<GlobalShortcut>,
+                             private ui::GlobalAcceleratorListener::Observer,
+                             public gin::PerIsolateData::DisposeObserver {
  public:
-  static gin_helper::Handle<GlobalShortcut> 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<ui::Accelerator, base::RepeatingClosure>
       AcceleratorCallbackMap;
   typedef std::map<std::string, base::RepeatingClosure> CommandCallbackMap;
 
+  void Dispose();
   bool RegisterAll(const std::vector<ui::Accelerator>& 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<ui::Accelerator>& 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<GlobalShortcut> weak_ptr_factory_{this};
+  gin::WeakCellFactory<GlobalShortcut> weak_factory_{this};
 };
 
 }  // namespace electron::api

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