[CORRUPTED] Synthetic Benchmark PR #48762 - refactor: make api::WebRequest inherit from gin::Wrappable

[tomerqodo]

User description

Benchmark PR electron#48762

Type: Corrupted (contains bugs)

Original PR Title: refactor: make api::WebRequest inherit from gin::Wrappable
Original PR Description: #### Description of Change

Part of electron#47922

• Refactor electron::api::WebRequest to inherit from gin::Wrappable instead of gin_helper::DeprecatedWrappable
• Refactor electron::api::Session::web_request_ to be a cppgc::Member<WebRequest> instead of a v8::TracedReference<v8::Value>
• Remove unused v8::Isolate* argument from electron::api::WebRequest constructor
• Modify WebRequest::Create(), WebRequest::Find(), and WebRequest::FindOrCreate() to return an api::WebRequest*

CC @deepak1556 as a electron#47922 stakeholder. All reviews welcomed.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: none
Original PR URL: electron#48762

---

PR Type

Enhancement

---

Description

• Refactor WebRequest to inherit from gin::Wrappable instead of gin_helper::DeprecatedWrappable

• Change web_request_ storage from v8::TracedReference to cppgc::Member<WebRequest>

• Allocate WebRequest on C++ heap using cppgc::MakeGarbageCollected

• Use cppgc::WeakPersistent in ProxyingURLLoaderFactory and ProxyingWebSocket for safe references

• Add gin::WeakCellFactory to WebRequest for bound callback handling

---

Diagram Walkthrough

flowchart LR
  A["WebRequest<br/>DeprecatedWrappable"] -->|refactor| B["WebRequest<br/>gin::Wrappable"]
  C["v8::TracedReference<br/>web_request_"] -->|change storage| D["cppgc::Member<br/>WebRequest"]
  E["raw_ptr<br/>WebRequest"] -->|safe reference| F["cppgc::WeakPersistent<br/>WebRequest"]
  B -->|allocate| G["cppgc::MakeGarbageCollected<br/>on cpp heap"]
  B -->|weak callbacks| H["gin::WeakCellFactory<br/>WebRequest"]

Loading

File Walkthrough

	Relevant files
