network: Extract FileOpenerForUpload from url_loader.cc

This CL extracts the inner class `URLLoader::FileOpenerForUpload` into a
standalone class `network::FileOpenerForUpload` located in its own
header and source files (`services/network/file_opener_for_upload.h`
and `.cc`).

The motivation for this change is to improve modularity and decouple
the logic for asynchronously opening files for upload from the main
`URLLoader` class. This makes both classes simpler and easier to
maintain.

Key changes include:
- Moved the `FileOpenerForUpload` class definition and implementation
  to new files.
- Updated `URLLoader` to include and use the new standalone class.
  - The request URL is now explicitly passed to the
    `FileOpenerForUpload` constructor.
  - The `LogUnblocked` call in `SetUpUpload` is now correctly tied
    to the callback originating from `FileOpenerForUpload`.
- Removed the inner class definition and friend declaration from
  `url_loader.h`.
- Added the new files to the `network_service` component build target.
- Updated `url_loader_unittest.cc` to include the new header and
  use the correct scope for `kMaxFileUploadRequestsPerBatch`.

This CL also changes the TaskPriority which is used for closing the
files from USER_BLOCKING to USER_VISIBLE.
Other than that, no functional changes are intended.

Bug: 408106280
Change-Id: Ia07b367787adcc65df4afd13ccb8e0ff49a274e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6428886
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1443881}

services/network/BUILD.gn

@@ -46,6 +46,8 @@ component("network_service") {
     "disk_cache/mojo_backend_file_operations_factory.h",
     "dns_config_change_manager.cc",
     "dns_config_change_manager.h",
+    "file_opener_for_upload.cc",
+    "file_opener_for_upload.h",
     "host_resolver.cc",
     "host_resolver.h",
     "host_resolver_mdns_listener.cc",

services/network/file_opener_for_upload.cc

@@ -0,0 +1,104 @@
+// Copyright 2025 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/network/file_opener_for_upload.h"
+
+#include "base/task/thread_pool.h"
+#include "net/base/net_errors.h"
+#include "services/network/public/mojom/network_context_client.mojom.h"
+
+namespace network {
+
+namespace {
+
+// `opened_files` need to be closed on a blocking task runner, so move the
+// `opened_files` vector onto a sequence that can block so it gets destroyed
+// there.
+void PostCloseFiles(std::vector<base::File> opened_files) {
+  base::ThreadPool::PostTask(
+      FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_VISIBLE},
+      base::DoNothingWithBoundArgs(std::move(opened_files)));
+}
+
+}  // namespace
+
+FileOpenerForUpload::FileOpenerForUpload(
+    std::vector<base::FilePath> paths,
+    const GURL& url,
+    int32_t process_id,
+    mojom::NetworkContextClient* const network_context_client,
+    SetUpUploadCallback set_up_upload_callback)
+    : paths_(std::move(paths)),
+      url_(url),
+      process_id_(process_id),
+      network_context_client_(network_context_client),
+      set_up_upload_callback_(std::move(set_up_upload_callback)) {}
+
+FileOpenerForUpload::~FileOpenerForUpload() {
+  if (!opened_files_.empty()) {
+    PostCloseFiles(std::move(opened_files_));
+  }
+}
+
+void FileOpenerForUpload::Start() {
+  opened_files_.reserve(paths_.size());
+  StartOpeningNextBatch();
+}
+
+// static
+void FileOpenerForUpload::OnFilesForUploadOpened(
+    base::WeakPtr<FileOpenerForUpload> file_opener,
+    size_t num_files_requested,
+    int error_code,
+    std::vector<base::File> opened_files) {
+  if (!file_opener) {
+    PostCloseFiles(std::move(opened_files));
+    return;
+  }
+
+  if (error_code == net::OK && num_files_requested != opened_files.size()) {
+    error_code = net::ERR_FAILED;
+  }
+
+  if (error_code != net::OK) {
+    PostCloseFiles(std::move(opened_files));
+    file_opener->FilesForUploadOpenedDone(error_code);
+    return;
+  }
+  file_opener->opened_files_.insert(
+      file_opener->opened_files_.end(),
+      std::make_move_iterator(opened_files.begin()),
+      std::make_move_iterator(opened_files.end()));
+
+  if (file_opener->opened_files_.size() < file_opener->paths_.size()) {
+    file_opener->StartOpeningNextBatch();
+    return;
+  }
+
+  file_opener->FilesForUploadOpenedDone(net::OK);
+}
+
+void FileOpenerForUpload::StartOpeningNextBatch() {
+  size_t num_files_to_request = std::min(paths_.size() - opened_files_.size(),
+                                         kMaxFileUploadRequestsPerBatch);
+  std::vector<base::FilePath> batch_paths(
+      paths_.begin() + opened_files_.size(),
+      paths_.begin() + opened_files_.size() + num_files_to_request);
+
+  network_context_client_->OnFileUploadRequested(
+      process_id_, /*async=*/true, batch_paths, url_.get(),
+      base::BindOnce(&FileOpenerForUpload::OnFilesForUploadOpened,
+                     weak_ptr_factory_.GetWeakPtr(), num_files_to_request));
+}
+
+void FileOpenerForUpload::FilesForUploadOpenedDone(int error_code) {
+  if (error_code == net::OK) {
+    std::move(set_up_upload_callback_).Run(std::move(opened_files_));
+  } else {
+    std::move(set_up_upload_callback_)
+        .Run(base::unexpected(static_cast<net::Error>(error_code)));
+  }
+}
+
+}  // namespace network

