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

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

shell/browser/api/electron_api_data_pipe_holder.cc

@@ -7,6 +7,8 @@
 #include <utility>
 #include <vector>
 
+#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<std::string>& AllDataPipeHolders() {
-  static base::NoDestructor<KeyWeakMap<std::string>> weak_map;
-  return *weak_map.get();
+[[nodiscard]] auto& AllDataPipeHolders() {
+  static base::NoDestructor<
+      base::flat_map<std::string, cppgc::WeakPersistent<DataPipeHolder>>>
+      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<v8::Promise> 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> 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<DataPipeHolder>(
+      isolate->GetCppHeap()->GetAllocationHandle(), element);
+  AllDataPipeHolders().insert_or_assign(holder->id(), holder);
+  return holder;
 }
 
 // static
-gin_helper::Handle<DataPipeHolder> DataPipeHolder::From(v8::Isolate* isolate,
-                                                        const std::string& id) {
-  v8::MaybeLocal<v8::Object> object = AllDataPipeHolders().Get(isolate, id);
-  if (!object.IsEmpty()) {
-    gin_helper::Handle<DataPipeHolder> 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

shell/browser/api/electron_api_data_pipe_holder.h

@@ -7,31 +7,28 @@
 
 #include <string>
 
+#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 <typename T>
-class Handle;
-}  // namespace gin_helper
 
 namespace electron::api {
 
 // Retains reference to the data pipe.
-class DataPipeHolder final
-    : public gin_helper::DeprecatedWrappable<DataPipeHolder> {
+class DataPipeHolder final : public gin::Wrappable<DataPipeHolder> {
  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<DataPipeHolder> Create(
-      v8::Isolate* isolate,
-      const network::DataElement& element);
-  static gin_helper::Handle<DataPipeHolder> 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<network::mojom::DataPipeGetter> data_pipe_;
 };

shell/browser/api/electron_api_session.cc

@@ -1044,15 +1044,13 @@ bool Session::IsPersistent() {
 
 v8::Local<v8::Promise> Session::GetBlobData(v8::Isolate* isolate,
                                             const std::string& uuid) {
-  gin_helper::Handle<DataPipeHolder> holder =
-      DataPipeHolder::From(isolate, uuid);
-  if (holder.IsEmpty()) {
-    gin_helper::Promise<v8::Local<v8::Value>> 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<v8::Local<v8::Value>> promise(isolate);
+  promise.RejectWithErrorMessage("Could not get blob data handle");
+  return promise.GetHandle();
 }
 
 void Session::DownloadURL(const GURL& url, gin::Arguments* args) {

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<v8::Value> Converter<network::ResourceRequestBody>::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);

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<object>) => waitUntil(() => {
+      v8Util.requestGarbageCollectionForTesting();
+      v8Util.runUntilIdle();
+      return weak.deref() === undefined;
+    });
+
+    const makeContent = (url: string, postData: string) => `<html>
+                       <script>
+                       let fd = new FormData();
+                       fd.append('file', new Blob(['${postData}'], {type:'application/json'}));
+                       fetch('${url}', {method:'POST', body: fd });
+                       </script>
+                       </html>`;
+
+    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;