fix: crash in gin::wrappable::secondweakcallback (#45368)

patches/chromium/.patches

@@ -140,3 +140,4 @@ build_add_public_config_simdutf_config.patch
 revert_code_health_clean_up_stale_macwebcontentsocclusion.patch
 build_remove_vr_directx_helpers_dependency.patch
 ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch
+feat_add_signals_when_embedder_cleanup_callbacks_run_for.patch

patches/chromium/feat_add_signals_when_embedder_cleanup_callbacks_run_for.patch

@@ -0,0 +1,168 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: deepak1556 <[email protected]>
+Date: Wed, 29 Jan 2025 17:01:03 +0900
+Subject: feat: add signals when embedder cleanup callbacks run for
+ gin::wrappable
+
+Current setup of finalization callbacks does not work well with
+gin_helper::CleanedUpAtExit for wrappables specifically on environment
+shutdown leading to UAF in the second pass.
+
+Details at  https://github.com/microsoft/vscode/issues/192119#issuecomment-2375851531
+
+The signals exposed in this patch does the following 2 things,
+
+1) Fix weak state of the wrapped object when the finializer callbacks
+   have not yet been processed
+2) Avoid calling into the second pass when the embedder has already
+  destroyed the wrapped object via CleanedUpAtExit.
+
+This patch is more of a bandaid fix to improve the lifetime
+management with existing finalizer callbacks. We should be able to
+remove this patch once gin::Wrappable can be managed by V8 Oilpan
+
+Refs https://issues.chromium.org/issues/40210365 which is blocked
+on https://issues.chromium.org/issues/42203693
+
+diff --git a/gin/isolate_holder.cc b/gin/isolate_holder.cc
+index 2be37976a1305f1deed561b3b829dbb5d7ae85e7..44eb16f17d272125e2b4a590f8962eb8144d9755 100644
+--- a/gin/isolate_holder.cc
++++ b/gin/isolate_holder.cc
+@@ -34,6 +34,8 @@ v8::ArrayBuffer::Allocator* g_array_buffer_allocator = nullptr;
+ const intptr_t* g_reference_table = nullptr;
+ v8::FatalErrorCallback g_fatal_error_callback = nullptr;
+ v8::OOMErrorCallback g_oom_error_callback = nullptr;
++bool g_initialized_microtasks_runner = false;
++bool g_destroyed_microtasks_runner = false;
+ 
+ std::unique_ptr<v8::Isolate::CreateParams> getModifiedIsolateParams(
+     std::unique_ptr<v8::Isolate::CreateParams> params,
+@@ -198,10 +200,26 @@ IsolateHolder::getDefaultIsolateParams() {
+   return params;
+ }
+ 
++// static
++bool IsolateHolder::DestroyedMicrotasksRunner() {
++  return g_initialized_microtasks_runner &&
++         g_destroyed_microtasks_runner;
++}
++
+ void IsolateHolder::EnableIdleTasks(
+     std::unique_ptr<V8IdleTaskRunner> idle_task_runner) {
+   DCHECK(isolate_data_.get());
+   isolate_data_->EnableIdleTasks(std::move(idle_task_runner));
+ }
+ 
++void IsolateHolder::WillCreateMicrotasksRunner() {
++  DCHECK(!g_initialized_microtasks_runner);
++  g_initialized_microtasks_runner = true;
++}
++
++void IsolateHolder::WillDestroyMicrotasksRunner() {
++  DCHECK(g_initialized_microtasks_runner);
++  g_destroyed_microtasks_runner = true;
++}
++
+ }  // namespace gin
+diff --git a/gin/public/isolate_holder.h b/gin/public/isolate_holder.h
+index 52b8c1af58678b9fee684ff75340c98fcc73b552..f79407d741a30298d09efd53589f16dc9b26107f 100644
+--- a/gin/public/isolate_holder.h
++++ b/gin/public/isolate_holder.h
+@@ -131,6 +131,8 @@ class GIN_EXPORT IsolateHolder {
+   // Should only be called after v8::IsolateHolder::Initialize() is invoked.
+   static std::unique_ptr<v8::Isolate::CreateParams> getDefaultIsolateParams();
+ 
++  static bool DestroyedMicrotasksRunner();
++
+   v8::Isolate* isolate() { return isolate_; }
+ 
+   // This method returns if v8::Locker is needed to access isolate.
+@@ -144,6 +146,9 @@ class GIN_EXPORT IsolateHolder {
+ 
+   void EnableIdleTasks(std::unique_ptr<V8IdleTaskRunner> idle_task_runner);
+ 
++  void WillCreateMicrotasksRunner();
++  void WillDestroyMicrotasksRunner();
++
+   // This method returns V8IsolateMemoryDumpProvider of this isolate, used for
+   // testing.
+   V8IsolateMemoryDumpProvider* isolate_memory_dump_provider_for_testing()
+diff --git a/gin/wrappable.cc b/gin/wrappable.cc
+index 402355cb836cea14e9ee725a142a4bad44fd5bed..7e7f028dcfb87c7b80adebabac19ced8791f642e 100644
+--- a/gin/wrappable.cc
++++ b/gin/wrappable.cc
+@@ -13,6 +13,9 @@ namespace gin {
+ WrappableBase::WrappableBase() = default;
+ 
+ WrappableBase::~WrappableBase() {
++  if (!wrapper_.IsEmpty()) {
++    wrapper_.ClearWeak();
++  }
+   wrapper_.Reset();
+ }
+ 
+@@ -28,15 +31,24 @@ const char* WrappableBase::GetTypeName() {
+ void WrappableBase::FirstWeakCallback(
+     const v8::WeakCallbackInfo<WrappableBase>& data) {
+   WrappableBase* wrappable = data.GetParameter();
+-  wrappable->dead_ = true;
+-  wrappable->wrapper_.Reset();
+-  data.SetSecondPassCallback(SecondWeakCallback);
++  WrappableBase* wrappable_from_field =
++      static_cast<WrappableBase*>(data.GetInternalField(1));
++  if (wrappable && wrappable == wrappable_from_field) {
++    wrappable->dead_ = true;
++    wrappable->wrapper_.Reset();
++    data.SetSecondPassCallback(SecondWeakCallback);
++  }
+ }
+ 
+ void WrappableBase::SecondWeakCallback(
+     const v8::WeakCallbackInfo<WrappableBase>& data) {
++  if (IsolateHolder::DestroyedMicrotasksRunner()) {
++    return;
++  }
+   WrappableBase* wrappable = data.GetParameter();
+-  delete wrappable;
++  if (wrappable) {
++    delete wrappable;
++  }
+ }
+ 
+ v8::MaybeLocal<v8::Object> WrappableBase::GetWrapperImpl(v8::Isolate* isolate,
+@@ -71,10 +83,16 @@ v8::MaybeLocal<v8::Object> WrappableBase::GetWrapperImpl(v8::Isolate* isolate,
+   void* values[] = {info, this};
+   wrapper->SetAlignedPointerInInternalFields(2, indices, values);
+   wrapper_.Reset(isolate, wrapper);
+-  wrapper_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kParameter);
++  wrapper_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kInternalFields);
+   return v8::MaybeLocal<v8::Object>(wrapper);
+ }
+ 
++void WrappableBase::ClearWeak() {
++  if (!wrapper_.IsEmpty()) {
++    wrapper_.ClearWeak();
++  }
++}
++
+ namespace internal {
+ 
+ void* FromV8Impl(v8::Isolate* isolate, v8::Local<v8::Value> val,
+diff --git a/gin/wrappable.h b/gin/wrappable.h
+index 4e7115685a5bf6997e78edcc1851e28bd00b1aa2..ca51fe33605e855438e88969e3d3cc734ef4523e 100644
+--- a/gin/wrappable.h
++++ b/gin/wrappable.h
+@@ -80,6 +80,13 @@ class GIN_EXPORT WrappableBase {
+   v8::MaybeLocal<v8::Object> GetWrapperImpl(v8::Isolate* isolate,
+                                             WrapperInfo* wrapper_info);
+ 
++  // Make this wrappable strong again. This is useful when the wrappable is
++  // destroyed outside the finalizer callbacks and we want to avoid scheduling
++  // the weak callbacks if they haven't been scheduled yet.
++  // NOTE!!! this does not prevent finalization callbacks from running if they
++  // have already been processed.
++  void ClearWeak();
++
+  private:
+   static void FirstWeakCallback(
+       const v8::WeakCallbackInfo<WrappableBase>& data);

patches/chromium/ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch

@@ -11,10 +11,10 @@ Bug: N/A
 Change-Id: I9fc472212b2d3afac2c8e18a2159bc2d50bbdf98
 
 diff --git a/AUTHORS b/AUTHORS
-index 55dc38c1448c1960b802c136018c8be22ed61c18..5cd195df3650331fbfd62b2f964368b5f3217f3c 100644
+index 6008db66fcb0686046a5ca40d04bb2534cb0b04a..d5079e68cef38168c4c66833a53121930fa59afd 100644
 --- a/AUTHORS
 +++ b/AUTHORS
-@@ -337,6 +337,7 @@ David Futcher <[email protected]>
+@@ -339,6 +339,7 @@ David Futcher <[email protected]>
  David Jin <[email protected]>
  David Lechner <[email protected]>
  David Leen <[email protected]>
@@ -23,10 +23,10 @@ index 55dc38c1448c1960b802c136018c8be22ed61c18..5cd195df3650331fbfd62b2f964368b5
  David McAllister <[email protected]>
  David Michael Barr <[email protected]>
 diff --git a/base/win/shortcut.cc b/base/win/shortcut.cc
-index e790adb2f1d6529ac0dd77145f5da2796264c7ae..8a7edcfaf9af963468b4b42fe55a771fb31f13a2 100644
+index 967e130e823f41c402411dfadb53b805e8a8c92b..3a9df7f31861ca69168fd24513ee554d0984798d 100644
 --- a/base/win/shortcut.cc
 +++ b/base/win/shortcut.cc
-@@ -342,8 +342,9 @@ bool ResolveShortcutProperties(const FilePath& shortcut_path,
+@@ -356,8 +356,9 @@ bool ResolveShortcutProperties(const FilePath& shortcut_path,
                *(pv_toast_activator_clsid.get().puuid));
            break;
          default:

shell/browser/api/electron_api_notification.cc

@@ -236,6 +236,10 @@ const char* Notification::GetTypeName() {
   return GetClassName();
 }
 
+void Notification::WillBeDestroyed() {
+  ClearWeak();
+}
+
 }  // namespace electron::api
 
 namespace {

shell/browser/api/electron_api_notification.h

@@ -57,6 +57,9 @@ class Notification final : public gin::Wrappable<Notification>,
   static gin::WrapperInfo kWrapperInfo;
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
   // disable copy
   Notification(const Notification&) = delete;
   Notification& operator=(const Notification&) = delete;

shell/browser/api/electron_api_session.cc

@@ -1909,6 +1909,10 @@ const char* Session::GetTypeName() {
   return GetClassName();
 }
 
+void Session::WillBeDestroyed() {
+  ClearWeak();
+}
+
 }  // namespace electron::api
 
 namespace {

shell/browser/api/electron_api_session.h

@@ -103,6 +103,9 @@ class Session final : public gin::Wrappable<Session>,
   static const char* GetClassName() { return "Session"; }
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
   // Methods.
   v8::Local<v8::Promise> ResolveHost(
       std::string host,

shell/browser/api/electron_api_tray.cc

@@ -431,6 +431,10 @@ const char* Tray::GetTypeName() {
   return GetClassName();
 }
 
+void Tray::WillBeDestroyed() {
+  ClearWeak();
+}
+
 }  // namespace electron::api
 
 namespace {

shell/browser/api/electron_api_tray.h

@@ -58,6 +58,9 @@ class Tray final : public gin::Wrappable<Tray>,
   static gin::WrapperInfo kWrapperInfo;
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
   // disable copy
   Tray(const Tray&) = delete;
   Tray& operator=(const Tray&) = delete;

shell/browser/api/electron_api_web_contents.cc

@@ -4569,6 +4569,10 @@ const char* WebContents::GetTypeName() {
   return GetClassName();
 }
 
+void WebContents::WillBeDestroyed() {
+  ClearWeak();
+}
+
 ElectronBrowserContext* WebContents::GetBrowserContext() const {
   return static_cast<ElectronBrowserContext*>(
       web_contents()->GetBrowserContext());

shell/browser/api/electron_api_web_contents.h

@@ -180,6 +180,9 @@ class WebContents final : public ExclusiveAccessContext,
   static gin::WrapperInfo kWrapperInfo;
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
   void Destroy();
   void Close(std::optional<gin_helper::Dictionary> options);
   base::WeakPtr<WebContents> GetWeakPtr() { return weak_factory_.GetWeakPtr(); }

shell/browser/api/message_port.cc

@@ -306,6 +306,10 @@ const char* MessagePort::GetTypeName() {
   return "MessagePort";
 }
 
+void MessagePort::WillBeDestroyed() {
+  ClearWeak();
+}
+
 }  // namespace electron
 
 namespace {

shell/browser/api/message_port.h

@@ -61,6 +61,9 @@ class MessagePort final : public gin::Wrappable<MessagePort>,
       v8::Isolate* isolate) override;
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
  private:
   MessagePort();
 

shell/browser/javascript_environment.cc

@@ -133,11 +133,16 @@ v8::Isolate* JavascriptEnvironment::GetIsolate() {
 void JavascriptEnvironment::CreateMicrotasksRunner() {
   DCHECK(!microtasks_runner_);
   microtasks_runner_ = std::make_unique<MicrotasksRunner>(isolate());
+  isolate_holder_.WillCreateMicrotasksRunner();
   base::CurrentThread::Get()->AddTaskObserver(microtasks_runner_.get());
 }
 
 void JavascriptEnvironment::DestroyMicrotasksRunner() {
   DCHECK(microtasks_runner_);
+  // Should be called before running gin_helper::CleanedUpAtExit::DoCleanup.
+  // This helps to signal wrappable finalizer callbacks to not act on freed
+  // parameters.
+  isolate_holder_.WillDestroyMicrotasksRunner();
   {
     v8::HandleScope scope(isolate_);
     gin_helper::CleanedUpAtExit::DoCleanup();

shell/common/api/electron_api_url_loader.cc

@@ -819,4 +819,8 @@ const char* SimpleURLLoaderWrapper::GetTypeName() {
   return "SimpleURLLoaderWrapper";
 }
 
+void SimpleURLLoaderWrapper::WillBeDestroyed() {
+  ClearWeak();
+}
+
 }  // namespace electron::api

shell/common/api/electron_api_url_loader.h

@@ -66,6 +66,9 @@ class SimpleURLLoaderWrapper final
       v8::Isolate* isolate) override;
   const char* GetTypeName() override;
 
+  // gin_helper::CleanedUpAtExit
+  void WillBeDestroyed() override;
+
  private:
   SimpleURLLoaderWrapper(ElectronBrowserContext* browser_context,
                          std::unique_ptr<network::ResourceRequest> request,

shell/common/gin_helper/cleaned_up_at_exit.cc

@@ -27,11 +27,14 @@ CleanedUpAtExit::~CleanedUpAtExit() {
   std::erase(GetDoomed(), this);
 }
 
+void CleanedUpAtExit::WillBeDestroyed() {}
+
 // static
 void CleanedUpAtExit::DoCleanup() {
   auto& doomed = GetDoomed();
   while (!doomed.empty()) {
     CleanedUpAtExit* next = doomed.back();
+    next->WillBeDestroyed();
     delete next;
   }
 }

shell/common/gin_helper/cleaned_up_at_exit.h

@@ -19,6 +19,8 @@ class CleanedUpAtExit {
   CleanedUpAtExit();
   virtual ~CleanedUpAtExit();
 
+  virtual void WillBeDestroyed();
+
   static void DoCleanup();
 };
 

shell/common/gin_helper/wrappable.cc

@@ -4,6 +4,7 @@
 
 #include "shell/common/gin_helper/wrappable.h"
 
+#include "gin/public/isolate_holder.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "v8/include/v8-function.h"
 
@@ -60,8 +61,10 @@ void WrappableBase::InitWith(v8::Isolate* isolate,
 // static
 void WrappableBase::FirstWeakCallback(
     const v8::WeakCallbackInfo<WrappableBase>& data) {
-  auto* wrappable = static_cast<WrappableBase*>(data.GetInternalField(0));
-  if (wrappable) {
+  WrappableBase* wrappable = data.GetParameter();
+  auto* wrappable_from_field =
+      static_cast<WrappableBase*>(data.GetInternalField(0));
+  if (wrappable && wrappable == wrappable_from_field) {
     wrappable->wrapper_.Reset();
     data.SetSecondPassCallback(SecondWeakCallback);
   }
@@ -70,6 +73,9 @@ void WrappableBase::FirstWeakCallback(
 // static
 void WrappableBase::SecondWeakCallback(
     const v8::WeakCallbackInfo<WrappableBase>& data) {
+  if (gin::IsolateHolder::DestroyedMicrotasksRunner()) {
+    return;
+  }
   delete static_cast<WrappableBase*>(data.GetInternalField(0));
 }