From 8c5c6a6088835846487982d32e7a7c77a951fcdd Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Fri, 23 Jan 2026 05:29:01 -0600 Subject: [PATCH] refactor: use `gin::Wrappable` for `electron::api::DataPipeHolder` (#49495) * refactor: make `DataPipeHolder` inherit from `gin::Wrappable` * test: add a test to ensure GC clears the data pipe holder * chore: e patches all * chore: e patches all (trivial only) * refactor: make AllDataPipeHolders a base::flat_map of WeakPersistent --- ...ctron_objects_to_wrappablepointertag.patch | 5 +- .../api/electron_api_data_pipe_holder.cc | 53 ++++----- .../api/electron_api_data_pipe_holder.h | 32 +++--- shell/browser/api/electron_api_session.cc | 12 +- shell/common/gin_converters/net_converter.cc | 3 +- spec/api-session-spec.ts | 106 +++++++++++++++++- 6 files changed, 156 insertions(+), 55 deletions(-) diff --git a/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch b/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch index 870da9ff98..621ea1532a 100644 --- a/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch +++ b/patches/chromium/chore_add_electron_objects_to_wrappablepointertag.patch @@ -8,16 +8,17 @@ electron objects that extend gin::Wrappable and gets allocated on the cpp heap diff --git a/gin/public/wrappable_pointer_tags.h b/gin/public/wrappable_pointer_tags.h -index 573bcb2e56068a2ade6d8ab28964b077487874fd..a21a0ebda93b16f592977768bfe1c717caaf93b5 100644 +index 573bcb2e56068a2ade6d8ab28964b077487874fd..16145e466cd560784b89681aa642e350d5b28f12 100644 --- a/gin/public/wrappable_pointer_tags.h +++ b/gin/public/wrappable_pointer_tags.h -@@ -74,7 +74,16 @@ enum WrappablePointerTag : uint16_t { +@@ -74,7 +74,17 @@ enum WrappablePointerTag : uint16_t { kTextInputControllerBindings, // content::TextInputControllerBindings kWebAXObjectProxy, // content::WebAXObjectProxy kWrappedExceptionHandler, // extensions::WrappedExceptionHandler - kLastPointerTag = kWrappedExceptionHandler, + kElectronApp, // electron::api::App + kElectronDebugger, // electron::api::Debugger ++ kElectronDataPipeHolder, // electron::api::DataPipeHolder + kElectronEvent, // gin_helper::internal::Event + kElectronMenu, // electron::api::Menu + kElectronNetLog, // electron::api::NetLog diff --git a/shell/browser/api/electron_api_data_pipe_holder.cc b/shell/browser/api/electron_api_data_pipe_holder.cc index 672ae1c9ff..960cf5d770 100644 --- a/shell/browser/api/electron_api_data_pipe_holder.cc +++ b/shell/browser/api/electron_api_data_pipe_holder.cc @@ -7,6 +7,8 @@ #include #include +#include "base/containers/flat_map.h" +#include "base/containers/map_util.h" #include "base/memory/weak_ptr.h" #include "base/no_destructor.h" #include "base/strings/string_number_conversions.h" @@ -14,10 +16,10 @@ #include "mojo/public/cpp/system/data_pipe.h" #include "mojo/public/cpp/system/simple_watcher.h" #include "net/base/net_errors.h" -#include "shell/common/gin_helper/handle.h" #include "shell/common/gin_helper/promise.h" -#include "shell/common/key_weak_map.h" #include "shell/common/node_util.h" +#include "v8/include/cppgc/allocation.h" +#include "v8/include/v8-cppgc.h" #include "shell/common/node_includes.h" @@ -29,9 +31,11 @@ namespace { int g_next_id = 0; // Map that manages all the DataPipeHolder objects. -KeyWeakMap& AllDataPipeHolders() { - static base::NoDestructor> weak_map; - return *weak_map.get(); +[[nodiscard]] auto& AllDataPipeHolders() { + static base::NoDestructor< + base::flat_map>> + weak_map; + return *weak_map; } // Utility class to read from data pipe. @@ -143,8 +147,9 @@ class DataPipeReader { } // namespace -gin::DeprecatedWrapperInfo DataPipeHolder::kWrapperInfo = { - gin::kEmbedderNativeGin}; +const gin::WrapperInfo DataPipeHolder::kWrapperInfo = { + {gin::kEmbedderNativeGin}, + gin::kElectronDataPipeHolder}; DataPipeHolder::DataPipeHolder(const network::DataElement& element) : id_(base::NumberToString(++g_next_id)) { @@ -166,30 +171,28 @@ v8::Local DataPipeHolder::ReadAll(v8::Isolate* isolate) { return handle; } -const char* DataPipeHolder::GetTypeName() { - return "DataPipeHolder"; +const gin::WrapperInfo* DataPipeHolder::wrapper_info() const { + return &kWrapperInfo; +} + +const char* DataPipeHolder::GetHumanReadableName() const { + return "Electron / DataPipeHolder"; } // static -gin_helper::Handle DataPipeHolder::Create( - v8::Isolate* isolate, - const network::DataElement& element) { - auto handle = gin_helper::CreateHandle(isolate, new DataPipeHolder(element)); - AllDataPipeHolders().Set(isolate, handle->id(), - handle->GetWrapper(isolate).ToLocalChecked()); - return handle; +DataPipeHolder* DataPipeHolder::Create(v8::Isolate* isolate, + const network::DataElement& element) { + auto* holder = cppgc::MakeGarbageCollected( + isolate->GetCppHeap()->GetAllocationHandle(), element); + AllDataPipeHolders().insert_or_assign(holder->id(), holder); + return holder; } // static -gin_helper::Handle DataPipeHolder::From(v8::Isolate* isolate, - const std::string& id) { - v8::MaybeLocal object = AllDataPipeHolders().Get(isolate, id); - if (!object.IsEmpty()) { - gin_helper::Handle handle; - if (gin::ConvertFromV8(isolate, object.ToLocalChecked(), &handle)) - return handle; - } - return {}; +DataPipeHolder* DataPipeHolder::From(v8::Isolate* isolate, + const std::string_view id) { + auto* found = base::FindOrNull(AllDataPipeHolders(), id); + return found ? found->Get() : nullptr; } } // namespace electron::api diff --git a/shell/browser/api/electron_api_data_pipe_holder.h b/shell/browser/api/electron_api_data_pipe_holder.h index 4913438675..2691ea2c61 100644 --- a/shell/browser/api/electron_api_data_pipe_holder.h +++ b/shell/browser/api/electron_api_data_pipe_holder.h @@ -7,31 +7,28 @@ #include +#include "gin/wrappable.h" #include "mojo/public/cpp/bindings/remote.h" #include "services/network/public/cpp/data_element.h" #include "services/network/public/mojom/data_pipe_getter.mojom.h" -#include "shell/common/gin_helper/wrappable.h" - -namespace gin_helper { -template -class Handle; -} // namespace gin_helper namespace electron::api { // Retains reference to the data pipe. -class DataPipeHolder final - : public gin_helper::DeprecatedWrappable { +class DataPipeHolder final : public gin::Wrappable { public: - // gin_helper::Wrappable - static gin::DeprecatedWrapperInfo kWrapperInfo; - const char* GetTypeName() override; + // gin::Wrappable + static const gin::WrapperInfo kWrapperInfo; + const gin::WrapperInfo* wrapper_info() const override; + const char* GetHumanReadableName() const override; - static gin_helper::Handle Create( - v8::Isolate* isolate, - const network::DataElement& element); - static gin_helper::Handle From(v8::Isolate* isolate, - const std::string& id); + static DataPipeHolder* Create(v8::Isolate* isolate, + const network::DataElement& element); + static DataPipeHolder* From(v8::Isolate* isolate, std::string_view id); + + // Make public for cppgc::MakeGarbageCollected. + explicit DataPipeHolder(const network::DataElement& element); + ~DataPipeHolder() override; // Read all data at once. // @@ -47,9 +44,6 @@ class DataPipeHolder final DataPipeHolder& operator=(const DataPipeHolder&) = delete; private: - explicit DataPipeHolder(const network::DataElement& element); - ~DataPipeHolder() override; - std::string id_; mojo::Remote data_pipe_; }; diff --git a/shell/browser/api/electron_api_session.cc b/shell/browser/api/electron_api_session.cc index 7514a8eca1..ceb29c29f0 100644 --- a/shell/browser/api/electron_api_session.cc +++ b/shell/browser/api/electron_api_session.cc @@ -1044,15 +1044,13 @@ bool Session::IsPersistent() { v8::Local Session::GetBlobData(v8::Isolate* isolate, const std::string& uuid) { - gin_helper::Handle holder = - DataPipeHolder::From(isolate, uuid); - if (holder.IsEmpty()) { - gin_helper::Promise> promise(isolate); - promise.RejectWithErrorMessage("Could not get blob data handle"); - return promise.GetHandle(); + if (DataPipeHolder* holder = DataPipeHolder::From(isolate, uuid)) { + return holder->ReadAll(isolate); } - return holder->ReadAll(isolate); + gin_helper::Promise> promise(isolate); + promise.RejectWithErrorMessage("Could not get blob data handle"); + return promise.GetHandle(); } void Session::DownloadURL(const GURL& url, gin::Arguments* args) { diff --git a/shell/common/gin_converters/net_converter.cc b/shell/common/gin_converters/net_converter.cc index e28d92fab4..6a7ade637f 100644 --- a/shell/common/gin_converters/net_converter.cc +++ b/shell/common/gin_converters/net_converter.cc @@ -33,6 +33,7 @@ #include "shell/common/gin_converters/value_converter.h" #include "shell/common/gin_helper/handle.h" #include "shell/common/gin_helper/promise.h" +#include "shell/common/gin_helper/wrappable.h" #include "shell/common/node_includes.h" #include "shell/common/node_util.h" #include "shell/common/v8_util.h" @@ -538,7 +539,7 @@ v8::Local Converter::ToV8( // TODO(zcbenz): After the NetworkService refactor, the old blobUUID API // becomes unnecessarily complex, we should deprecate the getBlobData // API and return the DataPipeHolder wrapper directly. - auto holder = electron::api::DataPipeHolder::Create(isolate, element); + auto* holder = electron::api::DataPipeHolder::Create(isolate, element); upload_data.Set("blobUUID", holder->id()); // The lifetime of data pipe is bound to the uploadData object. upload_data.Set("dataPipe", holder); diff --git a/spec/api-session-spec.ts b/spec/api-session-spec.ts index 37698bbdfa..0ce49400f4 100644 --- a/spec/api-session-spec.ts +++ b/spec/api-session-spec.ts @@ -12,7 +12,7 @@ import * as https from 'node:https'; import * as path from 'node:path'; import { setTimeout } from 'node:timers/promises'; -import { defer, ifit, listen } from './lib/spec-helpers'; +import { defer, ifit, listen, waitUntil } from './lib/spec-helpers'; import { closeAllWindows } from './lib/window-helpers'; describe('session module', () => { @@ -849,6 +849,110 @@ describe('session module', () => { }); }); + describe('ses.getBlobData() (gc)', () => { + const scheme = 'cors-blob'; + const protocol = session.defaultSession.protocol; + const v8Util = process._linkedBinding('electron_common_v8_util'); + + const waitForBlobDataRejection = (uuid: string) => waitUntil(async () => { + const attempt = session.defaultSession.getBlobData(uuid) + .then(() => false) + .catch(error => String(error).includes('Could not get blob data handle')); + const deadline = setTimeout(1000).then(() => false); + const rejected = await Promise.race([attempt, deadline]); + return rejected; + }); + + const waitForGarbageCollection = (weak: WeakRef) => waitUntil(() => { + v8Util.requestGarbageCollectionForTesting(); + v8Util.runUntilIdle(); + return weak.deref() === undefined; + }); + + const makeContent = (url: string, postData: string) => ` + + `; + + const registerPostHandler = ( + scheme: string, + content: string, + onDataPipe: (dataPipe: unknown) => void + ) => new Promise<{ uuid: string }>((resolve, reject) => { + protocol.registerStringProtocol(scheme, (request, callback) => { + try { + if (request.method === 'GET') { + callback({ data: content, mimeType: 'text/html' }); + } else if (request.method === 'POST') { + const uploadData = request.uploadData as any; + const uuid: string = uploadData[1].blobUUID; + const dataPipe = uploadData[1].dataPipe; + expect(dataPipe).to.be.ok(); + onDataPipe(dataPipe); + resolve({ uuid }); + callback(''); + } + } catch (error) { + reject(error); + } + }); + }); + + afterEach(closeAllWindows); + + it('keeps blob data alive while wrapper is referenced', async () => { + const url = `${scheme}://gc-alive-${Date.now()}`; + const postData = 'payload'; + const content = makeContent(url, postData); + + let heldDataPipe: unknown = null; + const postInfo = registerPostHandler(scheme, content, dataPipe => { + heldDataPipe = dataPipe; + }); + try { + const w = new BrowserWindow({ show: false }); + await w.loadURL(url); + + const { uuid } = await postInfo; + v8Util.requestGarbageCollectionForTesting(); + + const result = await session.defaultSession.getBlobData(uuid); + expect(result.toString()).to.equal(postData); + expect(heldDataPipe).to.be.ok(); + } finally { + await protocol.unregisterProtocol(scheme); + } + }); + + it('rejects after wrapper is collected', async () => { + const url = `${scheme}://gc-released-${Date.now()}`; + const postData = 'payload'; + const content = makeContent(url, postData); + + let heldDataPipe: unknown = null; + const postInfo = registerPostHandler(scheme, content, dataPipe => { + heldDataPipe = dataPipe; + }); + try { + const w = new BrowserWindow({ show: false }); + await w.loadURL(url); + + const { uuid } = await postInfo; + expect(heldDataPipe).to.be.ok(); + const weak = new WeakRef(heldDataPipe as object); + heldDataPipe = null; + + await waitForGarbageCollection(weak); + await waitForBlobDataRejection(uuid); + } finally { + await protocol.unregisterProtocol(scheme); + } + }); + }); + describe('ses.getBlobData2()', () => { const scheme = 'cors-blob'; const protocol = session.defaultSession.protocol;