From e4b3ddef2eb031944d6f13813bf7acf6f35210a7 Mon Sep 17 00:00:00 2001
From: Quentin Pubert <[email protected]>
Date: Tue, 23 Jun 2026 02:04:25 -0700
Subject: [PATCH] [iOS] Don't overwrite existing files in DownloadFileService

DownloadFileService::MoveDownloadFile resolved the destination path and
performed the move as separate sequenced tasks with main-thread hops in
between. If two downloads with the same suggested filename completed
close together, both could resolve to the same path and the second
base::Move (rename) would silently replace the first.

Re-check the destination on the file task runner immediately before the
move and pick the next available uniquified name if it is already taken.
DoMoveFileOnBackgroundThread now returns the actual destination so the
download record and the tab helper's final path reflect where the file
landed.

Fixed: 517621178
Change-Id: I7e5e1b250dd7a63fc96364e62d42454e2f1b4b5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7976024
Auto-Submit: Quentin Pubert <[email protected]>
Commit-Queue: Quentin Pubert <[email protected]>
Reviewed-by: Olivier Robin <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1650861}
---

diff --git a/ios/chrome/browser/download/model/download_file_service.h b/ios/chrome/browser/download/model/download_file_service.h
index f9e54b7..1a2ce696 100644
--- a/ios/chrome/browser/download/model/download_file_service.h
+++ b/ios/chrome/browser/download/model/download_file_service.h
@@ -36,7 +36,10 @@
 
   ~DownloadFileService() override;
 
-  // Asynchronously moves a download file from source to destination.
+  // Asynchronously moves a download file from source to destination. If a file
+  // already exists at `destination_path`, the next available uniquified name
+  // in the same directory is used instead. The actual destination is reported
+  // through `callback`.
   // Must be called on the main thread. Callback runs on the main thread.
   void MoveDownloadFile(const std::string& download_id,
                         const base::FilePath& source_path,
@@ -58,14 +61,15 @@
 
  private:
   // Called on the main thread when the file move operation completes.
+  // `final_path` is the path the file was moved to, or empty on failure.
   void OnFileMoveComplete(const std::string& download_id,
                           const base::FilePath& source_path,
-                          const base::FilePath& destination_path,
                           MoveCompleteCallback callback,
-                          bool move_success);
+                          base::FilePath final_path);
 
-  // Performs the actual file move on the background thread.
-  static bool DoMoveFileOnBackgroundThread(
+  // Performs the actual file move on the background thread. Returns the path
+  // the file was moved to, or an empty path on failure.
+  static base::FilePath DoMoveFileOnBackgroundThread(
       scoped_refptr<base::SequencedTaskRunner> file_task_runner,
       const base::FilePath& source_path,
       const base::FilePath& destination_path);
diff --git a/ios/chrome/browser/download/model/download_file_service.mm b/ios/chrome/browser/download/model/download_file_service.mm
index d43d16b..b410c68 100644
--- a/ios/chrome/browser/download/model/download_file_service.mm
+++ b/ios/chrome/browser/download/model/download_file_service.mm
@@ -49,7 +49,7 @@
                      file_task_runner_, source_path, destination_path),
       base::BindOnce(&DownloadFileService::OnFileMoveComplete,
                      weak_ptr_factory_.GetWeakPtr(), download_id, source_path,
-                     destination_path, std::move(callback)));
+                     std::move(callback)));
 }
 
 void DownloadFileService::ResolveAvailableFilePath(
@@ -75,29 +75,26 @@
       std::move(callback));
 }
 
-void DownloadFileService::OnFileMoveComplete(
-    const std::string& download_id,
-    const base::FilePath& source_path,
-    const base::FilePath& destination_path,
-    MoveCompleteCallback callback,
-    bool move_success) {
+void DownloadFileService::OnFileMoveComplete(const std::string& download_id,
+                                             const base::FilePath& source_path,
+                                             MoveCompleteCallback callback,
+                                             base::FilePath final_path) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(main_sequence_checker_);
 
+  const bool move_success = !final_path.empty();
   if (move_success && download_record_service_) {
     // Convert absolute path to relative path for storage.
-    base::FilePath relative_path =
-        ConvertToRelativeDownloadPath(destination_path);
+    base::FilePath relative_path = ConvertToRelativeDownloadPath(final_path);
     download_record_service_->UpdateDownloadFilePathAsync(download_id,
                                                           relative_path);
   }
 
   if (callback) {
-    std::move(callback).Run(move_success, download_id, source_path,
-                            destination_path);
+    std::move(callback).Run(move_success, download_id, source_path, final_path);
   }
 }
 
