refactor: make api::WebRequest inherit from gin::Wrappable (#48762)

* refactor: make api::WebRequest inherit from gin::Wrappable

refactor: remove unused v8::Isolate* arg from WebRequest ctor

refactor: make electron::api::Session::web_request_ a cppgc::Member<api::WebRequest>

refactor: allocate api::WebRequest on cpp heap

refactor: modify Create(), Find(), and FindOrCreate() to return a WebRequest*

* refactor: ProxyingURLLoaderFactory takes a concrete api::WebRequest instead of a WebRequestAPI

Experimental commit to ensure `ProxyingURLLoaderFactory::web_request_api_`
won't be a dangling pointer.

* chore: fix doc shear

* refactor: use cppgc::WeakPersistent<> in ProxyingURLLoaderFactory

* refactor: make ProxyingURLLoaderFactory::web_request_ const

* refactor: make ProxyingWebSocket::web_request_ a cppgc::WeakPersistent<>

* add a gin::WeakCellFactory to api::WebRequest

* refactor: use a gin::WeakCell for the bound WebRequest argument in HandleOnBeforeRequestResponseEvent()

* chore: update patches

shell/browser/api/electron_api_session.cc

@@ -1360,12 +1360,10 @@ v8::Local<v8::Value> Session::ServiceWorkerContext(v8::Isolate* isolate) {
   return service_worker_context_.Get(isolate);
 }
 
-v8::Local<v8::Value> Session::WebRequest(v8::Isolate* isolate) {
-  if (web_request_.IsEmptyThreadSafe()) {
-    auto handle = WebRequest::Create(base::PassKey<Session>{}, isolate);
-    web_request_.Reset(isolate, handle.ToV8());
-  }
-  return web_request_.Get(isolate);
+WebRequest* Session::WebRequest(v8::Isolate* isolate) {
+  if (!web_request_)
+    web_request_ = WebRequest::Create(isolate, base::PassKey<Session>{});
+  return web_request_;
 }
 
 NetLog* Session::NetLog(v8::Isolate* isolate) {

shell/browser/api/electron_api_session.h

@@ -61,6 +61,7 @@ struct PreloadScript;
 namespace api {
 
 class NetLog;
+class WebRequest;
 
 class Session final : public gin::Wrappable<Session>,
                       public gin_helper::Constructible<Session>,
@@ -169,7 +170,7 @@ class Session final : public gin::Wrappable<Session>,
   v8::Local<v8::Value> Extensions(v8::Isolate* isolate);
   v8::Local<v8::Value> Protocol(v8::Isolate* isolate);
   v8::Local<v8::Value> ServiceWorkerContext(v8::Isolate* isolate);
-  v8::Local<v8::Value> WebRequest(v8::Isolate* isolate);
+  WebRequest* WebRequest(v8::Isolate* isolate);
   api::NetLog* NetLog(v8::Isolate* isolate);
   void Preconnect(const gin_helper::Dictionary& options, gin::Arguments* args);
   v8::Local<v8::Promise> CloseAllConnections();
@@ -217,7 +218,7 @@ class Session final : public gin::Wrappable<Session>,
   v8::TracedReference<v8::Value> protocol_;
   cppgc::Member<api::NetLog> net_log_;
   v8::TracedReference<v8::Value> service_worker_context_;
-  v8::TracedReference<v8::Value> web_request_;
+  cppgc::Member<api::WebRequest> web_request_;
 
   raw_ptr<v8::Isolate> isolate_;
 

shell/browser/api/electron_api_web_request.cc

@@ -22,6 +22,7 @@
 #include "gin/converter.h"
 #include "gin/dictionary.h"
 #include "gin/object_template_builder.h"
+#include "gin/persistent.h"
 #include "shell/browser/api/electron_api_session.h"
 #include "shell/browser/api/electron_api_web_contents.h"
 #include "shell/browser/api/electron_api_web_frame_main.h"
@@ -204,7 +205,8 @@ CalculateOnBeforeSendHeadersDelta(const net::HttpRequestHeaders* old_headers,
 
 }  // namespace
 
-gin::DeprecatedWrapperInfo WebRequest::kWrapperInfo = {gin::kEmbedderNativeGin};
+const gin::WrapperInfo WebRequest::kWrapperInfo = {{gin::kEmbedderNativeGin},
+                                                   gin::kElectronWebRequest};
 
 WebRequest::RequestFilter::RequestFilter(
     std::set<URLPattern> include_url_patterns,
@@ -318,8 +320,7 @@ WebRequest::~WebRequest() = default;
 
 gin::ObjectTemplateBuilder WebRequest::GetObjectTemplateBuilder(
     v8::Isolate* isolate) {
-  return gin_helper::DeprecatedWrappable<WebRequest>::GetObjectTemplateBuilder(
-             isolate)
+  return gin::Wrappable<WebRequest>::GetObjectTemplateBuilder(isolate)
       .SetMethod(
           "onBeforeRequest",
           &WebRequest::SetResponseListener<ResponseEvent::kOnBeforeRequest>)
@@ -342,8 +343,17 @@ gin::ObjectTemplateBuilder WebRequest::GetObjectTemplateBuilder(
                  &WebRequest::SetSimpleListener<SimpleEvent::kOnCompleted>);
 }
 
-const char* WebRequest::GetTypeName() {
-  return GetClassName();
+const gin::WrapperInfo* WebRequest::wrapper_info() const {
+  return &kWrapperInfo;
+}
+
+const char* WebRequest::GetHumanReadableName() const {
+  return "Electron / WebRequest";
+}
+
+void WebRequest::Trace(cppgc::Visitor* visitor) const {
+  gin::Wrappable<WebRequest>::Trace(visitor);
+  visitor->Trace(weak_factory_);
 }
 
 bool WebRequest::HasListener() const {
@@ -381,9 +391,11 @@ int WebRequest::HandleOnBeforeRequestResponseEvent(
   gin_helper::Dictionary details(isolate, v8::Object::New(isolate));
   FillDetails(&details, request_info, request, *new_url);
 
-  ResponseCallback response =
-      base::BindOnce(&WebRequest::OnBeforeRequestListenerResult,
-                     base::Unretained(this), request_info->id);
+  auto& allocation_handle = isolate->GetCppHeap()->GetAllocationHandle();
+  ResponseCallback response = base::BindOnce(
+      &WebRequest::OnBeforeRequestListenerResult,
+      gin::WrapPersistent(weak_factory_.GetWeakCell(allocation_handle)),
+      request_info->id);
   info.listener.Run(gin::ConvertToV8(isolate, details), std::move(response));
   return net::ERR_IO_PENDING;
 }
@@ -777,22 +789,16 @@ void WebRequest::OnLoginAuthResult(
 }
 
 // static
-gin_helper::Handle<WebRequest> WebRequest::FromOrCreate(
-    v8::Isolate* isolate,
-    content::BrowserContext* browser_context) {
-  v8::Local<v8::Value> web_request =
-      Session::FromOrCreate(isolate, browser_context)->WebRequest(isolate);
-  gin_helper::Handle<WebRequest> handle;
-  gin::ConvertFromV8(isolate, web_request, &handle);
-  DCHECK(!handle.IsEmpty());
-  return handle;
+WebRequest* WebRequest::FromOrCreate(v8::Isolate* isolate,
+                                     content::BrowserContext* browser_context) {
+  return Session::FromOrCreate(isolate, browser_context)->WebRequest(isolate);
 }
 
 // static
-gin_helper::Handle<WebRequest> WebRequest::Create(
-    base::PassKey<Session> passkey,
-    v8::Isolate* isolate) {
-  return gin_helper::CreateHandle(isolate, new WebRequest{std::move(passkey)});
+WebRequest* WebRequest::Create(v8::Isolate* isolate,
+                               base::PassKey<Session> passkey) {
+  return cppgc::MakeGarbageCollected<WebRequest>(
+      isolate->GetCppHeap()->GetAllocationHandle(), std::move(passkey));
 }
 
 }  // namespace electron::api

shell/browser/api/electron_api_web_request.h

@@ -10,9 +10,10 @@
 #include <string>
 
 #include "base/types/pass_key.h"
+#include "gin/weak_cell.h"
+#include "gin/wrappable.h"
 #include "net/base/completion_once_callback.h"
 #include "services/network/public/cpp/resource_request.h"
-#include "shell/common/gin_helper/wrappable.h"
 
 class URLPattern;
 
@@ -38,7 +39,7 @@ namespace electron::api {
 
 class Session;
 
-class WebRequest final : public gin_helper::DeprecatedWrappable<WebRequest> {
+class WebRequest final : public gin::Wrappable<WebRequest> {
  public:
   using BeforeSendHeadersCallback =
       base::OnceCallback<void(const std::set<std::string>& removed_headers,
@@ -65,21 +66,29 @@ class WebRequest final : public gin_helper::DeprecatedWrappable<WebRequest> {
   // Convenience wrapper around api::Session::FromOrCreate()->WebRequest().
   // Creates the Session and WebRequest if they don't already exist.
   // Note that the WebRequest is owned by the session, not by the caller.
-  static gin_helper::Handle<WebRequest> FromOrCreate(
-      v8::Isolate* isolate,
-      content::BrowserContext* browser_context);
+  static WebRequest* FromOrCreate(v8::Isolate* isolate,
+                                  content::BrowserContext* browser_context);
 
   // Return a new WebRequest object. This can only be called by api::Session.
-  static gin_helper::Handle<WebRequest> Create(base::PassKey<Session>,
-                                               v8::Isolate* isolate);
+  static WebRequest* Create(v8::Isolate* isolate, base::PassKey<Session>);
+
+  // Make public for cppgc::MakeGarbageCollected.
+  explicit WebRequest(base::PassKey<Session>);
+  ~WebRequest() override;
+
+  // disable copy
+  WebRequest(const WebRequest&) = delete;
+  WebRequest& operator=(const WebRequest&) = delete;
 
   static const char* GetClassName() { return "WebRequest"; }
 
-  // gin_helper::Wrappable:
-  static gin::DeprecatedWrapperInfo kWrapperInfo;
+  // gin::Wrappable:
+  static const gin::WrapperInfo kWrapperInfo;
+  void Trace(cppgc::Visitor*) const override;
+  const gin::WrapperInfo* wrapper_info() const override;
+  const char* GetHumanReadableName() const override;
   gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
       v8::Isolate* isolate) override;
-  const char* GetTypeName() override;
 
   bool HasListener() const;
   int OnBeforeRequest(extensions::WebRequestInfo* info,
@@ -118,9 +127,6 @@ class WebRequest final : public gin_helper::DeprecatedWrappable<WebRequest> {
   void OnRequestWillBeDestroyed(extensions::WebRequestInfo* info);
 
  private:
-  explicit WebRequest(base::PassKey<Session>);
-  ~WebRequest() override;
-
   // Contains info about requests that are blocked waiting for a response from
   // the user.
   struct BlockedRequest;
@@ -235,6 +241,8 @@ class WebRequest final : public gin_helper::DeprecatedWrappable<WebRequest> {
   std::map<SimpleEvent, SimpleListenerInfo> simple_listeners_;
   std::map<ResponseEvent, ResponseListenerInfo> response_listeners_;
   std::map<uint64_t, BlockedRequest> blocked_requests_;
+
+  gin::WeakCellFactory<WebRequest> weak_factory_{this};
 };
 
 }  // namespace electron::api

shell/browser/electron_browser_client.cc

@@ -1272,11 +1272,11 @@ bool ElectronBrowserClient::WillInterceptWebSocket(
   v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
   v8::HandleScope scope(isolate);
   auto* browser_context = frame->GetProcess()->GetBrowserContext();
-  auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
+  auto* web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
 
   // NOTE: Some unit test environments do not initialize
   // BrowserContextKeyedAPI factories for e.g. WebRequest.
-  if (!web_request.get())
+  if (!web_request)
     return false;
 
   bool has_listener = web_request->HasListener();
@@ -1304,8 +1304,8 @@ void ElectronBrowserClient::CreateWebSocket(
   v8::HandleScope scope(isolate);
   auto* browser_context = frame->GetProcess()->GetBrowserContext();
 
-  auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
-  DCHECK(web_request.get());
+  auto* web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
+  DCHECK(web_request);
 
 #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
   if (!web_request->HasListener()) {
@@ -1322,7 +1322,7 @@ void ElectronBrowserClient::CreateWebSocket(
 #endif
 
   ProxyingWebSocket::StartProxying(
-      web_request.get(), std::move(factory), url, site_for_cookies, user_agent,
+      web_request, std::move(factory), url, site_for_cookies, user_agent,
       std::move(handshake_client), true, frame->GetProcess()->GetDeprecatedID(),
       frame->GetRoutingID(), frame->GetLastCommittedOrigin(), browser_context,
       &next_id_);
@@ -1346,8 +1346,9 @@ void ElectronBrowserClient::WillCreateURLLoaderFactory(
     scoped_refptr<base::SequencedTaskRunner> navigation_response_task_runner) {
   v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
   v8::HandleScope scope(isolate);
-  auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context);
-  DCHECK(web_request.get());
+  auto* const web_request =
+      api::WebRequest::FromOrCreate(isolate, browser_context);
+  DCHECK(web_request);
 
 #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
   if (!web_request->HasListener()) {
@@ -1388,13 +1389,18 @@ void ElectronBrowserClient::WillCreateURLLoaderFactory(
 
   auto* protocol_registry =
       ProtocolRegistry::FromBrowserContext(browser_context);
-  new ProxyingURLLoaderFactory(
-      web_request.get(), protocol_registry->intercept_handlers(),
+  new ProxyingURLLoaderFactory{
+      web_request,
+      protocol_registry->intercept_handlers(),
       render_process_id,
       frame_host ? frame_host->GetRoutingID() : IPC::mojom::kRoutingIdNone,
-      &next_id_, std::move(navigation_ui_data), std::move(navigation_id),
-      std::move(proxied_receiver), std::move(target_factory_remote),
-      std::move(header_client_receiver), type);
+      &next_id_,
+      std::move(navigation_ui_data),
+      std::move(navigation_id),
+      std::move(proxied_receiver),
+      std::move(target_factory_remote),
+      std::move(header_client_receiver),
+      type};
 }
 
 std::vector<std::unique_ptr<content::URLLoaderRequestInterceptor>>

shell/browser/net/proxying_url_loader_factory.cc

@@ -88,7 +88,7 @@ ProxyingURLLoaderFactory::InProgressRequest::~InProgressRequest() {
   // This is important to ensure that no outstanding blocking requests continue
   // to reference state owned by this object.
   if (info_) {
-    factory_->web_request()->OnRequestWillBeDestroyed(&info_.value());
+    factory_->web_request_->OnRequestWillBeDestroyed(&info_.value());
   }
   if (on_before_send_headers_callback_) {
     std::move(on_before_send_headers_callback_)
@@ -147,7 +147,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {
                             weak_factory_.GetWeakPtr());
   }
   redirect_url_ = GURL();
-  int result = factory_->web_request()->OnBeforeRequest(
+  int result = factory_->web_request_->OnBeforeRequest(
       &info_.value(), request_, continuation, &redirect_url_);
   if (result == net::ERR_BLOCKED_BY_CLIENT) {
     // The request was cancelled synchronously. Dispatch an error notification
@@ -293,8 +293,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnComplete(
   }
 
   target_client_->OnComplete(status);
-  factory_->web_request()->OnCompleted(&info_.value(), request_,
-                                       status.error_code);
+  factory_->web_request_->OnCompleted(&info_.value(), request_,
+                                      status.error_code);
 
   // Deletes |this|.
   factory_->RemoveRequest(network_service_request_id_, request_id_);
@@ -442,7 +442,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders(
   auto continuation = base::BindRepeating(
       &InProgressRequest::ContinueToSendHeaders, weak_factory_.GetWeakPtr());
   // Note: In Electron onBeforeSendHeaders is called for all protocols.
-  int result = factory_->web_request()->OnBeforeSendHeaders(
+  int result = factory_->web_request_->OnBeforeSendHeaders(
       &info_.value(), request_, continuation, &request_.headers);
 
   if (result == net::ERR_BLOCKED_BY_CLIENT) {
@@ -556,8 +556,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToSendHeaders(
     proxied_client_receiver_.Resume();
 
   // Note: In Electron onSendHeaders is called for all protocols.
-  factory_->web_request()->OnSendHeaders(&info_.value(), request_,
-                                         request_.headers);
+  factory_->web_request_->OnSendHeaders(&info_.value(), request_,
+                                        request_.headers);
 
   if (!current_request_uses_header_client_)
     ContinueToStartRequest(net::OK);
@@ -599,8 +599,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::
     if (info_->response_code == net::HTTP_PROXY_AUTHENTICATION_REQUIRED)
       return;
     // We notify the completion here, and delete |this|.
-    factory_->web_request()->OnResponseStarted(&info_.value(), request_);
-    factory_->web_request()->OnCompleted(&info_.value(), request_, net::OK);
+    factory_->web_request_->OnResponseStarted(&info_.value(), request_);
+    factory_->web_request_->OnCompleted(&info_.value(), request_, net::OK);
 
     factory_->RemoveRequest(network_service_request_id_, request_id_);
     return;
@@ -654,7 +654,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToResponseStarted(
 
   proxied_client_receiver_.Resume();
 
-  factory_->web_request()->OnResponseStarted(&info_.value(), request_);
+  factory_->web_request_->OnResponseStarted(&info_.value(), request_);
   target_client_->OnReceiveResponse(current_response_.Clone(),
                                     std::move(current_body_),
                                     std::move(current_cached_metadata_));
@@ -673,8 +673,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeRedirect(
   if (proxied_client_receiver_.is_bound())
     proxied_client_receiver_.Resume();
 
-  factory_->web_request()->OnBeforeRedirect(&info_.value(), request_,
-                                            redirect_info.new_url);
+  factory_->web_request_->OnBeforeRedirect(&info_.value(), request_,
+                                           redirect_info.new_url);
   target_client_->OnReceiveRedirect(redirect_info, current_response_.Clone());
   request_.url = redirect_info.new_url;
   request_.method = redirect_info.new_method;
@@ -697,7 +697,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::
 
   auto callback_pair = base::SplitOnceCallback(std::move(continuation));
   DCHECK(info_.has_value());
-  int result = factory_->web_request()->OnHeadersReceived(
+  int result = factory_->web_request_->OnHeadersReceived(
       &info_.value(), request_, std::move(callback_pair.first),
       current_response_->headers.get(), &override_headers_, &redirect_url_);
   if (result == net::ERR_BLOCKED_BY_CLIENT) {
@@ -725,8 +725,8 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnRequestError(
     const network::URLLoaderCompletionStatus& status) {
   if (target_client_)
     target_client_->OnComplete(status);
-  factory_->web_request()->OnErrorOccurred(&info_.value(), request_,
-                                           status.error_code);
+  factory_->web_request_->OnErrorOccurred(&info_.value(), request_,
+                                          status.error_code);
 
   // Deletes |this|.
   factory_->RemoveRequest(network_service_request_id_, request_id_);
@@ -825,7 +825,7 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
     return;
   }
 
-  if (!web_request()->HasListener()) {
+  if (!web_request_->HasListener()) {
     // Pass-through to the original factory.
     target_factory_->CreateLoaderAndStart(std::move(loader), request_id,
                                           options, request, std::move(client),

shell/browser/net/proxying_url_loader_factory.h

@@ -34,6 +34,7 @@
 #include "shell/browser/api/electron_api_web_request.h"
 #include "shell/browser/net/electron_url_loader_factory.h"
 #include "url/gurl.h"
+#include "v8/include/cppgc/persistent.h"
 
 namespace mojo {
 template <typename T>
@@ -244,8 +245,6 @@ class ProxyingURLLoaderFactory
   bool IsForServiceWorkerScript() const;
 
  private:
-  api::WebRequest* web_request() { return web_request_; }
-
   void OnTargetFactoryError();
   void OnProxyBindingError();
   void RemoveRequest(int32_t network_service_request_id, uint64_t request_id);
@@ -253,7 +252,7 @@ class ProxyingURLLoaderFactory
 
   bool ShouldIgnoreConnectionsLimit(const network::ResourceRequest& request);
 
-  raw_ptr<api::WebRequest> web_request_;
+  const cppgc::WeakPersistent<api::WebRequest> web_request_;
 
   // This is passed from api::Protocol.
   //

shell/browser/net/proxying_websocket.h

@@ -23,6 +23,7 @@
 #include "shell/browser/api/electron_api_web_request.h"
 #include "url/gurl.h"
 #include "url/origin.h"
+#include "v8/include/cppgc/persistent.h"
 
 namespace electron {
 
@@ -123,7 +124,7 @@ class ProxyingWebSocket : public network::mojom::WebSocketHandshakeClient,
   void OnMojoConnectionError();
 
   // Passed from api::WebRequest.
-  raw_ptr<api::WebRequest> web_request_;
+  const cppgc::WeakPersistent<api::WebRequest> web_request_;
 
   // Saved to feed the api::WebRequest.
   network::ResourceRequest request_;