From 2e5d1508b82e6876095dd61b7b799f1b78add940 Mon Sep 17 00:00:00 2001
From: Bryan Oltman <[email protected]>
Date: Mon, 04 May 2026 15:57:41 -0700
Subject: [PATCH] Fix FileSelectHelper double-release Use-After-Free

Fixes a Use-After-Free caused by a race condition in FileSelectHelper.
This occurred when a macOS package is selected for upload and the user
closes the tab before a content analysis scan completes.

This CL replaces manual refcounting with a WebContents-scoped manager
and moves cleanup to the destructor. This ensures cleanup occurs exactly
once when the last reference is dropped.

Fixed: 500416901
Change-Id: I1aae19469827bc660144b592ecf9916e18189c84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7810279
Reviewed-by: Erik Chen <[email protected]>
Commit-Queue: Bryan Oltman <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1624993}
---

diff --git a/chrome/browser/file_select_helper.cc b/chrome/browser/file_select_helper.cc
index b8a24fda..94f809d 100644
--- a/chrome/browser/file_select_helper.cc
+++ b/chrome/browser/file_select_helper.cc
@@ -67,6 +67,30 @@
 
 DEFINE_LOCAL_ELEMENT_IDENTIFIER_VALUE(kCancelButtonId);
 
+WEB_CONTENTS_USER_DATA_KEY_IMPL(FileSelectHelper::ActiveHelpers);
+
+FileSelectHelper::ActiveHelpers::ActiveHelpers(
+    content::WebContents* web_contents)
+    : content::WebContentsUserData<ActiveHelpers>(*web_contents) {}
+
+FileSelectHelper::ActiveHelpers::~ActiveHelpers() = default;
+
+// static
+void FileSelectHelper::ActiveHelpers::Add(
+    content::WebContents* tab,
+    scoped_refptr<FileSelectHelper> helper) {
+  GetOrCreateForWebContents(tab)->helpers_.insert(std::move(helper));
+}
+
+// static
+void FileSelectHelper::ActiveHelpers::Remove(FileSelectHelper* helper) {
+  if (helper->web_contents_) {
+    if (auto* active_helpers = FromWebContents(helper->web_contents_)) {
+      active_helpers->helpers_.erase(helper);
+    }
+  }
+}
+
 namespace {
 
 void DeleteFiles(std::vector<base::FilePath> paths) {
@@ -118,8 +142,13 @@
 FileSelectHelper::~FileSelectHelper() {
   // There may be pending file dialogs, we need to tell them that we've gone
   // away so they don't try and call back to us.
-  if (select_file_dialog_)
+  if (select_file_dialog_) {
     select_file_dialog_->ListenerDestroyed();
+  }
+
+  if (!temporary_files_.empty()) {
+    DeleteTemporaryFiles();
+  }
 }
 
 void FileSelectHelper::FileSelected(const ui::SelectedFileInfo& file,
@@ -411,16 +440,6 @@
       base::BindOnce(&DeleteFiles, std::move(temporary_files_)));
 }
 
-void FileSelectHelper::CleanUp() {
-  if (!temporary_files_.empty()) {
-    DeleteTemporaryFiles();
-
-    // Now that the temporary files have been scheduled for deletion, there
-    // is no longer any reason to keep this instance around.
-    Release();
-  }
-}
-
 bool FileSelectHelper::AbortIfWebContentsDestroyed() {
   if (abort_on_missing_web_contents_in_tests_ &&
       (render_frame_host_ == nullptr || web_contents_ == nullptr)) {
@@ -517,10 +536,15 @@
   Profile* profile = Profile::FromBrowserContext(
       render_frame_host->GetProcess()->GetBrowserContext());
 
-  // FileSelectHelper will keep itself alive until it sends the result
-  // message.
+  // The `FileSelectHelper::ActiveHelpers` will keep this instance alive for the
+  // duration of the operation on this tab. Asynchronous tasks (like zipping
+  // or content analysis) may also hold a reference.
   scoped_refptr<FileSelectHelper> file_select_helper(
       new FileSelectHelper(profile));
+  auto* web_contents = WebContents::FromRenderFrameHost(render_frame_host);
+  if (web_contents) {
+    ActiveHelpers::Add(web_contents, file_select_helper);
+  }
   file_select_helper->RunFileChooser(render_frame_host, std::move(listener),
                                      params.Clone());
 }
@@ -531,10 +555,14 @@
     scoped_refptr<content::FileSelectListener> listener,
     const base::FilePath& path) {
   Profile* profile = Profile::FromBrowserContext(tab->GetBrowserContext());
-  // FileSelectHelper will keep itself alive until it sends the result
-  // message.
+  // The `FileSelectHelper::ActiveHelpers` will keep this instance alive for the
+  // duration of the operation on this tab. Asynchronous tasks (like zipping
+  // or content analysis) may also hold a reference.
   scoped_refptr<FileSelectHelper> file_select_helper(
       new FileSelectHelper(profile));
+  if (tab) {
+    ActiveHelpers::Add(tab, file_select_helper);
+  }
   file_select_helper->EnumerateDirectoryImpl(tab, std::move(listener), path);
 }
 
@@ -582,13 +610,6 @@
       FROM_HERE, {base::MayBlock()},
       base::BindOnce(&FileSelectHelper::GetFileTypesInThreadPool, this,
                      std::move(params)));