-bool DownloadFileService::DoMoveFileOnBackgroundThread(
+base::FilePath DownloadFileService::DoMoveFileOnBackgroundThread(
     scoped_refptr<base::SequencedTaskRunner> file_task_runner,
     const base::FilePath& source_path,
     const base::FilePath& destination_path) {
@@ -105,21 +102,30 @@
 
   // Check if source file exists.
   if (!base::PathExists(source_path)) {
-    return false;
+    return base::FilePath();
   }
 
   // Create destination directory if it doesn't exist.
   base::FilePath destination_dir = destination_path.DirName();
   if (!base::CreateDirectory(destination_dir)) {
-    return false;
+    return base::FilePath();
+  }
+
+  // If a file already exists at the requested destination (e.g. another
+  // download with the same suggested name completed first), pick the next
+  // available name so the existing file is not overwritten.
+  base::FilePath final_path = destination_path;
+  if (base::PathExists(final_path)) {
+    final_path = FindAvailableDownloadFilePath(
+        file_task_runner, destination_dir, destination_path.BaseName());
   }
 
   // Move the file.
-  if (!base::Move(source_path, destination_path)) {
-    return false;
+  if (!base::Move(source_path, final_path)) {
+    return base::FilePath();
   }
 
-  return true;
+  return final_path;
 }
 
 base::FilePath DownloadFileService::FindAvailableDownloadFilePath(
diff --git a/ios/chrome/browser/download/model/download_file_service_unittest.mm b/ios/chrome/browser/download/model/download_file_service_unittest.mm
index d6e12a5..a483c78 100644
--- a/ios/chrome/browser/download/model/download_file_service_unittest.mm
+++ b/ios/chrome/browser/download/model/download_file_service_unittest.mm
@@ -358,6 +358,62 @@
   }
 }
 
+// Tests that moving a download file to a destination that already exists does
+// not overwrite the existing file. The move should pick the next available
+// uniquified name and report it through the callback.
+TEST_F(DownloadFileServiceTest, MoveDownloadFileDestinationExists) {
+  const char kExistingContent[] = "existing content";
+  const char kMovedContent[] = "moved content";
+
+  // Create a file already occupying the requested destination.
+  base::FilePath dest_path = dest_dir_.AppendASCII(kConflictFileName);
+  ASSERT_TRUE(base::WriteFile(dest_path, kExistingContent));
+
+  // Create the source file to be moved.
+  base::FilePath source_path =
+      CreateTestFile(source_dir_, kConflictFileName, kMovedContent);
+
+  base::FilePath expected_final_path =
+      dest_dir_.AppendASCII("conflict_file (1).txt");
+  base::FilePath expected_relative_path =
+      ConvertToRelativeDownloadPath(expected_final_path);
+  EXPECT_CALL(
+      *mock_download_record_service_,
+      UpdateDownloadFilePathAsync(kTestDownloadId, expected_relative_path, _))
+      .Times(1);
+
+  base::RunLoop run_loop;
+  bool callback_success = false;
+  base::FilePath callback_final_path;
+
+  service_->MoveDownloadFile(
+      kTestDownloadId, source_path, dest_path,
+      base::BindLambdaForTesting([&](bool success,
+                                     const std::string& download_id,
+                                     const base::FilePath& actual_source_path,
+                                     const base::FilePath& actual_final_path) {
+        callback_success = success;
+        callback_final_path = actual_final_path;
+        run_loop.Quit();
+      }));
+
+  run_loop.Run();
+
+  EXPECT_TRUE(callback_success);
+  EXPECT_EQ(expected_final_path, callback_final_path);

diff --git a/ios/chrome/browser/download/model/download_file_service_unittest.mm b/ios/chrome/browser/download/model/download_file_service_unittest.mm
index d6e12a5..a483c78 100644
--- a/ios/chrome/browser/download/model/download_file_service_unittest.mm
+++ b/ios/chrome/browser/download/model/download_file_service_unittest.mm
@@ -358,6 +358,62 @@
   }
 }
 
+// Tests that moving a download file to a destination that already exists does
+// not overwrite the existing file. The move should pick the next available
+// uniquified name and report it through the callback.
+TEST_F(DownloadFileServiceTest, MoveDownloadFileDestinationExists) {
+  const char kExistingContent[] = "existing content";
+  const char kMovedContent[] = "moved content";
+
+  // Create a file already occupying the requested destination.
+  base::FilePath dest_path = dest_dir_.AppendASCII(kConflictFileName);
+  ASSERT_TRUE(base::WriteFile(dest_path, kExistingContent));
+
+  // Create the source file to be moved.
+  base::FilePath source_path =
+      CreateTestFile(source_dir_, kConflictFileName, kMovedContent);
+
+  base::FilePath expected_final_path =
+      dest_dir_.AppendASCII("conflict_file (1).txt");
+  base::FilePath expected_relative_path =
+      ConvertToRelativeDownloadPath(expected_final_path);
+  EXPECT_CALL(
+      *mock_download_record_service_,
+      UpdateDownloadFilePathAsync(kTestDownloadId, expected_relative_path, _))
+      .Times(1);
+
+  base::RunLoop run_loop;
+  bool callback_success = false;
+  base::FilePath callback_final_path;
+
+  service_->MoveDownloadFile(
+      kTestDownloadId, source_path, dest_path,
+      base::BindLambdaForTesting([&](bool success,
+                                     const std::string& download_id,
+                                     const base::FilePath& actual_source_path,
+                                     const base::FilePath& actual_final_path) {
+        callback_success = success;
+        callback_final_path = actual_final_path;
+        run_loop.Quit();
+      }));
+
+  run_loop.Run();
+
+  EXPECT_TRUE(callback_success);
+  EXPECT_EQ(expected_final_path, callback_final_path);
+
+  // The existing file must be left untouched.
+  std::string existing_content;
+  ASSERT_TRUE(base::ReadFileToString(dest_path, &existing_content));
+  EXPECT_EQ(kExistingContent, existing_content);
+
+  // The moved file must land at the uniquified path.
+  EXPECT_FALSE(base::PathExists(source_path));
+  std::string moved_content;
+  ASSERT_TRUE(base::ReadFileToString(expected_final_path, &moved_content));
+  EXPECT_EQ(kMovedContent, moved_content);
+}
+
 // Tests moving a download file to a non-existent directory.
 // The directory should be created and the file moved successfully.
 TEST_F(DownloadFileServiceTest, MoveDownloadFileCreateDestinationDirectory) {