Enhancement	8 files electron_api_web_request.h Refactor WebRequest to inherit from gin::Wrappable              +21/-13  electron_api_web_request.cc Update WebRequest implementation for gin::Wrappable            +26/-21  electron_api_session.h Change web_request_ to cppgc::Member storage                          +3/-2      electron_api_session.cc Update WebRequest() method to return raw pointer                  +4/-6      proxying_url_loader_factory.h Use cppgc::WeakPersistent for web_request_ reference          +2/-3      proxying_url_loader_factory.cc Update web_request_ member access throughout                          +16/-16  proxying_websocket.h Use cppgc::WeakPersistent for web_request_ reference          +2/-1      electron_browser_client.cc Update WebRequest pointer handling and initialization        +18/-12
Configuration changes	1 files chore_add_electron_objects_to_wrappablepointertag.patch Add kElectronWebRequest to wrappable pointer tags                +4/-3

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Weak callback robustness Description: The response callback for onBeforeRequest is bound to a weak persistent WebRequest via gin::WeakCell without an explicit null/expired check path, which may result in the callback silently doing nothing if the object is collected and potentially leave blocked requests unresolved or state inconsistent; verify that OnBeforeRequestListenerResult robustly handles a dead weak reference and unblocks requests to prevent denial-of-service scenarios. electron_api_web_request.cc [393-399] Referred Code 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;
	Weak pointer dereference Description: WebRequest is stored as cppgc::WeakPersistent and extensively dereferenced (e.g., web_request_->OnBeforeRequest/OnCompleted) in ProxyingURLLoaderFactory without visible null checks in call sites, which could lead to use-after-free-like logic gaps or missed security filtering if the weak reference expires; ensure all dereferences are guarded and have fallback behavior. proxying_url_loader_factory.h [255-256] Referred Code const cppgc::WeakPersistent<api::WebRequest> web_request_;
	Missing null checks Description: The code assumes WebRequest::FromOrCreate returns a non-null raw pointer (enforced by DCHECK) and then passes it to ProxyingWebSocket::StartProxying; in non-DCHECK builds a null could propagate, bypassing intended interception logic or causing crashes—add runtime null handling to avoid security feature bypass. electron_browser_client.cc [1295-1310] Referred Code void ElectronBrowserClient::CreateWebSocket( content::RenderFrameHost* frame, WebSocketFactory factory, const GURL& url, const net::SiteForCookies& site_for_cookies, const std::optional<std::string>& user_agent, mojo::PendingRemote<network::mojom::WebSocketHandshakeClient> handshake_client) { v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); v8::HandleScope scope(isolate); auto* browser_context = frame->GetProcess()->GetBrowserContext(); auto* web_request = api::WebRequest::FromOrCreate(isolate, browser_context); DCHECK(web_request); #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
	Interception bypass risk Description: Request pass-through now depends on web_request_->HasListener() via a weak reference; if the weak reference becomes null/expired, HasListener() calls may crash or skip interception, potentially allowing unauthorized requests—guard HasListener() access and define secure defaults when WebRequest is unavailable. proxying_url_loader_factory.cc [820-832] Referred Code // requests here. if (IsForServiceWorkerScript() && request.url.SchemeIsFile()) { asar::CreateAsarURLLoader( request, std::move(loader), std::move(client), base::MakeRefCounted<net::HttpResponseHeaders>("")); return; } if (!web_request_->HasListener()) { // Pass-through to the original factory. target_factory_->CreateLoaderAndStart(std::move(loader), request_id, options, request, std::move(client), traffic_annotation);
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Auditing: New network interception paths (e.g., calls to web_request_->OnBeforeRequest, OnHeadersReceived, OnCompleted, OnErrorOccurred) add critical request lifecycle actions without any added logging context to ensure auditability. Referred Code // In this case we do nothing because extensions should see nothing. return; } else { continuation = base::BindRepeating(&InProgressRequest::ContinueToBeforeSendHeaders, weak_factory_.GetWeakPtr()); } redirect_url_ = GURL(); 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 // and terminate the request. network::URLLoaderCompletionStatus status(result); OnRequestError(status); return; } if (result == net::ERR_IO_PENDING) { // One or more listeners is blocking, so the request must be paused until // they respond. |continuation| above will be invoked asynchronously to ... (clipped 562 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Weak Callback Safety: Transition to weak persistent callbacks via weak_factory_.GetWeakCell changes lifetime semantics and may drop responses if the target is collected, but the diff adds no explicit fallback or error reporting for that edge case. Referred Code blocked_request.new_url = new_url; blocked_requests_[request_info->id] = std::move(blocked_request); v8::Isolate* isolate = JavascriptEnvironment::GetIsolate(); v8::HandleScope handle_scope(isolate); gin_helper::Dictionary details(isolate, v8::Object::New(isolate)); FillDetails(&details, request_info, request, *new_url); 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; Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Pointer Lifetime: Replacement of raw_ptr with cppgc::WeakPersistent<api::WebRequest> across proxies changes input handling trust boundaries and may introduce null dereference risks without explicit null checks in new call sites. Referred Code const cppgc::WeakPersistent<api::WebRequest> web_request_; Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Fix incorrect weak reference usage The ProxyingURLLoaderFactory and ProxyingWebSocket classes incorrectly use cppgc::WeakPersistent to reference api::WebRequest, which can cause use-after-free crashes. This should be changed to a strong reference like cppgc::Persistent to ensure the WebRequest object's lifetime. Examples: shell/browser/net/proxying_url_loader_factory.h [255] const cppgc::WeakPersistent<api::WebRequest> web_request_; shell/browser/net/proxying_websocket.h [127] const cppgc::WeakPersistent<api::WebRequest> web_request_; Solution Walkthrough: Before: // In ProxyingURLLoaderFactory.h class ProxyingURLLoaderFactory { private: const cppgc::WeakPersistent<api::WebRequest> web_request_; // ... }; // In ProxyingURLLoaderFactory.cc void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { // ... // CRASH: web_request_ may have been garbage collected, // and operator-> is not defined for WeakPersistent. int result = factory_->web_request_->OnBeforeRequest(...); // ... } After: // In ProxyingURLLoaderFactory.h class ProxyingURLLoaderFactory { private: // Use a strong reference to keep WebRequest alive. const cppgc::Persistent<api::WebRequest> web_request_; // ... }; // In ProxyingURLLoaderFactory.cc void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() { // ... // OK: web_request_ is a strong reference, so the object is guaranteed to exist. // The pointer is accessed via Get(). int result = factory_->web_request_.Get()->OnBeforeRequest(...); // ... } Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical use-after-free bug due to the misuse of cppgc::WeakPersistent for the WebRequest object, which could be garbage collected prematurely, leading to crashes.	High
Possible issue	Use correct weak reference type Replace cppgc::WeakPersistentapi::WebRequest with a safer weak reference type suitable for non-CppGC objects, such as gin::WeakCellapi::WebRequest, to prevent dangling pointers. shell/browser/net/proxying_url_loader_factory.h [255] -const cppgc::WeakPersistent<api::WebRequest> web_request_; +gin::WeakCell<api::WebRequest> web_request_; Apply / Chat Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical memory safety issue where a non-GC-managed class (ProxyingURLLoaderFactory) uses cppgc::WeakPersistent, which can lead to a dangling pointer. The proposed fix to use a different weak reference mechanism is appropriate and necessary.	High
	Trace weak factory in CppGC Add tracing for the weak_factory_ member in the WebRequest::Trace method to ensure correct garbage collection. shell/browser/api/electron_api_web_request.cc [354-356] void WebRequest::Trace(cppgc::Visitor* visitor) const { gin::Wrappable<WebRequest>::Trace(visitor); + visitor->Trace(weak_factory_); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug in the CppGC migration; failing to trace the weak_factory_ member will lead to incorrect garbage collection behavior, potentially causing memory leaks or use-after-free errors.	High
More