-
-  // Because this class returns notifications to the RenderViewHost, it is
-  // difficult for callers to know how long to keep a reference to this
-  // instance. We AddRef() here to keep the instance alive after we return
-  // to the caller, until the last callback is received from the file dialog.
-  // At that point, we must call RunFileChooserEnd().
-  AddRef();
 }
 
 void FileSelectHelper::GetFileTypesInThreadPool(FileChooserParamsPtr params) {
@@ -675,8 +696,8 @@
 }
 
 // This method is called when we receive the last callback from the file chooser
-// dialog or if the renderer was destroyed. Perform any cleanup and release the
-// reference we added in RunFileChooser().
+// dialog or if the renderer was destroyed. Perform any cleanup and unregister
+// from the tab-scoped manager.
 void FileSelectHelper::RunFileChooserEnd() {
 #if !BUILDFLAG(IS_ANDROID)
   // Ensure picture-in-picture occlusion mitigation stops, even if we need to
@@ -690,20 +711,11 @@
   // If there are temporary files, then this instance needs to stick around
   // until web_contents_ is destroyed, so that this instance can delete the
   // temporary files.
-  if (!temporary_files_.empty())
+  if (!temporary_files_.empty()) {
     return;
-
-  if (listener_)
-    listener_->FileSelectionCanceled();
-  render_frame_host_ = nullptr;
-  web_contents_ = nullptr;
-  // If the dialog was actually opened, dispose of our reference.
-  if (select_file_dialog_) {
-    select_file_dialog_->ListenerDestroyed();
-    select_file_dialog_.reset();
   }
 
-  Release();
+  ActiveHelpers::Remove(this);
 }
 
 void FileSelectHelper::EnumerateDirectoryImpl(
@@ -715,12 +727,6 @@
   dialog_type_ = ui::SelectFileDialog::SELECT_NONE;
   web_contents_ = tab;
   listener_ = std::move(listener);
-  // Because this class returns notifications to the RenderViewHost, it is
-  // difficult for callers to know how long to keep a reference to this
-  // instance. We AddRef() here to keep the instance alive after we return
-  // to the caller, until the last callback is received from the enumeration
-  // code. At that point, we must call EnumerateDirectoryEnd().
-  AddRef();
 #if BUILDFLAG(IS_ANDROID)
   if (path.IsContentUri()) {
     base::ThreadPool::PostTaskAndReplyWithResult(
@@ -733,10 +739,9 @@
 }
 
 // This method is called when we receive the last callback from the enumeration
-// code. Perform any cleanup and release the reference we added in
-// EnumerateDirectoryImpl().
+// code. Perform any cleanup and unregister from the tab-scoped manager.
 void FileSelectHelper::EnumerateDirectoryEnd() {
-  Release();
+  ActiveHelpers::Remove(this);
 }
 
 void FileSelectHelper::RenderFrameHostChanged(
@@ -763,7 +768,6 @@
   render_frame_host_ = nullptr;
   web_contents_ = nullptr;

diff --git a/chrome/browser/file_select_helper_unittest.cc b/chrome/browser/file_select_helper_unittest.cc
index 173c1ee..d09e084e 100644
--- a/chrome/browser/file_select_helper_unittest.cc
+++ b/chrome/browser/file_select_helper_unittest.cc
@@ -237,19 +237,15 @@
   for (const auto& mode : modes) {
     file_select_helper->dialog_mode_ = mode;
 
-    file_select_helper->AddRef();  // Normally called by RunFileChooser().
     file_select_helper->FileSelected(ui::SelectedFileInfo(file_path_1), index);
     EXPECT_EQ(dir_path_1, profile.last_selected_directory());
 
-    file_select_helper->AddRef();  // Normally called by RunFileChooser().
     file_select_helper->FileSelected(ui::SelectedFileInfo(file_path_2), index);
     EXPECT_EQ(dir_path_1, profile.last_selected_directory());
 
-    file_select_helper->AddRef();  // Normally called by RunFileChooser().
     file_select_helper->FileSelected(ui::SelectedFileInfo(file_path_3), index);
     EXPECT_EQ(dir_path_2, profile.last_selected_directory());
 
-    file_select_helper->AddRef();  // Normally called by RunFileChooser().
     file_select_helper->MultiFilesSelected(
         ui::FilePathListToSelectedFileInfoList(files));
     EXPECT_EQ(dir_path_1, profile.last_selected_directory());
@@ -258,15 +254,12 @@
   // Type where the selected folder itself is remembered.
   file_select_helper->dialog_mode_ = FileChooserParams::Mode::kUploadFolder;
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->FileSelected(ui::SelectedFileInfo(dir_path_1), index);
   EXPECT_EQ(dir_path_1, profile.last_selected_directory());
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->FileSelected(ui::SelectedFileInfo(dir_path_2), index);
   EXPECT_EQ(dir_path_2, profile.last_selected_directory());
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->MultiFilesSelected(
       ui::FilePathListToSelectedFileInfoList(dirs));
   EXPECT_EQ(dir_path_1, profile.last_selected_directory());
@@ -290,7 +283,6 @@
   std::vector<blink::mojom::FileChooserFileInfoPtr> orig_files;
   enterprise_connectors::ContentAnalysisDelegate::Data data;
   enterprise_connectors::ContentAnalysisDelegate::Result result;
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -314,7 +306,6 @@
   PrepareContentAnalysisCompletionCallbackArgs(
       {data_dir_.AppendASCII("foo.doc")}, {true}, &orig_files, &data, &result);
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -339,7 +330,6 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {true, true}, &orig_files, &data, &result);
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -364,10 +354,8 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {false, false}, &orig_files, &data, &result);
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
-
   EXPECT_EQ(0u, files.size());
 }
 
@@ -389,7 +377,6 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {false, true}, &orig_files, &data, &result);
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -419,7 +406,6 @@
         blink::mojom::FileSystemFileInfo::New()));
   }
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -454,7 +440,6 @@
   orig_files.push_back(blink::mojom::FileChooserFileInfo::NewFileSystem(
       blink::mojom::FileSystemFileInfo::New()));
 