services/network/file_opener_for_upload.h

@@ -0,0 +1,87 @@
+// Copyright 2025 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_NETWORK_FILE_OPENER_FOR_UPLOAD_H_
+#define SERVICES_NETWORK_FILE_OPENER_FOR_UPLOAD_H_
+
+#include <memory>
+#include <optional>
+#include <vector>
+
+#include "base/files/file.h"
+#include "base/functional/callback.h"
+#include "base/memory/raw_ref.h"
+#include "base/memory/weak_ptr.h"
+#include "base/types/expected.h"
+#include "net/base/net_errors.h"
+
+class GURL;
+
+namespace network {
+namespace mojom {
+class NetworkContextClient;
+}  // namespace mojom
+
+// Manages opening a list of files specified by `paths_`. This is used by
+// URLLoader to prepare files listed in a ResourceRequestBody for upload. It
+// interacts with the NetworkContextClient to request file access and opens
+// files in batches to avoid overwhelming the system.
+class FileOpenerForUpload {
+ public:
+  // Callback type invoked when all files have been processed (successfully or
+  // with an error).
+  using SetUpUploadCallback = base::OnceCallback<void(
+      base::expected<std::vector<base::File>, net::Error>)>;
+
+  // Maximum number of file open requests sent to the NetworkContextClient
+  // in a single batch via OnFileUploadRequested.
+  static constexpr size_t kMaxFileUploadRequestsPerBatch = 64;
+
+  // Constructor initiates the file opening process.
+  FileOpenerForUpload(std::vector<base::FilePath> paths,
+                      const GURL& url,
+                      int32_t process_id,
+                      mojom::NetworkContextClient* const network_context_client,
+                      SetUpUploadCallback set_up_upload_callback);
+
+  FileOpenerForUpload(const FileOpenerForUpload&) = delete;
+  FileOpenerForUpload& operator=(const FileOpenerForUpload&) = delete;
+
+  ~FileOpenerForUpload();
+
+  // Starts the file opening operations.
+  void Start();
+
+ private:
+  static void OnFilesForUploadOpened(
+      base::WeakPtr<FileOpenerForUpload> file_opener,
+      size_t num_files_requested,
+      int error_code,
+      std::vector<base::File> opened_files);
+
+  // Initiates the process of opening the next batch of files from `paths_`.
+  void StartOpeningNextBatch();
+
+  // Called when all batches have been processed or an error occurred.
+  // Invokes the final `set_up_upload_callback_`.
+  void FilesForUploadOpenedDone(int error_code);
+
+  // The list of file paths requested to be opened.
+  const std::vector<base::FilePath> paths_;
+  // The URL associated with the upload request (used in OnFileUploadRequested).
+  // Stored as a raw_ref as the GURL's lifetime is expected to exceed this
+  // object.
+  const raw_ref<const GURL> url_;
+  const int32_t process_id_;
+  const raw_ptr<mojom::NetworkContextClient> network_context_client_;
+  SetUpUploadCallback set_up_upload_callback_;
+  // The files opened so far.
+  std::vector<base::File> opened_files_;
+
+  base::WeakPtrFactory<FileOpenerForUpload> weak_ptr_factory_{this};
+};
+
+}  // namespace network
+
+#endif  // SERVICES_NETWORK_FILE_OPENER_FOR_UPLOAD_H_

