diff --git a/patches/chromium/.patches b/patches/chromium/.patches index df0230ef290c..fd4c1b71dd1f 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -138,3 +138,4 @@ support_bstr_pkey_appusermodel_id_in_windows_shortcuts.patch cherry-pick-1282289030ab.patch cherry-pick-3dc17c461b12.patch ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch +feat_add_signals_when_embedder_cleanup_callbacks_run_for.patch diff --git a/patches/chromium/feat_add_signals_when_embedder_cleanup_callbacks_run_for.patch b/patches/chromium/feat_add_signals_when_embedder_cleanup_callbacks_run_for.patch new file mode 100644 index 000000000000..a19135828cdf --- /dev/null +++ b/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 +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 getModifiedIsolateParams( + std::unique_ptr 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 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 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 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& data) { + WrappableBase* wrappable = data.GetParameter(); +- wrappable->dead_ = true; +- wrappable->wrapper_.Reset(); +- data.SetSecondPassCallback(SecondWeakCallback); ++ WrappableBase* wrappable_from_field = ++ static_cast(data.GetInternalField(1)); ++ if (wrappable && wrappable == wrappable_from_field) { ++ wrappable->dead_ = true; ++ wrappable->wrapper_.Reset(); ++ data.SetSecondPassCallback(SecondWeakCallback); ++ } + } + + void WrappableBase::SecondWeakCallback( + const v8::WeakCallbackInfo& data) { ++ if (IsolateHolder::DestroyedMicrotasksRunner()) { ++ return; ++ } + WrappableBase* wrappable = data.GetParameter(); +- delete wrappable; ++ if (wrappable) { ++ delete wrappable; ++ } + } + + v8::MaybeLocal WrappableBase::GetWrapperImpl(v8::Isolate* isolate, +@@ -71,10 +83,16 @@ v8::MaybeLocal 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(wrapper); + } + ++void WrappableBase::ClearWeak() { ++ if (!wrapper_.IsEmpty()) { ++ wrapper_.ClearWeak(); ++ } ++} ++ + namespace internal { + + void* FromV8Impl(v8::Isolate* isolate, v8::Local 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 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& data); diff --git a/patches/chromium/ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch b/patches/chromium/ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch index 6f97dd9b9ce8..f55532ec4416 100644 --- a/patches/chromium/ignore_parse_errors_for_pkey_appusermodel_toastactivatorclsid.patch +++ b/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 +@@ -339,6 +339,7 @@ David Futcher David Jin David Lechner David Leen @@ -23,10 +23,10 @@ index 55dc38c1448c1960b802c136018c8be22ed61c18..5cd195df3650331fbfd62b2f964368b5 David McAllister David Michael Barr 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: diff --git a/shell/browser/api/electron_api_notification.cc b/shell/browser/api/electron_api_notification.cc index 459a2e1b7e2b..db63ac7326c5 100644 --- a/shell/browser/api/electron_api_notification.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_notification.h b/shell/browser/api/electron_api_notification.h index c67c5d716010..96f5329158ca 100644 --- a/shell/browser/api/electron_api_notification.h +++ b/shell/browser/api/electron_api_notification.h @@ -57,6 +57,9 @@ class Notification final : public gin::Wrappable, 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; diff --git a/shell/browser/api/electron_api_session.cc b/shell/browser/api/electron_api_session.cc index ae705c4fa5e6..e3e756750dd3 100644 --- a/shell/browser/api/electron_api_session.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_session.h b/shell/browser/api/electron_api_session.h index 902609b2c707..327e9a7552c5 100644 --- a/shell/browser/api/electron_api_session.h +++ b/shell/browser/api/electron_api_session.h @@ -102,6 +102,9 @@ class Session final : public gin::Wrappable, static const char* GetClassName() { return "Session"; } const char* GetTypeName() override; + // gin_helper::CleanedUpAtExit + void WillBeDestroyed() override; + // Methods. v8::Local ResolveHost( std::string host, diff --git a/shell/browser/api/electron_api_tray.cc b/shell/browser/api/electron_api_tray.cc index 1d69d848858e..0cfd05323439 100644 --- a/shell/browser/api/electron_api_tray.cc +++ b/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 { diff --git a/shell/browser/api/electron_api_tray.h b/shell/browser/api/electron_api_tray.h index fbcb908d044a..b59615c095a3 100644 --- a/shell/browser/api/electron_api_tray.h +++ b/shell/browser/api/electron_api_tray.h @@ -58,6 +58,9 @@ class Tray final : public gin::Wrappable, 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; diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index abd9ec4f1232..6c28fee6f80e 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -4497,6 +4497,10 @@ const char* WebContents::GetTypeName() { return GetClassName(); } +void WebContents::WillBeDestroyed() { + ClearWeak(); +} + ElectronBrowserContext* WebContents::GetBrowserContext() const { return static_cast( web_contents()->GetBrowserContext()); diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index ac91f0db0cbf..2c75de6c140c 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -175,6 +175,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 options); base::WeakPtr GetWeakPtr() { return weak_factory_.GetWeakPtr(); } diff --git a/shell/browser/api/message_port.cc b/shell/browser/api/message_port.cc index 9792bedaca30..3d56361fc7d8 100644 --- a/shell/browser/api/message_port.cc +++ b/shell/browser/api/message_port.cc @@ -306,6 +306,10 @@ const char* MessagePort::GetTypeName() { return "MessagePort"; } +void MessagePort::WillBeDestroyed() { + ClearWeak(); +} + } // namespace electron namespace { diff --git a/shell/browser/api/message_port.h b/shell/browser/api/message_port.h index 1a2c7ff52b46..e46be76cbebb 100644 --- a/shell/browser/api/message_port.h +++ b/shell/browser/api/message_port.h @@ -61,6 +61,9 @@ class MessagePort final : public gin::Wrappable, v8::Isolate* isolate) override; const char* GetTypeName() override; + // gin_helper::CleanedUpAtExit + void WillBeDestroyed() override; + private: MessagePort(); diff --git a/shell/browser/javascript_environment.cc b/shell/browser/javascript_environment.cc index be9d3931f5e9..df6d6a9707c9 100644 --- a/shell/browser/javascript_environment.cc +++ b/shell/browser/javascript_environment.cc @@ -132,11 +132,16 @@ v8::Isolate* JavascriptEnvironment::GetIsolate() { void JavascriptEnvironment::CreateMicrotasksRunner() { DCHECK(!microtasks_runner_); microtasks_runner_ = std::make_unique(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(); diff --git a/shell/common/api/electron_api_url_loader.cc b/shell/common/api/electron_api_url_loader.cc index 109e84024479..afc409c68ca4 100644 --- a/shell/common/api/electron_api_url_loader.cc +++ b/shell/common/api/electron_api_url_loader.cc @@ -817,4 +817,8 @@ const char* SimpleURLLoaderWrapper::GetTypeName() { return "SimpleURLLoaderWrapper"; } +void SimpleURLLoaderWrapper::WillBeDestroyed() { + ClearWeak(); +} + } // namespace electron::api diff --git a/shell/common/api/electron_api_url_loader.h b/shell/common/api/electron_api_url_loader.h index f3f14cf6fa39..fa789b4ebd3b 100644 --- a/shell/common/api/electron_api_url_loader.h +++ b/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 request, diff --git a/shell/common/gin_helper/cleaned_up_at_exit.cc b/shell/common/gin_helper/cleaned_up_at_exit.cc index 2632061219ef..6a73bfeccb73 100644 --- a/shell/common/gin_helper/cleaned_up_at_exit.cc +++ b/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; } } diff --git a/shell/common/gin_helper/cleaned_up_at_exit.h b/shell/common/gin_helper/cleaned_up_at_exit.h index 35d9e1618eb7..e037264275c9 100644 --- a/shell/common/gin_helper/cleaned_up_at_exit.h +++ b/shell/common/gin_helper/cleaned_up_at_exit.h @@ -19,6 +19,8 @@ class CleanedUpAtExit { CleanedUpAtExit(); virtual ~CleanedUpAtExit(); + virtual void WillBeDestroyed(); + static void DoCleanup(); }; diff --git a/shell/common/gin_helper/wrappable.cc b/shell/common/gin_helper/wrappable.cc index 06b5061f18be..81fa5b8373ea 100644 --- a/shell/common/gin_helper/wrappable.cc +++ b/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& data) { - auto* wrappable = static_cast(data.GetInternalField(0)); - if (wrappable) { + WrappableBase* wrappable = data.GetParameter(); + auto* wrappable_from_field = + static_cast(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& data) { + if (gin::IsolateHolder::DestroyedMicrotasksRunner()) { + return; + } delete static_cast(data.GetInternalField(0)); }