From 542ff828aba8047e747ec8b9fda5a2d4792cb4ee Mon Sep 17 00:00:00 2001 From: Robo Date: Wed, 25 Mar 2026 06:59:32 +0900 Subject: [PATCH] refactor: SafeV8Function to be backed by cppgc (#50397) * refactor: SafeV8Function to be backed by cppgc * spec: focus renderer before attempting paste * spec: remove listeners to prevent leak on failed tests --- shell/common/api/electron_api_testing.cc | 145 ++++++++++++++++++ shell/common/gin_helper/callback.cc | 63 ++++---- shell/common/gin_helper/callback.h | 6 +- spec/chromium-spec.ts | 8 + spec/cpp-heap-spec.ts | 187 ++++++++++++++++++++++- 5 files changed, 370 insertions(+), 39 deletions(-) diff --git a/shell/common/api/electron_api_testing.cc b/shell/common/api/electron_api_testing.cc index 9af44fefe5..83fdae3fc7 100644 --- a/shell/common/api/electron_api_testing.cc +++ b/shell/common/api/electron_api_testing.cc @@ -2,13 +2,17 @@ // Use of this source code is governed by the MIT license that can be // found in the LICENSE file. +#include + #include "base/command_line.h" #include "base/dcheck_is_on.h" #include "base/logging.h" +#include "base/no_destructor.h" #include "content/browser/network_service_instance_impl.h" // nogncheck #include "content/public/browser/network_service_instance.h" #include "content/public/common/content_switches.h" #include "shell/common/callback_util.h" +#include "shell/common/gin_converters/callback_converter.h" #include "shell/common/gin_helper/dictionary.h" #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" @@ -17,6 +21,93 @@ #if DCHECK_IS_ON() namespace { +class CallbackTestingHelper final { + public: + void HoldRepeatingCallback(const base::RepeatingClosure& callback) { + repeating_callback_ = callback; + } + + bool CopyHeldRepeatingCallback() { + if (!repeating_callback_) + return false; + + repeating_callback_copy_ = *repeating_callback_; + return true; + } + + bool InvokeHeldRepeatingCallback(v8::Isolate* isolate) { + if (!repeating_callback_) + return false; + + return InvokeRepeatingCallback(isolate, *repeating_callback_); + } + + bool InvokeCopiedRepeatingCallback(v8::Isolate* isolate) { + if (!repeating_callback_copy_) + return false; + + return InvokeRepeatingCallback(isolate, *repeating_callback_copy_); + } + + void HoldOnceCallback(base::OnceClosure callback) { + once_callback_ = std::move(callback); + } + + bool InvokeHeldOnceCallback(v8::Isolate* isolate) { + if (!once_callback_) + return false; + + base::OnceClosure callback = std::move(*once_callback_); + once_callback_.reset(); + return InvokeOnceCallback(isolate, std::move(callback)); + } + + void ClearPrimaryHeldRepeatingCallback() { repeating_callback_.reset(); } + + int GetHeldRepeatingCallbackCount() const { + return (repeating_callback_ ? 1 : 0) + (repeating_callback_copy_ ? 1 : 0); + } + + void ClearAllHeldCallbacks() { + repeating_callback_.reset(); + repeating_callback_copy_.reset(); + once_callback_.reset(); + } + + private: + bool InvokeRepeatingCallback(v8::Isolate* isolate, + const base::RepeatingClosure& callback) { + v8::TryCatch try_catch(isolate); + callback.Run(); + if (try_catch.HasCaught()) { + try_catch.Reset(); + return false; + } + + return true; + } + + bool InvokeOnceCallback(v8::Isolate* isolate, base::OnceClosure callback) { + v8::TryCatch try_catch(isolate); + std::move(callback).Run(); + if (try_catch.HasCaught()) { + try_catch.Reset(); + return false; + } + + return true; + } + + std::optional repeating_callback_; + std::optional repeating_callback_copy_; + std::optional once_callback_; +}; + +CallbackTestingHelper& GetCallbackTestingHelper() { + static base::NoDestructor helper; + return *helper; +} + void Log(int severity, std::string text) { switch (severity) { case logging::LOGGING_VERBOSE: @@ -57,6 +148,44 @@ v8::Local SimulateNetworkServiceCrash(v8::Isolate* isolate) { return handle; } +void HoldRepeatingCallbackForTesting(const base::RepeatingClosure& callback) { + GetCallbackTestingHelper().HoldRepeatingCallback(callback); +} + +bool CopyHeldRepeatingCallbackForTesting() { + return GetCallbackTestingHelper().CopyHeldRepeatingCallback(); +} + +bool InvokeHeldRepeatingCallbackForTesting(gin::Arguments* args) { + return GetCallbackTestingHelper().InvokeHeldRepeatingCallback( + args->isolate()); +} + +bool InvokeCopiedRepeatingCallbackForTesting(gin::Arguments* args) { + return GetCallbackTestingHelper().InvokeCopiedRepeatingCallback( + args->isolate()); +} + +void HoldOnceCallbackForTesting(base::OnceClosure callback) { + GetCallbackTestingHelper().HoldOnceCallback(std::move(callback)); +} + +bool InvokeHeldOnceCallbackForTesting(gin::Arguments* args) { + return GetCallbackTestingHelper().InvokeHeldOnceCallback(args->isolate()); +} + +void ClearPrimaryHeldRepeatingCallbackForTesting() { + GetCallbackTestingHelper().ClearPrimaryHeldRepeatingCallback(); +} + +int GetHeldRepeatingCallbackCountForTesting() { + return GetCallbackTestingHelper().GetHeldRepeatingCallbackCount(); +} + +void ClearHeldCallbacksForTesting() { + GetCallbackTestingHelper().ClearAllHeldCallbacks(); +} + void Initialize(v8::Local exports, v8::Local unused, v8::Local context, @@ -66,6 +195,22 @@ void Initialize(v8::Local exports, dict.SetMethod("log", &Log); dict.SetMethod("getLoggingDestination", &GetLoggingDestination); dict.SetMethod("simulateNetworkServiceCrash", &SimulateNetworkServiceCrash); + dict.SetMethod("holdRepeatingCallbackForTesting", + &HoldRepeatingCallbackForTesting); + dict.SetMethod("copyHeldRepeatingCallbackForTesting", + &CopyHeldRepeatingCallbackForTesting); + dict.SetMethod("invokeHeldRepeatingCallbackForTesting", + &InvokeHeldRepeatingCallbackForTesting); + dict.SetMethod("invokeCopiedRepeatingCallbackForTesting", + &InvokeCopiedRepeatingCallbackForTesting); + dict.SetMethod("clearPrimaryHeldRepeatingCallbackForTesting", + &ClearPrimaryHeldRepeatingCallbackForTesting); + dict.SetMethod("getHeldRepeatingCallbackCountForTesting", + &GetHeldRepeatingCallbackCountForTesting); + dict.SetMethod("holdOnceCallbackForTesting", &HoldOnceCallbackForTesting); + dict.SetMethod("invokeHeldOnceCallbackForTesting", + &InvokeHeldOnceCallbackForTesting); + dict.SetMethod("clearHeldCallbacksForTesting", &ClearHeldCallbacksForTesting); } } // namespace diff --git a/shell/common/gin_helper/callback.cc b/shell/common/gin_helper/callback.cc index 403fe751b1..9f499a79bb 100644 --- a/shell/common/gin_helper/callback.cc +++ b/shell/common/gin_helper/callback.cc @@ -4,12 +4,32 @@ #include "shell/common/gin_helper/callback.h" -#include "content/public/browser/browser_thread.h" #include "gin/dictionary.h" -#include "shell/common/process_util.h" +#include "gin/persistent.h" +#include "v8/include/cppgc/allocation.h" +#include "v8/include/v8-cppgc.h" +#include "v8/include/v8-traced-handle.h" namespace gin_helper { +class SafeV8FunctionHandle final + : public cppgc::GarbageCollected { + public: + SafeV8FunctionHandle(v8::Isolate* isolate, v8::Local value) + : v8_function_(isolate, value.As()) {} + + void Trace(cppgc::Visitor* visitor) const { visitor->Trace(v8_function_); } + + [[nodiscard]] bool IsAlive() const { return !v8_function_.IsEmpty(); } + + v8::Local NewHandle(v8::Isolate* isolate) const { + return v8_function_.Get(isolate); + } + + private: + v8::TracedReference v8_function_; +}; + namespace { struct TranslatorHolder { @@ -71,46 +91,19 @@ void CallTranslator(v8::Local external, } // namespace -// Destroy the class on UI thread when possible. -struct DeleteOnUIThread { - template - static void Destruct(const T* x) { - if (electron::IsBrowserProcess() && - !content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) { - content::GetUIThreadTaskRunner({})->DeleteSoon(FROM_HERE, x); - } else { - delete x; - } - } -}; - -// Like v8::Global, but ref-counted. -template -class RefCountedGlobal - : public base::RefCountedThreadSafe, DeleteOnUIThread> { - public: - RefCountedGlobal(v8::Isolate* isolate, v8::Local value) - : handle_(isolate, value.As()) {} - - [[nodiscard]] bool IsAlive() const { return !handle_.IsEmpty(); } - - v8::Local NewHandle(v8::Isolate* isolate) const { - return v8::Local::New(isolate, handle_); - } - - private: - v8::Global handle_; -}; - SafeV8Function::SafeV8Function(v8::Isolate* isolate, v8::Local value) - : v8_function_(new RefCountedGlobal(isolate, value)) {} + : v8_function_( + gin::WrapPersistent(cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), + isolate, + value))) {} SafeV8Function::SafeV8Function(const SafeV8Function& other) = default; SafeV8Function::~SafeV8Function() = default; bool SafeV8Function::IsAlive() const { - return v8_function_.get() && v8_function_->IsAlive(); + return v8_function_ && v8_function_->IsAlive(); } v8::Local SafeV8Function::NewHandle(v8::Isolate* isolate) const { diff --git a/shell/common/gin_helper/callback.h b/shell/common/gin_helper/callback.h index b16c431f34..e9173ff938 100644 --- a/shell/common/gin_helper/callback.h +++ b/shell/common/gin_helper/callback.h @@ -12,14 +12,14 @@ #include "shell/common/gin_converters/std_converter.h" #include "shell/common/gin_helper/function_template.h" #include "shell/common/gin_helper/locker.h" +#include "v8/include/cppgc/persistent.h" #include "v8/include/v8-function.h" #include "v8/include/v8-microtask-queue.h" // Implements safe conversions between JS functions and base::RepeatingCallback. namespace gin_helper { -template -class RefCountedGlobal; +class SafeV8FunctionHandle; // Manages the V8 function with RAII. class SafeV8Function { @@ -32,7 +32,7 @@ class SafeV8Function { v8::Local NewHandle(v8::Isolate* isolate) const; private: - scoped_refptr> v8_function_; + cppgc::Persistent v8_function_; }; // Helper to invoke a V8 function with C++ parameters. diff --git a/spec/chromium-spec.ts b/spec/chromium-spec.ts index dab20e629d..6ef04a70ff 100644 --- a/spec/chromium-spec.ts +++ b/spec/chromium-spec.ts @@ -993,6 +993,8 @@ describe('chromium features', () => { let w: BrowserWindow | null = null; afterEach(() => { + ipcMain.removeAllListeners('did-create-file-handle'); + ipcMain.removeAllListeners('did-create-directory-handle'); session.defaultSession.setPermissionRequestHandler(null); closeAllWindows(); }); @@ -1110,6 +1112,7 @@ describe('chromium features', () => { w.webContents.once('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testFile]); + w.webContents.focus(); w.webContents.paste(); }); }); @@ -1161,6 +1164,7 @@ describe('chromium features', () => { w.webContents.once('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testFile]); + w.webContents.focus(); w.webContents.paste(); }); }); @@ -1212,6 +1216,7 @@ describe('chromium features', () => { w.webContents.once('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testFile]); + w.webContents.focus(); w.webContents.paste(); }); }); @@ -1258,6 +1263,7 @@ describe('chromium features', () => { w.webContents.once('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testDir]); + w.webContents.focus(); w.webContents.paste(); }); }); @@ -1305,6 +1311,7 @@ describe('chromium features', () => { w.webContents.once('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testDir]); + w.webContents.focus(); w.webContents.paste(); }); }); @@ -1362,6 +1369,7 @@ describe('chromium features', () => { w.webContents.on('did-finish-load', () => { // @ts-expect-error Undocumented testing method. clipboard._writeFilesForTesting([testFile]); + w.webContents.focus(); w.webContents.paste(); }); }); diff --git a/spec/cpp-heap-spec.ts b/spec/cpp-heap-spec.ts index bc36fd8283..d4f8ee5950 100644 --- a/spec/cpp-heap-spec.ts +++ b/spec/cpp-heap-spec.ts @@ -2,7 +2,7 @@ import { expect } from 'chai'; import * as path from 'node:path'; -import { startRemoteControlApp } from './lib/spec-helpers'; +import { ifdescribe, isTestingBindingAvailable, startRemoteControlApp } from './lib/spec-helpers'; describe('cpp heap', () => { describe('app module', () => { @@ -77,6 +77,191 @@ describe('cpp heap', () => { }); }); + ifdescribe(isTestingBindingAvailable())('SafeV8Function callback conversion', () => { + const gcTestArgv = ['--js-flags=--expose-gc']; + + it('retains repeating callback while held, allows multiple invocations, then releases', async () => { + const { remotely } = await startRemoteControlApp(gcTestArgv); + const result = await remotely(async () => { + const testingBinding = (process as any)._linkedBinding('electron_common_testing'); + const v8Util = (process as any)._linkedBinding('electron_common_v8_util'); + + const waitForGC = async (fn: () => boolean) => { + for (let i = 0; i < 30; ++i) { + await new Promise(resolve => setTimeout(resolve, 0)); + v8Util.requestGarbageCollectionForTesting(); + if (fn()) return true; + } + return false; + }; + + let callCount = 0; + let repeating: any = () => { callCount++; }; + const repeatingWeakRef = new WeakRef(repeating); + testingBinding.holdRepeatingCallbackForTesting(repeating); + repeating = null; + + const invoked0 = testingBinding.invokeHeldRepeatingCallbackForTesting(); + const invoked1 = testingBinding.invokeHeldRepeatingCallbackForTesting(); + const invoked2 = testingBinding.invokeHeldRepeatingCallbackForTesting(); + + testingBinding.clearHeldCallbacksForTesting(); + const releasedAfterClear = await waitForGC(() => repeatingWeakRef.deref() === undefined); + + return { invoked0, invoked1, invoked2, callCount, releasedAfterClear }; + }); + + expect(result.invoked0).to.equal(true, 'first invocation should succeed'); + expect(result.invoked1).to.equal(true, 'second invocation should succeed'); + expect(result.invoked2).to.equal(true, 'third invocation should succeed'); + expect(result.callCount).to.equal(3, 'callback should have been called 3 times'); + expect(result.releasedAfterClear).to.equal(true, 'callback should be released after clear'); + }); + + it('consumes once callback on first invoke and releases it', async () => { + const { remotely } = await startRemoteControlApp(gcTestArgv); + const result = await remotely(async () => { + const testingBinding = (process as any)._linkedBinding('electron_common_testing'); + const v8Util = (process as any)._linkedBinding('electron_common_v8_util'); + + const waitForGC = async (fn: () => boolean) => { + for (let i = 0; i < 30; ++i) { + await new Promise(resolve => setTimeout(resolve, 0)); + v8Util.requestGarbageCollectionForTesting(); + if (fn()) return true; + } + return false; + }; + + let callCount = 0; + let once: any = () => { callCount++; }; + const onceWeakRef = new WeakRef(once); + testingBinding.holdOnceCallbackForTesting(once); + once = null; + + const first = testingBinding.invokeHeldOnceCallbackForTesting(); + const second = testingBinding.invokeHeldOnceCallbackForTesting(); + + testingBinding.clearHeldCallbacksForTesting(); + const released = await waitForGC(() => onceWeakRef.deref() === undefined); + + return { first, second, callCount, released }; + }); + + expect(result.first).to.equal(true, 'first invoke should succeed'); + expect(result.second).to.equal(false, 'second invoke should fail (consumed)'); + expect(result.callCount).to.equal(1, 'callback should have been called once'); + expect(result.released).to.equal(true, 'callback should be released after consume + clear'); + }); + + it('releases replaced repeating callback while keeping latest callback alive', async () => { + const { remotely } = await startRemoteControlApp(gcTestArgv); + const result = await remotely(async () => { + const testingBinding = (process as any)._linkedBinding('electron_common_testing'); + const v8Util = (process as any)._linkedBinding('electron_common_v8_util'); + + const waitForGC = async (fn: () => boolean) => { + for (let i = 0; i < 30; ++i) { + await new Promise(resolve => setTimeout(resolve, 0)); + v8Util.requestGarbageCollectionForTesting(); + if (fn()) return true; + } + return false; + }; + + let callbackA: any = () => {}; + const weakA = new WeakRef(callbackA); + testingBinding.holdRepeatingCallbackForTesting(callbackA); + callbackA = null; + + let callbackB: any = () => {}; + const weakB = new WeakRef(callbackB); + testingBinding.holdRepeatingCallbackForTesting(callbackB); + callbackB = null; + + const releasedA = await waitForGC(() => weakA.deref() === undefined); + + testingBinding.clearHeldCallbacksForTesting(); + const releasedB = await waitForGC(() => weakB.deref() === undefined); + + return { releasedA, releasedB }; + }); + + expect(result.releasedA).to.equal(true, 'replaced callback A should be released'); + expect(result.releasedB).to.equal(true, 'callback B should be released after clear'); + }); + + it('keeps callback alive while copied holder exists and releases after all copies clear', async () => { + const { remotely } = await startRemoteControlApp(gcTestArgv); + const result = await remotely(async () => { + const testingBinding = (process as any)._linkedBinding('electron_common_testing'); + const v8Util = (process as any)._linkedBinding('electron_common_v8_util'); + + const waitForGC = async (fn: () => boolean) => { + for (let i = 0; i < 30; ++i) { + await new Promise(resolve => setTimeout(resolve, 0)); + v8Util.requestGarbageCollectionForTesting(); + if (fn()) return true; + } + return false; + }; + + let repeating: any = () => {}; + const weakRef = new WeakRef(repeating); + testingBinding.holdRepeatingCallbackForTesting(repeating); + repeating = null; + + const copied = testingBinding.copyHeldRepeatingCallbackForTesting(); + const countAfterCopy = testingBinding.getHeldRepeatingCallbackCountForTesting(); + testingBinding.clearPrimaryHeldRepeatingCallbackForTesting(); + + const invokedViaCopy = testingBinding.invokeCopiedRepeatingCallbackForTesting(); + + testingBinding.clearHeldCallbacksForTesting(); + const releasedAfterClear = await waitForGC(() => weakRef.deref() === undefined); + + return { copied, countAfterCopy, invokedViaCopy, releasedAfterClear }; + }); + + expect(result.copied).to.equal(true, 'copy should succeed'); + expect(result.countAfterCopy).to.equal(2, 'should have 2 holders after copy'); + expect(result.invokedViaCopy).to.equal(true, 'invoke via copy should succeed'); + expect(result.releasedAfterClear).to.equal(true, 'callback should be released after all copies clear'); + }); + + it('does not leak repeating callback when callback throws during invocation', async () => { + const { remotely } = await startRemoteControlApp(gcTestArgv); + const result = await remotely(async () => { + const testingBinding = (process as any)._linkedBinding('electron_common_testing'); + const v8Util = (process as any)._linkedBinding('electron_common_v8_util'); + + const waitForGC = async (fn: () => boolean) => { + for (let i = 0; i < 30; ++i) { + await new Promise(resolve => setTimeout(resolve, 0)); + v8Util.requestGarbageCollectionForTesting(); + if (fn()) return true; + } + return false; + }; + + let throwing: any = () => { throw new Error('expected test throw'); }; + const weakRef = new WeakRef(throwing); + testingBinding.holdRepeatingCallbackForTesting(throwing); + throwing = null; + + const invokeResult = testingBinding.invokeHeldRepeatingCallbackForTesting(); + + testingBinding.clearHeldCallbacksForTesting(); + const releasedAfterClear = await waitForGC(() => weakRef.deref() === undefined); + + return { invokeResult, releasedAfterClear }; + }); + + expect(result.invokeResult).to.equal(false, 'invoke should fail (callback throws)'); + expect(result.releasedAfterClear).to.equal(true, 'throwing callback should be released after clear'); + }); + }); + describe('internal event', () => { it('should record as node in heap snapshot', async () => { const { remotely } = await startRemoteControlApp(['--expose-internals']);