services/network/url_loader.cc

@@ -81,6 +81,7 @@
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "services/network/ad_heuristic_cookie_overrides.h"
+#include "services/network/file_opener_for_upload.h"
 #include "services/network/orb/orb_impl.h"
 #include "services/network/public/cpp/client_hints.h"
 #include "services/network/public/cpp/constants.h"
@@ -618,109 +619,6 @@ void URLLoader::SetUpUrlRequestCallbacks(
   }
 }
 
-// This class is used to manage the queue of pending file upload operations
-// initiated by the URLLoader::OpenFilesForUpload().
-class URLLoader::FileOpenerForUpload {
- public:
-  typedef base::OnceCallback<void(int, std::vector<base::File>)>
-      SetUpUploadCallback;
-
-  FileOpenerForUpload(std::vector<base::FilePath> paths,
-                      URLLoader* url_loader,
-                      int32_t process_id,
-                      mojom::NetworkContextClient* const network_context_client,
-                      SetUpUploadCallback set_up_upload_callback)
-      : paths_(std::move(paths)),
-        url_loader_(url_loader),
-        process_id_(process_id),
-        network_context_client_(network_context_client),
-        set_up_upload_callback_(std::move(set_up_upload_callback)) {
-    StartOpeningNextBatch();
-  }
-
-  FileOpenerForUpload(const FileOpenerForUpload&) = delete;
-  FileOpenerForUpload& operator=(const FileOpenerForUpload&) = delete;
-
-  ~FileOpenerForUpload() {
-    if (!opened_files_.empty())
-      PostCloseFiles(std::move(opened_files_));
-  }
-
- private:
-  static void OnFilesForUploadOpened(
-      base::WeakPtr<FileOpenerForUpload> file_opener,
-      size_t num_files_requested,
-      int error_code,
-      std::vector<base::File> opened_files) {
-    if (!file_opener) {
-      PostCloseFiles(std::move(opened_files));
-      return;
-    }
-
-    if (error_code == net::OK && num_files_requested != opened_files.size())
-      error_code = net::ERR_FAILED;
-
-    if (error_code != net::OK) {
-      PostCloseFiles(std::move(opened_files));
-      file_opener->FilesForUploadOpenedDone(error_code);
-      return;
-    }
-
-    for (base::File& file : opened_files)
-      file_opener->opened_files_.push_back(std::move(file));
-
-    if (file_opener->opened_files_.size() < file_opener->paths_.size()) {
-      file_opener->StartOpeningNextBatch();
-      return;
-    }
-
-    file_opener->FilesForUploadOpenedDone(net::OK);
-  }
-
-  // |opened_files| need to be closed on a blocking task runner, so move the
-  // |opened_files| vector onto a sequence that can block so it gets destroyed
-  // there.
-  static void PostCloseFiles(std::vector<base::File> opened_files) {
-    base::ThreadPool::PostTask(
-        FROM_HERE, {base::MayBlock(), base::TaskPriority::USER_BLOCKING},
-        base::DoNothingWithBoundArgs(std::move(opened_files)));
-  }
-
-  void StartOpeningNextBatch() {
-    size_t num_files_to_request = std::min(paths_.size() - opened_files_.size(),
-                                           kMaxFileUploadRequestsPerBatch);
-    std::vector<base::FilePath> batch_paths(
-        paths_.begin() + opened_files_.size(),
-        paths_.begin() + opened_files_.size() + num_files_to_request);
-
-    network_context_client_->OnFileUploadRequested(
-        process_id_, /*async=*/true, batch_paths,
-        url_loader_->url_request_->url(),
-        base::BindOnce(&FileOpenerForUpload::OnFilesForUploadOpened,
-                       weak_ptr_factory_.GetWeakPtr(), num_files_to_request));
-  }
-
-  void FilesForUploadOpenedDone(int error_code) {
-    url_loader_->url_request_->LogUnblocked();
-
-    if (error_code == net::OK)
-      std::move(set_up_upload_callback_).Run(net::OK, std::move(opened_files_));
-    else
-      std::move(set_up_upload_callback_).Run(error_code, {});
-  }
-
-  // The paths of files for upload
-  const std::vector<base::FilePath> paths_;
-  const raw_ptr<URLLoader> url_loader_;
-  const int32_t process_id_;
-  const raw_ptr<mojom::NetworkContextClient> network_context_client_;
-  SetUpUploadCallback set_up_upload_callback_;
-  // The files opened so far.
-  std::vector<base::File> opened_files_;
-
-  base::WeakPtrFactory<FileOpenerForUpload> weak_ptr_factory_{this};
-};
-
 void URLLoader::OpenFilesForUpload(const ResourceRequest& request) {
   std::vector<base::FilePath> paths;
   for (const auto& element : *request.request_body.get()->elements()) {
@@ -729,7 +627,7 @@ void URLLoader::OpenFilesForUpload(const ResourceRequest& request) {
     }
   }
   if (paths.empty()) {
-    SetUpUpload(request, net::OK, std::vector<base::File>());
+    SetUpUpload(request, std::vector<base::File>());
     return;
   }
   if (!network_context_client_) {
@@ -745,28 +643,35 @@ void URLLoader::OpenFilesForUpload(const ResourceRequest& request) {
   }
   url_request_->LogBlockedBy("Opening Files");
   file_opener_for_upload_ = std::make_unique<FileOpenerForUpload>(
-      std::move(paths), this, factory_params_->process_id,
+      std::move(paths), url_request_->url(), factory_params_->process_id,
       network_context_client_,
       base::BindOnce(&URLLoader::SetUpUpload, base::Unretained(this), request));
+  file_opener_for_upload_->Start();
 }
 
-void URLLoader::SetUpUpload(const ResourceRequest& request,
-                            int error_code,
-                            std::vector<base::File> opened_files) {
-  if (error_code != net::OK) {
-    DCHECK(opened_files.empty());
+void URLLoader::SetUpUpload(
+    const ResourceRequest& request,
+    base::expected<std::vector<base::File>, net::Error> file_open_result) {
+  if (file_opener_for_upload_) {
+    file_opener_for_upload_.reset();
+    // This corresponds to the LogBlockedBy call made just before creating
+    // FileOpenerForUpload.
+    url_request_->LogUnblocked();
+  }
+  if (!file_open_result.has_value()) {
     // Defer calling NotifyCompleted to make sure the URLLoader finishes
     // initializing before getting deleted.
     base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
         FROM_HERE, base::BindOnce(&URLLoader::NotifyCompleted,
-                                  weak_ptr_factory_.GetWeakPtr(), error_code));
+                                  weak_ptr_factory_.GetWeakPtr(),
+                                  file_open_result.error()));
     return;
   }
   scoped_refptr<base::SequencedTaskRunner> task_runner =
       base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE});
   url_request_->set_upload(url_loader_util::CreateUploadDataStream(
-      request.request_body.get(), opened_files, task_runner.get()));
+      request.request_body.get(), file_open_result.value(), task_runner.get()));
 
   if (request.enable_upload_progress) {
     upload_progress_tracker_ = std::make_unique<UploadProgressTracker>(

services/network/url_loader.h

@@ -99,8 +99,7 @@ enum class FetchKeepAliveRequestNetworkMetricType {
 
 }  // namespace internal
 
-constexpr size_t kMaxFileUploadRequestsPerBatch = 64;
-
+class FileOpenerForUpload;
 class KeepaliveStatisticsRecorder;
 class NetToMojoPendingBuffer;
 class ScopedThrottlingToken;
@@ -327,9 +326,6 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
     const raw_ptr<URLLoader> pointer_;
   };
 
-  class FileOpenerForUpload;
-  friend class FileOpenerForUpload;
-
   // An enum class representing the result of keepalive requests. This is used
   // for UMA so do NOT re-assign values.
   enum class KeepaliveRequestResult {
@@ -347,8 +343,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
 
   void OpenFilesForUpload(const ResourceRequest& request);
   void SetUpUpload(const ResourceRequest& request,
-                   int error_code,
-                   const std::vector<base::File> opened_files);
+                   base::expected<std::vector<base::File>, net::Error>);
 
   // A continuation of `OnConnected` to process the result of the asynchronous
   // Local Network Access permission check.
@@ -708,6 +703,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
 
   mojo::Remote<mojom::TrustedHeaderClient> header_client_;
 
+  // Handles asynchronously opening files for upload. Holds a reference to the
+  // request's URL (from `url_request_`), so `url_request_` must outlive this.
   std::unique_ptr<FileOpenerForUpload> file_opener_for_upload_;
 
   // If the request is configured for Trust Tokens

services/network/url_loader_unittest.cc

@@ -99,6 +99,7 @@
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_test_job.h"
 #include "net/url_request/url_request_test_util.h"
+#include "services/network/file_opener_for_upload.h"
 #include "services/network/public/cpp/cors/origin_access_list.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/ip_address_space_util.h"
@@ -3155,7 +3156,7 @@ TEST_P(ParameterizedURLLoaderTest, UploadTwoBatchesOfFiles) {
   base::FilePath file_path = GetTestFilePath("simple_page.html");
 
   std::string expected_body;
-  size_t num_files = 2 * kMaxFileUploadRequestsPerBatch;
+  size_t num_files = 2 * FileOpenerForUpload::kMaxFileUploadRequestsPerBatch;
   for (size_t i = 0; i < num_files; ++i) {
     std::string tmp_expected_body;
     ASSERT_TRUE(base::ReadFileToString(file_path, &tmp_expected_body))
@@ -3182,7 +3183,7 @@ TEST_P(ParameterizedURLLoaderTest,
   base::FilePath file_path = GetTestFilePath("simple_page.html");
 
   scoped_refptr<ResourceRequestBody> request_body(new ResourceRequestBody());
-  size_t num_files = 2 * kMaxFileUploadRequestsPerBatch;
+  size_t num_files = 2 * FileOpenerForUpload::kMaxFileUploadRequestsPerBatch;
   for (size_t i = 0; i < num_files; ++i) {
     request_body->AppendFileRange(
         file_path, 0, std::numeric_limits<uint64_t>::max(), base::Time());
@@ -3199,7 +3200,7 @@ TEST_P(ParameterizedURLLoaderTest,
   base::FilePath file_path = GetTestFilePath("simple_page.html");
 
   scoped_refptr<ResourceRequestBody> request_body(new ResourceRequestBody());
-  size_t num_files = 2 * kMaxFileUploadRequestsPerBatch;
+  size_t num_files = 2 * FileOpenerForUpload::kMaxFileUploadRequestsPerBatch;
   for (size_t i = 0; i < num_files; ++i) {
     request_body->AppendFileRange(
         file_path, 0, std::numeric_limits<uint64_t>::max(), base::Time());