chore: pull ProxyingURLLoaderFactory closer to upstream class it mirrors (#29486)

* chore: pull ProxyingURLLoaderFactory closer to upstream class it mirrors

* chore: add another change which was accepted upstream

shell/browser/net/proxying_url_loader_factory.cc

@@ -4,8 +4,11 @@
 
 #include "shell/browser/net/proxying_url_loader_factory.h"
 
+#include <memory>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -14,12 +17,15 @@
 #include "extensions/browser/extension_navigation_ui_data.h"
 #include "net/base/completion_repeating_callback.h"
 #include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/http/http_util.h"
+#include "net/url_request/redirect_info.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 #include "services/network/public/cpp/features.h"
 #include "shell/browser/net/asar/asar_url_loader.h"
 #include "shell/common/options_switches.h"
+#include "url/origin.h"
 
 namespace electron {
 
@@ -43,9 +49,9 @@ ProxyingURLLoaderFactory::InProgressRequest::InProgressRequest(
       request_(request),
       original_initiator_(request.request_initiator),
       request_id_(web_request_id),
+      network_service_request_id_(network_service_request_id),
       view_routing_id_(view_routing_id),
       frame_routing_id_(frame_routing_id),
-      network_service_request_id_(network_service_request_id),
       options_(options),
       traffic_annotation_(traffic_annotation),
       proxied_loader_receiver_(this, std::move(loader_receiver)),
@@ -159,7 +165,7 @@ void ProxyingURLLoaderFactory::InProgressRequest::RestartInternal() {
     // they respond. |continuation| above will be invoked asynchronously to
     // continue or cancel the request.
     //
-    // We pause the binding here to prevent further client message processing.
+    // We pause the receiver here to prevent further client message processing.
     if (proxied_client_receiver_.is_bound())
       proxied_client_receiver_.Pause();
 
@@ -229,6 +235,11 @@ void ProxyingURLLoaderFactory::InProgressRequest::ResumeReadingBodyFromNet() {
     target_loader_->ResumeReadingBodyFromNet();
 }
 
+void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveEarlyHints(
+    network::mojom::EarlyHintsPtr early_hints) {
+  target_client_->OnReceiveEarlyHints(std::move(early_hints));
+}
+
 void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveResponse(
     network::mojom::URLResponseHeadPtr head) {
   if (current_request_uses_header_client_) {
@@ -246,11 +257,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveResponse(
   }
 }
 
-void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveEarlyHints(
-    network::mojom::EarlyHintsPtr early_hints) {
-  target_client_->OnReceiveEarlyHints(std::move(early_hints));
-}
-
 void ProxyingURLLoaderFactory::InProgressRequest::OnReceiveRedirect(
     const net::RedirectInfo& redirect_info,
     network::mojom::URLResponseHeadPtr head) {
@@ -313,12 +319,17 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnComplete(
   factory_->RemoveRequest(network_service_request_id_, request_id_);
 }
 
+bool ProxyingURLLoaderFactory::IsForServiceWorkerScript() const {
+  return loader_factory_type_ == content::ContentBrowserClient::
+                                     URLLoaderFactoryType::kServiceWorkerScript;
+}
+
 void ProxyingURLLoaderFactory::InProgressRequest::OnLoaderCreated(
     mojo::PendingReceiver<network::mojom::TrustedHeaderClient> receiver) {
   header_client_receiver_.Bind(std::move(receiver));
   if (for_cors_preflight_) {
     // In this case we don't have |target_loader_| and
-    // |proxied_client_binding_|, and |receiver| is the only connection to the
+    // |proxied_client_receiver_|, and |receiver| is the only connection to the
     // network service, so we observe mojo connection errors.
     header_client_receiver_.set_disconnect_handler(base::BindOnce(
         &ProxyingURLLoaderFactory::InProgressRequest::OnRequestError,
@@ -365,25 +376,57 @@ void ProxyingURLLoaderFactory::InProgressRequest::OnHeadersReceived(
                      weak_factory_.GetWeakPtr()));
 }
 
-void ProxyingURLLoaderFactory::OnLoaderForCorsPreflightCreated(
-    const network::ResourceRequest& request,
-    mojo::PendingReceiver<network::mojom::TrustedHeaderClient> receiver) {
-  // Please note that the URLLoader is now starting, without waiting for
-  // additional signals from here. The URLLoader will be blocked before
-  // sending HTTP request headers (TrustedHeaderClient.OnBeforeSendHeaders),
-  // but the connection set up will be done before that. This is acceptable from
-  // Web Request API because the extension has already allowed to set up
-  // a connection to the same URL (i.e., the actual request), and distinguishing
-  // two connections for the actual request and the preflight request before
-  // sending request headers is very difficult.
-  const uint64_t web_request_id = ++(*request_id_generator_);
+void ProxyingURLLoaderFactory::InProgressRequest::
+    HandleBeforeRequestRedirect() {
+  // The extension requested a redirect. Close the connection with the current
+  // URLLoader and inform the URLLoaderClient the WebRequest API generated a
+  // redirect. To load |redirect_url_|, a new URLLoader will be recreated
+  // after receiving FollowRedirect().
 
-  auto result = requests_.insert(std::make_pair(
-      web_request_id, std::make_unique<InProgressRequest>(
-                          this, web_request_id, frame_routing_id_, request)));
+  // Forgetting to close the connection with the current URLLoader caused
+  // bugs. The latter doesn't know anything about the redirect. Continuing
+  // the load with it gives unexpected results. See
+  // https://crbug.com/882661#c72.
+  proxied_client_receiver_.reset();
+  header_client_receiver_.reset();
+  target_loader_.reset();
 
-  result.first->second->OnLoaderCreated(std::move(receiver));
-  result.first->second->Restart();
+  constexpr int kInternalRedirectStatusCode = net::HTTP_TEMPORARY_REDIRECT;
+
+  net::RedirectInfo redirect_info;
+  redirect_info.status_code = kInternalRedirectStatusCode;
+  redirect_info.new_method = request_.method;
+  redirect_info.new_url = redirect_url_;
+  redirect_info.new_site_for_cookies =
+      net::SiteForCookies::FromUrl(redirect_url_);
+
+  auto head = network::mojom::URLResponseHead::New();
+  std::string headers = base::StringPrintf(
+      "HTTP/1.1 %i Internal Redirect\n"
+      "Location: %s\n"
+      "Non-Authoritative-Reason: WebRequest API\n\n",
+      kInternalRedirectStatusCode, redirect_url_.spec().c_str());
+
+  // Cross-origin requests need to modify the Origin header to 'null'. Since
+  // CorsURLLoader sets |request_initiator| to the Origin request header in
+  // NetworkService, we need to modify |request_initiator| here to craft the
+  // Origin header indirectly.
+  // Following checks implement the step 10 of "4.4. HTTP-redirect fetch",
+  // https://fetch.spec.whatwg.org/#http-redirect-fetch
+  if (request_.request_initiator &&
+      (!url::Origin::Create(redirect_url_)
+            .IsSameOriginWith(url::Origin::Create(request_.url)) &&
+       !request_.request_initiator->IsSameOriginWith(
+           url::Origin::Create(request_.url)))) {
+    // Reset the initiator to pretend tainted origin flag of the spec is set.
+    request_.request_initiator = url::Origin();
+  }
+  head->headers = base::MakeRefCounted<net::HttpResponseHeaders>(
+      net::HttpUtil::AssembleRawHeaders(headers));
+  head->encoded_data_length = 0;
+
+  current_response_ = std::move(head);
+  ContinueToBeforeRedirect(redirect_info, net::OK);
 }
 
 void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders(
@@ -435,6 +478,50 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeSendHeaders(
                         net::OK);
 }
 
+void ProxyingURLLoaderFactory::InProgressRequest::ContinueToStartRequest(
+    int error_code) {
+  if (error_code != net::OK) {
+    OnRequestError(network::URLLoaderCompletionStatus(error_code));
+    return;
+  }
+
+  if (current_request_uses_header_client_ && !redirect_url_.is_empty()) {
+    HandleBeforeRequestRedirect();
+    return;
+  }
+
+  if (proxied_client_receiver_.is_bound())
+    proxied_client_receiver_.Resume();
+
+  if (header_client_receiver_.is_bound())
+    header_client_receiver_.Resume();
+
+  if (for_cors_preflight_) {
+    // For CORS preflight requests, we have already started the request in
+    // the network service. We did block the request by blocking
+    // |header_client_receiver_|, which we unblocked right above.
+    return;
+  }
+
+  if (!target_loader_.is_bound() && factory_->target_factory_.is_bound()) {
+    // No extensions have cancelled us up to this point, so it's now OK to
+    // initiate the real network request.
+    uint32_t options = options_;
+    // Even if this request does not use the header client, future redirects
+    // might, so we need to set the option on the loader.
+    if (has_any_extra_headers_listeners_)
+      options |= network::mojom::kURLLoadOptionUseHeaderClient;
+    factory_->target_factory_->CreateLoaderAndStart(
+        mojo::MakeRequest(&target_loader_), network_service_request_id_,
+        options, request_, proxied_client_receiver_.BindNewPipeAndPassRemote(),
+        traffic_annotation_);
+  }
+
+  // From here the lifecycle of this request is driven by subsequent events on
+  // either |proxied_loader_receiver_|, |proxied_client_receiver_|, or
+  // |header_client_receiver_|.
+}
+
 void ProxyingURLLoaderFactory::InProgressRequest::ContinueToSendHeaders(
     const std::set<std::string>& removed_headers,
     const std::set<std::string>& set_headers,
@@ -485,50 +572,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToSendHeaders(
     ContinueToStartRequest(net::OK);
 }
 
-void ProxyingURLLoaderFactory::InProgressRequest::ContinueToStartRequest(
-    int error_code) {
-  if (error_code != net::OK) {
-    OnRequestError(network::URLLoaderCompletionStatus(error_code));
-    return;
-  }
-
-  if (current_request_uses_header_client_ && !redirect_url_.is_empty()) {
-    HandleBeforeRequestRedirect();
-    return;
-  }
-
-  if (proxied_client_receiver_.is_bound())
-    proxied_client_receiver_.Resume();
-
-  if (header_client_receiver_.is_bound())
-    header_client_receiver_.Resume();
-
-  if (for_cors_preflight_) {
-    // For CORS preflight requests, we have already started the request in
-    // the network service. We did block the request by blocking
-    // |header_client_receiver_|, which we unblocked right above.
-    return;
-  }
-
-  if (!target_loader_.is_bound() && factory_->target_factory_.is_bound()) {
-    // No extensions have cancelled us up to this point, so it's now OK to
-    // initiate the real network request.
-    uint32_t options = options_;
-    // Even if this request does not use the header client, future redirects
-    // might, so we need to set the option on the loader.
-    if (has_any_extra_headers_listeners_)
-      options |= network::mojom::kURLLoadOptionUseHeaderClient;
-    factory_->target_factory_->CreateLoaderAndStart(
-        mojo::MakeRequest(&target_loader_), network_service_request_id_,
-        options, request_, proxied_client_receiver_.BindNewPipeAndPassRemote(),
-        traffic_annotation_);
-  }
-
-  // From here the lifecycle of this request is driven by subsequent events on
-  // either |proxy_loader_binding_|, |proxy_client_receiver_|, or
-  // |header_client_receiver_|.
-}
-
 void ProxyingURLLoaderFactory::InProgressRequest::
     ContinueToHandleOverrideHeaders(int error_code) {
   if (error_code != net::OK) {
@@ -648,59 +691,6 @@ void ProxyingURLLoaderFactory::InProgressRequest::ContinueToBeforeRedirect(
     request_.request_body = nullptr;
 }
 
-void ProxyingURLLoaderFactory::InProgressRequest::
-    HandleBeforeRequestRedirect() {
-  // The extension requested a redirect. Close the connection with the current
-  // URLLoader and inform the URLLoaderClient the WebRequest API generated a
-  // redirect. To load |redirect_url_|, a new URLLoader will be recreated
-  // after receiving FollowRedirect().
-
-  // Forgetting to close the connection with the current URLLoader caused
-  // bugs. The latter doesn't know anything about the redirect. Continuing
-  // the load with it gives unexpected results. See
-  // https://crbug.com/882661#c72.
-  proxied_client_receiver_.reset();
-  header_client_receiver_.reset();
-  target_loader_.reset();
-
-  constexpr int kInternalRedirectStatusCode = net::HTTP_TEMPORARY_REDIRECT;
-
-  net::RedirectInfo redirect_info;
-  redirect_info.status_code = kInternalRedirectStatusCode;
-  redirect_info.new_method = request_.method;
-  redirect_info.new_url = redirect_url_;
-  redirect_info.new_site_for_cookies =
-      net::SiteForCookies::FromUrl(redirect_url_);
-
-  auto head = network::mojom::URLResponseHead::New();
-  std::string headers = base::StringPrintf(
-      "HTTP/1.1 %i Internal Redirect\n"
-      "Location: %s\n"
-      "Non-Authoritative-Reason: WebRequest API\n\n",
-      kInternalRedirectStatusCode, redirect_url_.spec().c_str());
-
-  // Cross-origin requests need to modify the Origin header to 'null'. Since
-  // CorsURLLoader sets |request_initiator| to the Origin request header in
-  // NetworkService, we need to modify |request_initiator| here to craft the
-  // Origin header indirectly.
-  // Following checks implement the step 10 of "4.4. HTTP-redirect fetch",
-  // https://fetch.spec.whatwg.org/#http-redirect-fetch
-  if (request_.request_initiator &&
-      (!url::Origin::Create(redirect_url_)
-            .IsSameOriginWith(url::Origin::Create(request_.url)) &&
-       !request_.request_initiator->IsSameOriginWith(
-           url::Origin::Create(request_.url)))) {
-    // Reset the initiator to pretend tainted origin flag of the spec is set.
-    request_.request_initiator = url::Origin();
-  }
-  head->headers = base::MakeRefCounted<net::HttpResponseHeaders>(
-      net::HttpUtil::AssembleRawHeaders(headers));
-  head->encoded_data_length = 0;
-
-  current_response_ = std::move(head);
-  ContinueToBeforeRedirect(redirect_info, net::OK);
-}
-
 void ProxyingURLLoaderFactory::InProgressRequest::
     HandleResponseOrRedirectHeaders(net::CompletionOnceCallback continuation) {
   override_headers_ = nullptr;
@@ -785,8 +775,6 @@ ProxyingURLLoaderFactory::ProxyingURLLoaderFactory(
       ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
 }
 
-ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default;
-
 bool ProxyingURLLoaderFactory::ShouldIgnoreConnectionsLimit(
     const network::ResourceRequest& request) {
   for (const auto& domain : ignore_connections_limit_domains_) {
@@ -884,11 +872,29 @@ void ProxyingURLLoaderFactory::OnLoaderCreated(
   request_it->second->OnLoaderCreated(std::move(receiver));
 }
 
-bool ProxyingURLLoaderFactory::IsForServiceWorkerScript() const {
-  return loader_factory_type_ == content::ContentBrowserClient::
-                                     URLLoaderFactoryType::kServiceWorkerScript;
+void ProxyingURLLoaderFactory::OnLoaderForCorsPreflightCreated(
+    const network::ResourceRequest& request,
+    mojo::PendingReceiver<network::mojom::TrustedHeaderClient> receiver) {
+  // Please note that the URLLoader is now starting, without waiting for
+  // additional signals from here. The URLLoader will be blocked before
+  // sending HTTP request headers (TrustedHeaderClient.OnBeforeSendHeaders),
+  // but the connection set up will be done before that. This is acceptable from
+  // Web Request API because the extension has already allowed to set up
+  // a connection to the same URL (i.e., the actual request), and distinguishing
+  // two connections for the actual request and the preflight request before
+  // sending request headers is very difficult.
+  const uint64_t web_request_id = ++(*request_id_generator_);
+
+  auto result = requests_.insert(std::make_pair(
+      web_request_id, std::make_unique<InProgressRequest>(
+                          this, web_request_id, frame_routing_id_, request)));
+
+  result.first->second->OnLoaderCreated(std::move(receiver));
+  result.first->second->Restart();
 }
 
+ProxyingURLLoaderFactory::~ProxyingURLLoaderFactory() = default;
+
 void ProxyingURLLoaderFactory::OnTargetFactoryError() {
   target_factory_.reset();
   proxy_receivers_.Clear();

shell/browser/net/proxying_url_loader_factory.h

@@ -5,27 +5,34 @@
 #ifndef SHELL_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_
 #define SHELL_BROWSER_NET_PROXYING_URL_LOADER_FACTORY_H_
 
+#include <cstdint>
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_frame_host.h"
 #include "extensions/browser/api/web_request/web_request_info.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "net/base/completion_once_callback.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 #include "services/network/public/mojom/url_response_head.mojom.h"
-#include "shell/browser/api/electron_api_web_request.h"
 #include "shell/browser/net/electron_url_loader_factory.h"
 #include "shell/browser/net/web_request_api_interface.h"
 #include "third_party/abseil-cpp/absl/types/optional.h"
+#include "url/gurl.h"
 
 namespace electron {
 
@@ -43,7 +50,7 @@ class ProxyingURLLoaderFactory
                             public network::mojom::URLLoaderClient,
                             public network::mojom::TrustedHeaderClient {
    public:
-    // For usual requests.
+    // For usual requests
     InProgressRequest(
         ProxyingURLLoaderFactory* factory,
         int64_t web_request_id,
@@ -55,7 +62,7 @@ class ProxyingURLLoaderFactory
         const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
         mojo::PendingReceiver<network::mojom::URLLoader> loader_receiver,
         mojo::PendingRemote<network::mojom::URLLoaderClient> client);
-    // For CORS preflights.
+    // For CORS preflights
     InProgressRequest(ProxyingURLLoaderFactory* factory,
                       uint64_t request_id,
                       int32_t frame_routing_id,
@@ -76,9 +83,9 @@ class ProxyingURLLoaderFactory
     void ResumeReadingBodyFromNet() override;
 
     // network::mojom::URLLoaderClient:
-    void OnReceiveResponse(network::mojom::URLResponseHeadPtr head) override;
     void OnReceiveEarlyHints(
         network::mojom::EarlyHintsPtr early_hints) override;
+    void OnReceiveResponse(network::mojom::URLResponseHeadPtr head) override;
     void OnReceiveRedirect(const net::RedirectInfo& redirect_info,
                            network::mojom::URLResponseHeadPtr head) override;
     void OnUploadProgress(int64_t current_position,
@@ -114,18 +121,18 @@ class ProxyingURLLoaderFactory
     void ContinueToResponseStarted(int error_code);
     void ContinueToBeforeRedirect(const net::RedirectInfo& redirect_info,
                                   int error_code);
-    void HandleBeforeRequestRedirect();
     void HandleResponseOrRedirectHeaders(
         net::CompletionOnceCallback continuation);
     void OnRequestError(const network::URLLoaderCompletionStatus& status);
+    void HandleBeforeRequestRedirect();
 
-    ProxyingURLLoaderFactory* factory_;
+    ProxyingURLLoaderFactory* const factory_;
     network::ResourceRequest request_;
     const absl::optional<url::Origin> original_initiator_;
     const uint64_t request_id_ = 0;
+    const int32_t network_service_request_id_ = 0;
     const int32_t view_routing_id_ = MSG_ROUTING_NONE;
     const int32_t frame_routing_id_ = MSG_ROUTING_NONE;
-    const int32_t network_service_request_id_ = 0;
     const uint32_t options_ = 0;
     const net::MutableNetworkTrafficAnnotationTag traffic_annotation_;
     mojo::Receiver<network::mojom::URLLoader> proxied_loader_receiver_;
@@ -133,14 +140,14 @@ class ProxyingURLLoaderFactory
 
     absl::optional<extensions::WebRequestInfo> info_;
 
-    network::mojom::URLResponseHeadPtr current_response_;
-    scoped_refptr<net::HttpResponseHeaders> override_headers_;
-    GURL redirect_url_;
-
     mojo::Receiver<network::mojom::URLLoaderClient> proxied_client_receiver_{
         this};
     network::mojom::URLLoaderPtr target_loader_;
 
+    network::mojom::URLResponseHeadPtr current_response_;
+    scoped_refptr<net::HttpResponseHeaders> override_headers_;
+    GURL redirect_url_;
+
     const bool for_cors_preflight_ = false;
 
     // If |has_any_extra_headers_listeners_| is set to true, the request will be
@@ -192,6 +199,7 @@ class ProxyingURLLoaderFactory
       mojo::PendingReceiver<network::mojom::TrustedURLLoaderHeaderClient>
           header_client_receiver,
       content::ContentBrowserClient::URLLoaderFactoryType loader_factory_type);
+
   ~ProxyingURLLoaderFactory() override;
 
   // network::mojom::URLLoaderFactory: