From 8a767c6b6625bdffd186b35631023c6f62d34d2d Mon Sep 17 00:00:00 2001
From: Quentin Pubert <[email protected]>
Date: Wed, 10 Jun 2026 02:30:50 -0700
Subject: [PATCH] [iOS] Move SharingCoordinator downloads to unique destination directory

This CL changes how the SharingCoordinator temporarily stores downloaded
files.

Before: `<NSTemporaryDirectory>/OpenIn` is created when the coordinator
starts and emptied if necessary. Then a file is downloaded directly in
this directory and then removed when the activity service (created from
this coordinator) is dismissed.

After: `<NSTemporaryDirectory>/OpenIn/<UUID>` is created when the
coordinator starts. Then a file is downloaded directly in this directory
and then `<NSTemporaryDirectory>/OpenIn/<UUID>` is removed when the
activity service is dismissed.

As for `<NSTemporaryDirectory>` itself, "the system may purge this
directory when your app isn’t running" so there is no need to worry
about it growing too much.

Fixed: 518088219
Change-Id: I507803602c9640c0da2fefec1c3ac2d1e5861a32
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7894787
Reviewed-by: Gauthier Ambard <[email protected]>
Auto-Submit: Quentin Pubert <[email protected]>
Commit-Queue: Quentin Pubert <[email protected]>
Reviewed-by: Olivier Robin <[email protected]>
Reviewed-by: Daniel White <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1644533}
---

diff --git a/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator.mm b/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator.mm
index a286600f..77cd13b 100644
--- a/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator.mm
+++ b/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator.mm
@@ -43,10 +43,10 @@
 // Exposes methods to allow calling the from helper free functions.
 @interface SharingCoordinator (ForHelperFunction)
 
-// Starts the download if `directoryCreated`. If not, show the share menu
-// without file options.
+// Starts the download if `destinationDirectory` is not nil. If not, show the
+// share menu without file options.
 - (void)startDownloadForWebState:(web::WebState*)webState
-                directoryCreated:(BOOL)directoryCreated;
+            destinationDirectory:(NSString*)destinationDirectory;
 
 // The download is successful and should proceed.
 - (void)downloadShouldProceed:(BOOL)shouldProceed;
@@ -65,68 +65,41 @@
       stringByAppendingPathComponent:kDocumentsTemporaryPath];
 }
 
-// Removes all the stored files at `path`.
-void RemoveAllStoredDocumentsAtPath(NSString* path) {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::WILL_BLOCK);
-  NSFileManager* file_manager = [NSFileManager defaultManager];
-
-  NSError* error = nil;
-  NSArray<NSString*>* document_files =
-      [file_manager contentsOfDirectoryAtPath:path error:&error];
-  if (!document_files) {
-    DLOG(ERROR) << "Failed to get content of directory at path: "
-                << base::SysNSStringToUTF8([error description]);
-    return;
-  }
-
-  for (NSString* filename in document_files) {
-    NSString* file_path = [path stringByAppendingPathComponent:filename];
-    if (![file_manager removeItemAtPath:file_path error:&error]) {
-      DLOG(ERROR) << "Failed to remove file: "
-                  << base::SysNSStringToUTF8([error description]);
-    }
-  }
-}
-
-// Remove a file stored at `path` if it exists.
-void RemoveFileAtPath(NSString* path) {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::WILL_BLOCK);
-  NSFileManager* file_manager = [NSFileManager defaultManager];
-
-  if ([file_manager fileExistsAtPath:path]) {
-    NSError* error = nil;
-    if (![file_manager removeItemAtPath:path error:&error]) {
-      DLOG(ERROR) << "Failed to remove file: "
-                  << base::SysNSStringToUTF8([error description]);
-    }
-  }
-}
-
-// Ensures the destination directory is created and any contained obsolete files
-// are deleted. Returns YES if the directory is created successfully.
-BOOL CreateDestinationDirectoryAndRemoveObsoleteFiles() {
+// Ensures the destination directory is created. Returns `nil` if the directory
+// was not created successfully.
+NSString* CreateUniqueDestinationDirectory() {
   NSString* temporary_directory_path = GetTemporaryDocumentDirectory();
+  NSString* destination_directory_path = [temporary_directory_path
+      stringByAppendingPathComponent:[[NSUUID UUID] UUIDString]];
   base::File::Error error;
   if (!CreateDirectoryAndGetError(
-          base::apple::NSStringToFilePath(temporary_directory_path), &error)) {
+          base::apple::NSStringToFilePath(destination_directory_path),
+          &error)) {
     DLOG(ERROR) << "Error creating destination dir: " << error;
-    return NO;
+    return nil;
   }
-  // Remove all documents that might be still on temporary storage.
-  RemoveAllStoredDocumentsAtPath(temporary_directory_path);
-  return YES;
+  return destination_directory_path;
+}
+
+// Remove a directory created using CreateUniqueDestinationDirectory().
+void RemoveUniqueDestinationDirectory(NSString* destination_directory_path) {
+  if (destination_directory_path.length == 0) {
+    return;
+  }
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
+  std::ignore = base::DeletePathRecursively(
+      base::apple::NSStringToFilePath(destination_directory_path));
 }
 
 // Starts download for `weak_web_state` if `directory_created` using
 // `coordinator`.
 void StartDownloadForWebState(__weak SharingCoordinator* coordinator,
                               base::WeakPtr<web::WebState> weak_web_state,
-                              BOOL directory_created) {
+                              NSString* destination_directory) {
   if (web::WebState* web_state = weak_web_state.get()) {
     [coordinator startDownloadForWebState:web_state
-                         directoryCreated:directory_created];
+                     destinationDirectory:destination_directory];
   }
 }
 
