fix: access violation during redirects (intercepted requests) (#25393)

* fix: don't delete loader factory when request is pending

When intercepted request is pending we need to make sure that
loader factory is not deleted, especially when redirect occurs.
Otherwise, it may cause access violation.

* fix: added logic that removes requests from collection

* fix: fixed lint errors

* fix: fixed review remark

* fix: fixed review remarks

Removed intercepted_requests_ collection
and leverage pending_receivers_.

* fix: brought back removed line

shell/browser/net/electron_url_loader_factory.cc

@@ -200,11 +200,22 @@ void ElectronURLLoaderFactory::CreateLoaderAndStart(
     mojo::PendingRemote<network::mojom::URLLoaderClient> client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  handler_.Run(
+  mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory;
-      request,
+  handler_.Run(request, base::BindOnce(&ElectronURLLoaderFactory::StartLoading,
-      base::BindOnce(&ElectronURLLoaderFactory::StartLoading, std::move(loader),
+                                       std::move(loader), routing_id,
-                     routing_id, request_id, options, request,
+                                       request_id, options, request,
-                     std::move(client), traffic_annotation, nullptr, type_));
+                                       std::move(client), traffic_annotation,
+                                       std::move(proxy_factory), type_));
+}
+
+// static
+void ElectronURLLoaderFactory::OnComplete(
+    mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+    int32_t request_id,
+    const network::URLLoaderCompletionStatus& status) {
+  mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+      std::move(client));
+  client_remote->OnComplete(status);
 }
 
 // static
@@ -216,7 +227,7 @@ void ElectronURLLoaderFactory::StartLoading(
     const network::ResourceRequest& request,
     mojo::PendingRemote<network::mojom::URLLoaderClient> client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-    network::mojom::URLLoaderFactory* proxy_factory,
+    mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory,
     ProtocolType type,
     gin::Arguments* args) {
   // Send network error when there is no argument passed.
@@ -225,10 +236,8 @@ void ElectronURLLoaderFactory::StartLoading(
   // passed, to keep compatibility with old code.
   v8::Local<v8::Value> response;
   if (!args->GetNext(&response)) {
-    mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+    OnComplete(std::move(client), request_id,
-        std::move(client));
+               network::URLLoaderCompletionStatus(net::ERR_NOT_IMPLEMENTED));
-    client_remote->OnComplete(
-        network::URLLoaderCompletionStatus(net::ERR_NOT_IMPLEMENTED));
     return;
   }
 
@@ -237,9 +246,8 @@ void ElectronURLLoaderFactory::StartLoading(
   if (!dict.IsEmpty()) {
     int error_code;
     if (dict.Get("error", &error_code)) {
-      mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+      OnComplete(std::move(client), request_id,
-          std::move(client));
+                 network::URLLoaderCompletionStatus(error_code));
-      client_remote->OnComplete(network::URLLoaderCompletionStatus(error_code));
       return;
     }
   }
@@ -286,10 +294,14 @@ void ElectronURLLoaderFactory::StartLoading(
     // module.
     //
     // I'm not sure whether this is an intended behavior in Chromium.
-    if (proxy_factory) {
+    mojo::Remote<network::mojom::URLLoaderFactory> proxy_factory_remote(
-      proxy_factory->CreateLoaderAndStart(
+        std::move(proxy_factory));
+
+    if (proxy_factory_remote.is_bound()) {
+      proxy_factory_remote->CreateLoaderAndStart(
           std::move(loader), routing_id, request_id, options, new_request,
           std::move(client), traffic_annotation);
+      proxy_factory = proxy_factory_remote.Unbind();
     } else {
       StartLoadingHttp(std::move(loader), new_request, std::move(client),
                        traffic_annotation,
@@ -300,10 +312,8 @@ void ElectronURLLoaderFactory::StartLoading(
 
   // Some protocol accepts non-object responses.
   if (dict.IsEmpty() && ResponseMustBeObject(type)) {
-    mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+    OnComplete(std::move(client), request_id,
-        std::move(client));
+               network::URLLoaderCompletionStatus(net::ERR_NOT_IMPLEMENTED));
-    client_remote->OnComplete(
-        network::URLLoaderCompletionStatus(net::ERR_NOT_IMPLEMENTED));
     return;
   }
 
@@ -330,15 +340,13 @@ void ElectronURLLoaderFactory::StartLoading(
     case ProtocolType::kFree:
       ProtocolType type;
       if (!gin::ConvertFromV8(args->isolate(), response, &type)) {
-        mojo::Remote<network::mojom::URLLoaderClient> client_remote(
+        OnComplete(std::move(client), request_id,
-            std::move(client));
+                   network::URLLoaderCompletionStatus(net::ERR_FAILED));
-        client_remote->OnComplete(
-            network::URLLoaderCompletionStatus(net::ERR_FAILED));
         return;
       }
       StartLoading(std::move(loader), routing_id, request_id, options, request,
-                   std::move(client), traffic_annotation, proxy_factory, type,
+                   std::move(client), traffic_annotation,
-                   args);
+                   std::move(proxy_factory), type, args);
       break;
   }
 }

shell/browser/net/electron_url_loader_factory.h

@@ -66,7 +66,7 @@ class ElectronURLLoaderFactory
       const network::ResourceRequest& request,
       mojo::PendingRemote<network::mojom::URLLoaderClient> client,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-      network::mojom::URLLoaderFactory* proxy_factory,
+      mojo::PendingRemote<network::mojom::URLLoaderFactory> proxy_factory,
       ProtocolType type,
       gin::Arguments* args);
 
@@ -77,6 +77,10 @@ class ElectronURLLoaderFactory
       mojo::PendingReceiver<network::mojom::URLLoaderFactory> factory_receiver);
   ~ElectronURLLoaderFactory() override;
 
+  static void OnComplete(
+      mojo::PendingRemote<network::mojom::URLLoaderClient> client,
+      int32_t request_id,
+      const network::URLLoaderCompletionStatus& status);
   static void StartLoadingBuffer(
       mojo::PendingRemote<network::mojom::URLLoaderClient> client,
       network::mojom::URLResponseHeadPtr head,

shell/browser/net/proxying_url_loader_factory.cc

@@ -801,12 +801,16 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
   // Check if user has intercepted this scheme.
   auto it = intercepted_handlers_.find(request.url.scheme());
   if (it != intercepted_handlers_.end()) {
+    mojo::Remote<network::mojom::URLLoaderFactory> loader_remote;
+    this->Clone(loader_remote.BindNewPipeAndPassReceiver());
+
     // <scheme, <type, handler>>
     it->second.second.Run(
-        request, base::BindOnce(&ElectronURLLoaderFactory::StartLoading,
+        request,
-                                std::move(loader), routing_id, request_id,
+        base::BindOnce(&ElectronURLLoaderFactory::StartLoading,
-                                options, request, std::move(client),
+                       std::move(loader), routing_id, request_id, options,
-                                traffic_annotation, this, it->second.first));
+                       request, std::move(client), traffic_annotation,
+                       loader_remote.Unbind(), it->second.first));
     return;
   }
 
@@ -896,7 +900,8 @@ void ProxyingURLLoaderFactory::RemoveRequest(int32_t network_service_request_id,
 void ProxyingURLLoaderFactory::MaybeDeleteThis() {
   // Even if all URLLoaderFactory pipes connected to this object have been
   // closed it has to stay alive until all active requests have completed.
-  if (target_factory_.is_bound() || !requests_.empty())
+  if (target_factory_.is_bound() || !requests_.empty() ||
+      !proxy_receivers_.empty())
     return;
 
   delete this;