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

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

patches/chromium/.patches

@@ -139,3 +139,4 @@ refactor_unfilter_unresponsive_events.patch
 build_disable_thin_lto_mac.patch
 build_add_public_config_simdutf_config.patch
 revert_code_health_clean_up_stale_macwebcontentsocclusion.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 e5ee2c6b3cb787ff9f8272d4344a1e18c44971e2..22469cf0ab1025eefcf94e2cd351087e52182130 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,
+@@ -194,10 +196,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 c22b0a7f9af621573e888a518ccdc22293ce07ef..d3e5ced425df54f42534cec5cc0c5bbfb9d79c6c 100644
+--- a/gin/public/isolate_holder.h
++++ b/gin/public/isolate_holder.h
+@@ -130,6 +130,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.
+@@ -143,6 +145,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/printing.patch

@@ -605,7 +605,7 @@ index 402be34ab888cdf834d0fb65de0832e9a8021ced..82ddc92a35d824926c30279e660cc4e8
  
  #if BUILDFLAG(IS_CHROMEOS)
 diff --git a/chrome/browser/printing/printer_query_oop.cc b/chrome/browser/printing/printer_query_oop.cc
-index e271d17b3261c47f3dbeaf9be0b3c4229ee27181..00b906660580aca7d5aabcde54d68c2161ea71dd 100644
+index e271d17b3261c47f3dbeaf9be0b3c4229ee27181..ed19764250d9125915f0f948896acd7eae7911a6 100644
 --- a/chrome/browser/printing/printer_query_oop.cc
 +++ b/chrome/browser/printing/printer_query_oop.cc
 @@ -127,7 +127,7 @@ void PrinterQueryOop::OnDidAskUserForSettings(

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

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

shell/browser/api/electron_api_session.h

@@ -102,6 +102,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

@@ -4570,6 +4570,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

@@ -179,6 +179,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

@@ -132,11 +132,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));
 }