-  file_select_helper->AddRef();  // Normally called by RunFileChooser().
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
@@ -488,15 +473,9 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {true, true}, &orig_files, &data, &result);
 
-  // Calling the content analysis completion callback would normally
-  // release `file_select_helper`, so we add a reference and validate that
-  // it goes down to 1 after the call.
-  file_select_helper->AddRef();
-  EXPECT_FALSE(file_select_helper->HasOneRef());
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
-  EXPECT_TRUE(file_select_helper->HasOneRef());
   EXPECT_EQ(2u, files.size());
 }
 
@@ -522,14 +501,8 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {false, false}, &orig_files, &data, &result);
 
-  // Calling the content analysis completion callback would normally
-  // release `file_select_helper`, so we add a reference and validate that
-  // it goes down to 1 after the call.
-  file_select_helper->AddRef();
-  EXPECT_FALSE(file_select_helper->HasOneRef());
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
-  EXPECT_TRUE(file_select_helper->HasOneRef());
   EXPECT_EQ(0u, files.size());
 }
 
@@ -555,15 +528,9 @@
       {data_dir_.AppendASCII("foo.doc"), data_dir_.AppendASCII("bar.doc")},
       {true, false}, &orig_files, &data, &result);
 
-  // Calling the content analysis completion callback would normally
-  // release `file_select_helper`, so we add a reference and validate that
-  // it goes down to 1 after the call.
-  file_select_helper->AddRef();
-  EXPECT_FALSE(file_select_helper->HasOneRef());
   file_select_helper->ContentAnalysisCompletionCallback(std::move(orig_files),
                                                         data, result);
 
-  EXPECT_TRUE(file_select_helper->HasOneRef());
   // Files should be cleared.
   EXPECT_EQ(0u, files.size());
 }
@@ -661,3 +628,41 @@
   EXPECT_EQ(callback_count_, 3);
   EXPECT_EQ(selected_files_.size(), 0u);
 }
+
+// Tests that a pending asynchronous operation (like content analysis) will
+// keep the helper alive until it completes, even if the tab is destroyed.
+TEST_F(FileSelectHelperTest, TaskKeepsAlive) {
+  content::BrowserTaskEnvironment task_environment;
+  TestingProfile profile;
+  auto web_contents = content::WebContents::Create(
+      content::WebContents::CreateParams(&profile));
+
+  scoped_refptr<FileSelectHelper> helper = new FileSelectHelper(&profile);
+  base::WeakPtr<FileSelectHelper> weak_helper = helper->GetWeakPtr();
+
+  // Register with the manager to keep the helper alive.
+  FileSelectHelper::ActiveHelpers::Add(web_contents.get(), helper);
+
+  // Simulate a background task holding a reference.
+  base::OnceClosure task;
+  base::RunLoop run_loop;
+  task = base::BindOnce(
+      [](scoped_refptr<FileSelectHelper> helper,
+         base::OnceClosure quit_closure) {
+        // Do nothing, just hold the reference.
+        std::move(quit_closure).Run();
+      },
+      helper, run_loop.QuitClosure());
+
+  // Drop our local reference and destroy the WebContents.
+  helper = nullptr;
+  web_contents.reset();
+
+  // The helper should be kept alive by the pending task.
+  EXPECT_TRUE(weak_helper);
+
+  // Once the task finishes, the helper should be destroyed.
+  std::move(task).Run();
+  run_loop.Run();
+  EXPECT_FALSE(weak_helper);
+}