@@ -199,6 +172,8 @@
   base::WeakPtr<web::WebState> _originatingWebState;
   // The GURL of the download.
   GURL _downloadGURL;
+  // The path to the directory where the download is saved.
+  NSString* _destinationDirectory;
 }
 
 - (instancetype)
@@ -278,8 +253,7 @@
     // background sequence, then on current sequence complete the workflow.
     __weak SharingCoordinator* weakSelf = self;
     _taskRunner->PostTaskAndReplyWithResult(
-        FROM_HERE,
-        base::BindOnce(&CreateDestinationDirectoryAndRemoveObsoleteFiles),
+        FROM_HERE, base::BindOnce(&CreateUniqueDestinationDirectory),
         base::BindOnce(&StartDownloadForWebState, weakSelf,
                        _originatingWebState));
   } else {
@@ -316,11 +290,12 @@
 - (void)activityServiceDidEndPresenting {
   [self.activityServiceCoordinator stop];
   self.activityServiceCoordinator = nil;
-
-  // If a new download with a file with the same name exist it will throw an
-  // error in downloadDidFailWithError method.
-  _taskRunner->PostTask(FROM_HERE,
-                        base::BindOnce(&RemoveFileAtPath, self.filePath));
+  if (_destinationDirectory) {
+    _taskRunner->PostTask(FROM_HERE,
+                          base::BindOnce(&RemoveUniqueDestinationDirectory,
+                                         _destinationDirectory));
+    _destinationDirectory = nil;
+  }
 }
 
 #pragma mark - WebStateListObserving
@@ -356,11 +331,17 @@
 #pragma mark - Private Methods
 
 - (void)startDownloadForWebState:(web::WebState*)webState
-                directoryCreated:(BOOL)directoryCreated {
+            destinationDirectory:(NSString*)destinationDirectory {
   if (_stopped) {
+    if (destinationDirectory) {
+      _taskRunner->PostTask(FROM_HERE,
+                            base::BindOnce(&RemoveUniqueDestinationDirectory,
+                                           destinationDirectory));
+    }
     return;
   }
-  if (directoryCreated) {
+  if (destinationDirectory) {
+    _destinationDirectory = destinationDirectory;
     [self startDisplayDownloadOverlayOnWebView:webState];
     [self startDownloadFromWebState:webState];
   } else {
@@ -415,7 +396,8 @@

diff --git a/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator_unittest.mm b/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator_unittest.mm
index 4ce633c..986fb7f5 100644
--- a/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator_unittest.mm
+++ b/ios/chrome/browser/sharing/ui_bundled/sharing_coordinator_unittest.mm
@@ -9,6 +9,7 @@
 #import "base/files/scoped_temp_dir.h"
 #import "base/ios/block_types.h"
 #import "base/strings/sys_string_conversions.h"
+#import "base/task/thread_pool/thread_pool_instance.h"
 #import "base/test/ios/wait_util.h"
 #import "base/test/run_until.h"
 #import "base/test/scoped_feature_list.h"
@@ -55,6 +56,10 @@
 using base::test::ios::WaitUntilConditionOrTimeout;
 using bookmarks::BookmarkNode;
 
+@interface SharingCoordinator (Testing)
+- (void)startDownloadFromWebState:(web::WebState*)webState;
+@end
+
 // Test fixture for testing SharingCoordinator.
 class SharingCoordinatorTest : public BookmarkIOSUnitTestSupport {
  protected:
@@ -480,3 +485,106 @@
   EXPECT_OCMOCK_VERIFY(vc_partial_mock);
   [coordinator stop];
 }
+
+// Test that starting a download creates a unique destination directory on disk,
+// and stopping/tearing down the coordinator destroys it.
+TEST_F(SharingCoordinatorTest, Start_CreatesAndDestroysDirectory) {
+  url_value_ = base::Value("https://example.com/test.pdf");
+  SetupForFileDownload();
+
+  SharingParams* params =
+      [[SharingParams alloc] initWithScenario:test_scenario_];
+
+  SharingCoordinator* coordinator = [[SharingCoordinator alloc]
+      initWithBaseViewController:base_view_controller_
+                         browser:browser_.get()
+                          params:params
+                      sourceItem:fake_origin_view_];
+
+  NSString* temp_dir =
+      [NSTemporaryDirectory() stringByAppendingPathComponent:@"OpenIn"];
+  NSFileManager* file_manager = [NSFileManager defaultManager];
+
+  NSArray<NSString*>* files_before = nil;
+  if ([file_manager fileExistsAtPath:temp_dir]) {
+    files_before = [file_manager contentsOfDirectoryAtPath:temp_dir error:nil];
+  }
+  size_t count_before = files_before.count;
+
+  [coordinator start];
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  ASSERT_TRUE(base::test::RunUntil(^bool {
+    NSArray<NSString*>* files = [file_manager contentsOfDirectoryAtPath:temp_dir
+                                                                  error:nil];
+    return files.count == count_before + 1;
+  }));
+
+  [coordinator stop];
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+  ASSERT_TRUE(base::test::RunUntil(^bool {
+    NSArray<NSString*>* files = nil;
+    if ([file_manager fileExistsAtPath:temp_dir]) {
+      files = [file_manager contentsOfDirectoryAtPath:temp_dir error:nil];
+    }
+    return files.count == count_before;
+  }));
+}
+
+// Test that stopping the coordinator before the directory is created on the
+// background thread cleans up the directory and does not start the download.
+TEST_F(SharingCoordinatorTest, StopBeforeDirectoryCreated_NoLeakAndNoDownload) {
+  url_value_ = base::Value("https://example.com/test.pdf");
+  SetupForFileDownload();
+
+  SharingParams* params =
+      [[SharingParams alloc] initWithScenario:test_scenario_];
+
+  SharingCoordinator* coordinator = [[SharingCoordinator alloc]
+      initWithBaseViewController:base_view_controller_
+                         browser:browser_.get()
+                          params:params
+                      sourceItem:fake_origin_view_];
+
+  id coordinator_mock = OCMPartialMock(coordinator);
+  [[coordinator_mock reject]
+      startDownloadFromWebState:static_cast<web::WebState*>(
+                                    [OCMArg anyPointer])];
+
+  NSString* temp_dir =
+      [NSTemporaryDirectory() stringByAppendingPathComponent:@"OpenIn"];
+  NSFileManager* file_manager = [NSFileManager defaultManager];
+
+  NSArray<NSString*>* files_before = nil;
+  if ([file_manager fileExistsAtPath:temp_dir]) {
+    files_before = [file_manager contentsOfDirectoryAtPath:temp_dir error:nil];
+  }
+  size_t count_before = files_before.count;
+
+  [coordinator start];
+  [coordinator stop];
+
+  base::ThreadPoolInstance::Get()->FlushForTesting();
+
+  // The directory has been created by the background thread (so count is +1)
+  // but the reply hasn't processed on the main thread yet. Checking this
+  // immediately after `FlushForTesting` is safe and not flaky because the main
+  // thread has not run its message loop yet, meaning the reply task (which
+  // deletes the directory since the coordinator was stopped) has not had a
+  // chance to execute.
+  NSArray<NSString*>* files_after_start =
+      [file_manager contentsOfDirectoryAtPath:temp_dir error:nil];
+  EXPECT_EQ(files_after_start.count, count_before + 1);
+
+  // Now run the loop until the reply is executed on the main thread, which will
+  // schedule the deletion task on the background thread, and the background
+  // task completes the deletion.
+  ASSERT_TRUE(base::test::RunUntil(^bool {
+    NSArray<NSString*>* files = nil;
+    if ([file_manager fileExistsAtPath:temp_dir]) {
+      files = [file_manager contentsOfDirectoryAtPath:temp_dir error:nil];
+    }
+    return files.count == count_before;
+  }));
+
+  EXPECT_OCMOCK_VERIFY(coordinator_mock);
+}