From e46ff6303d1aa0c1b5d1f11eb91cba36eda84a06 Mon Sep 17 00:00:00 2001
From: Fergal Daly <[email protected]>
Date: Wed, 24 Sep 2025 21:56:58 -0700
Subject: [PATCH] Close file picker dialog if the tab becomes invisible.

A previous fix for a similar bug (https://crrev.com/c/659915) added code
to not show the dialog if the tab is already invisible however there's
lots of opportunity for races.

This fixes that by making the FileSystemChooser a WebContentsObserver to
catch later changes. The observer is created immediately after checking
the visibility so there is no race. The old code is removed as it does
the checking at a point many steps away from creating the chooser.

The old unittest is removed rather than updated. The relevant code has
moved and rather write a new unittest, I'm relying on coverage in
content_browsertests and browser_tests.

The changes to file_system_chooser_unittest.cc are basically a no-op but
all of the tests need a WebContents instance now.

Bug: 419721056,337356054
Change-Id: I6d3fb55c4fc7c4468b6172cd505bef081060f722
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6966639
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Fergal Daly <[email protected]>
Reviewed-by: Mingyu Lei <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1520251}
---

diff --git a/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc b/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
index d8073d33..3f97c77 100644
--- a/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
+++ b/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
@@ -10,6 +10,7 @@
 #include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/task/current_thread.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_path_override.h"
 #include "base/test/test_file_util.h"
@@ -46,6 +47,7 @@
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/fenced_frame_test_util.h"
+#include "content/public/test/file_system_chooser_test_helpers.h"
 #include "content/public/test/prerender_test_util.h"
 #include "content/public/test/scoped_web_ui_controller_factory_registration.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -1322,11 +1324,44 @@
 
   // Try to show a file picker in the background tab.
   // This should be blocked.
-  EXPECT_EQ("NotAllowedError",
+  EXPECT_EQ("AbortError",
             content::EvalJs(second_tab,
                             "self.showOpenFilePicker().catch(e => e.name)"));
 }
 
+// Test that opening another tab while the dialog is showing closes the dialog.
+// https://crbug.com/419721056
+IN_PROC_BROWSER_TEST_P(FileSystemAccessBrowserTest, ShowOpenFileThenHide) {
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/title1.html")));
+  content::WebContents* first_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Open the dialog and wait until it's created.
+  content::SelectFileDialogRecorder recorder;
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::ObservableSelectFileDialogFactory>(&recorder));
+  ASSERT_EQ(42,
+            content::EvalJs(
+                first_tab,
+                "window.p = self.showOpenFilePicker().catch(e => e.name); 42"));
+  ASSERT_TRUE(base::test::RunUntil([&recorder]() {
+    return recorder.state != content::SelectFileDialogRecorder::kNotCreated;
+  }));
+
+  // Create a second tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), embedded_test_server()->GetURL("/title2.html"),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // The first tab should not be the active tab anymore.
+  ASSERT_NE(first_tab, browser()->tab_strip_model()->GetActiveWebContents());
+
+  // Check that the dialog was closed.
+  ASSERT_EQ("AbortError", content::EvalJs(first_tab, "window.p"));
+}
+
 class FileSystemAccessBrowserTestForWebUI
     : public InProcessBrowserTest,
       public ::testing::WithParamInterface<bool> {
diff --git a/content/browser/file_system_access/file_system_access_manager_impl.cc b/content/browser/file_system_access/file_system_access_manager_impl.cc
index 8debb42..39aa8aa 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl.cc
@@ -587,15 +587,6 @@
     return;
   }
 
-  if (web_contents->GetVisibility() != Visibility::VISIBLE) {
-    std::move(callback).Run(
-        file_system_access_error::FromStatus(
-            FileSystemAccessStatus::kPermissionDenied,
-            "Tab must be visible in order to show a file picker."),
-        std::vector<blink::mojom::FileSystemAccessEntryPtr>());
-    return;
-  }
-
   // Consume user activation to address this issue: crbug.com/40059071
   // TODO(crbug.com/411125804): Consider moving this user activation check to
   // the renderer process or informing the renderer that it lost user
diff --git a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
index 045041db..c2d849d2 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
@@ -2109,61 +2109,6 @@
             bad_message_observer.WaitForBadMessage());
 }
 
-TEST_F(FileSystemAccessManagerImplTest, ChooseEntries_HiddenTab) {
-  base::FilePath test_file = dir_.GetPath().AppendASCII("asdf");
-  ASSERT_TRUE(base::CreateTemporaryFile(&test_file));
-  PathInfo test_file_info(test_file);
-
-  manager_->SetFilePickerResultForTesting(test_file_info);
-
-  static_cast<TestRenderFrameHost*>(web_contents_->GetPrimaryMainFrame())
-      ->SimulateUserActivation();
-
-  mojo::Remote<blink::mojom::FileSystemAccessManager> manager_remote;
-  FileSystemAccessManagerImpl::BindingContext binding_context = {
-      kTestStorageKey, kTestURL,
-      web_contents_->GetPrimaryMainFrame()->GetGlobalId()};
-  manager_->BindReceiver(binding_context,
-                         manager_remote.BindNewPipeAndPassReceiver());
-
-  EXPECT_CALL(permission_context_,
-              CanObtainReadPermission(kTestStorageKey.origin()))
-      .WillOnce(testing::Return(true));
-  EXPECT_CALL(permission_context_,
-              CanObtainWritePermission(kTestStorageKey.origin()))
-      .WillOnce(testing::Return(true));
-  EXPECT_CALL(permission_context_, CheckPathsAgainstEnterprisePolicy(
-                                       testing::_, testing::_, testing::_))
-      .Times(0);
-
-  // Hiding the web contents should make ChooseEntries return an error.
-  web_contents_->WasHidden();
-
-  auto save_file_picker_options = blink::mojom::SaveFilePickerOptions::New(
-      blink::mojom::AcceptsTypesInfo::New(
-          std::vector<blink::mojom::ChooseFileSystemEntryAcceptsOptionPtr>(),
-          /*include_accepts_all=*/true),
-      /*suggested_name=*/std::string());
-  auto picker_options = blink::mojom::FilePickerOptions::New(
-      blink::mojom::TypeSpecificFilePickerOptionsUnion::
-          NewSaveFilePickerOptions(std::move(save_file_picker_options)),
-      /*starting_directory_id=*/std::string(),
-      blink::mojom::FilePickerStartInOptionsUnionPtr());
-
-  base::test::TestFuture<blink::mojom::FileSystemAccessErrorPtr,
-                         std::vector<blink::mojom::FileSystemAccessEntryPtr>>
-      future;
-  manager_remote->ChooseEntries(std::move(picker_options),
-                                future.GetCallback());
-  ASSERT_TRUE(future.Wait());
-
-  EXPECT_EQ(future.Get<0>()->status,
-            blink::mojom::FileSystemAccessStatus::kPermissionDenied);
-  EXPECT_EQ(future.Get<0>()->message,
-            "Tab must be visible in order to show a file picker.");
-  EXPECT_EQ(future.Get<1>().size(), 0);
-}
-
 TEST_F(FileSystemAccessManagerImplTest, GetUniqueId) {
   const PathInfo kTestPathInfo(dir_.GetPath().AppendASCII("foo"));
   ASSERT_OK_AND_ASSIGN(auto default_bucket,
diff --git a/content/browser/file_system_access/file_system_chooser.cc b/content/browser/file_system_access/file_system_chooser.cc
index d510a87..e240d013 100644
--- a/content/browser/file_system_access/file_system_chooser.cc
+++ b/content/browser/file_system_access/file_system_chooser.cc
@@ -19,6 +19,7 @@
 #include "content/browser/file_system_access/file_system_access_error.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/visibility.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "net/base/mime_util.h"
@@ -293,6 +294,16 @@
     : fullscreen_block(std::move(fullscreen_block)),
       pip_tucker(std::move(pip_tucker)) {}
 
+namespace {
+// Called when no file is selected due to being aborted.

diff --git a/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc b/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
index d8073d33..3f97c77 100644
--- a/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
+++ b/chrome/browser/ui/views/file_system_access/file_system_access_browsertest.cc
@@ -10,6 +10,7 @@
 #include "base/path_service.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/task/current_thread.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_path_override.h"
 #include "base/test/test_file_util.h"
@@ -46,6 +47,7 @@
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/fenced_frame_test_util.h"
+#include "content/public/test/file_system_chooser_test_helpers.h"
 #include "content/public/test/prerender_test_util.h"
 #include "content/public/test/scoped_web_ui_controller_factory_registration.h"
 #include "content/public/test/test_navigation_observer.h"
@@ -1322,11 +1324,44 @@
 
   // Try to show a file picker in the background tab.
   // This should be blocked.
-  EXPECT_EQ("NotAllowedError",
+  EXPECT_EQ("AbortError",
             content::EvalJs(second_tab,
                             "self.showOpenFilePicker().catch(e => e.name)"));
 }
 
+// Test that opening another tab while the dialog is showing closes the dialog.
+// https://crbug.com/419721056
+IN_PROC_BROWSER_TEST_P(FileSystemAccessBrowserTest, ShowOpenFileThenHide) {
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/title1.html")));
+  content::WebContents* first_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Open the dialog and wait until it's created.
+  content::SelectFileDialogRecorder recorder;
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::ObservableSelectFileDialogFactory>(&recorder));
+  ASSERT_EQ(42,
+            content::EvalJs(
+                first_tab,
+                "window.p = self.showOpenFilePicker().catch(e => e.name); 42"));
+  ASSERT_TRUE(base::test::RunUntil([&recorder]() {
+    return recorder.state != content::SelectFileDialogRecorder::kNotCreated;
+  }));
+
+  // Create a second tab.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), embedded_test_server()->GetURL("/title2.html"),
+      WindowOpenDisposition::NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // The first tab should not be the active tab anymore.
+  ASSERT_NE(first_tab, browser()->tab_strip_model()->GetActiveWebContents());
+
+  // Check that the dialog was closed.
+  ASSERT_EQ("AbortError", content::EvalJs(first_tab, "window.p"));
+}
+
 class FileSystemAccessBrowserTestForWebUI
     : public InProcessBrowserTest,
       public ::testing::WithParamInterface<bool> {
diff --git a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
index 045041db..c2d849d2 100644
--- a/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_manager_impl_unittest.cc
@@ -2109,61 +2109,6 @@
             bad_message_observer.WaitForBadMessage());
 }
 
-TEST_F(FileSystemAccessManagerImplTest, ChooseEntries_HiddenTab) {
-  base::FilePath test_file = dir_.GetPath().AppendASCII("asdf");
-  ASSERT_TRUE(base::CreateTemporaryFile(&test_file));
-  PathInfo test_file_info(test_file);
-
-  manager_->SetFilePickerResultForTesting(test_file_info);
-
-  static_cast<TestRenderFrameHost*>(web_contents_->GetPrimaryMainFrame())
-      ->SimulateUserActivation();
-
-  mojo::Remote<blink::mojom::FileSystemAccessManager> manager_remote;
-  FileSystemAccessManagerImpl::BindingContext binding_context = {
-      kTestStorageKey, kTestURL,
-      web_contents_->GetPrimaryMainFrame()->GetGlobalId()};
-  manager_->BindReceiver(binding_context,
-                         manager_remote.BindNewPipeAndPassReceiver());
-
-  EXPECT_CALL(permission_context_,
-              CanObtainReadPermission(kTestStorageKey.origin()))
-      .WillOnce(testing::Return(true));
-  EXPECT_CALL(permission_context_,
-              CanObtainWritePermission(kTestStorageKey.origin()))
-      .WillOnce(testing::Return(true));
-  EXPECT_CALL(permission_context_, CheckPathsAgainstEnterprisePolicy(
-                                       testing::_, testing::_, testing::_))
-      .Times(0);
-
-  // Hiding the web contents should make ChooseEntries return an error.
-  web_contents_->WasHidden();
-
-  auto save_file_picker_options = blink::mojom::SaveFilePickerOptions::New(
-      blink::mojom::AcceptsTypesInfo::New(
-          std::vector<blink::mojom::ChooseFileSystemEntryAcceptsOptionPtr>(),
-          /*include_accepts_all=*/true),
-      /*suggested_name=*/std::string());
-  auto picker_options = blink::mojom::FilePickerOptions::New(
-      blink::mojom::TypeSpecificFilePickerOptionsUnion::
-          NewSaveFilePickerOptions(std::move(save_file_picker_options)),
-      /*starting_directory_id=*/std::string(),
-      blink::mojom::FilePickerStartInOptionsUnionPtr());
-
-  base::test::TestFuture<blink::mojom::FileSystemAccessErrorPtr,
-                         std::vector<blink::mojom::FileSystemAccessEntryPtr>>
-      future;
-  manager_remote->ChooseEntries(std::move(picker_options),
-                                future.GetCallback());
-  ASSERT_TRUE(future.Wait());
-
-  EXPECT_EQ(future.Get<0>()->status,
-            blink::mojom::FileSystemAccessStatus::kPermissionDenied);
-  EXPECT_EQ(future.Get<0>()->message,
-            "Tab must be visible in order to show a file picker.");
-  EXPECT_EQ(future.Get<1>().size(), 0);
-}
-
 TEST_F(FileSystemAccessManagerImplTest, GetUniqueId) {
   const PathInfo kTestPathInfo(dir_.GetPath().AppendASCII("foo"));
   ASSERT_OK_AND_ASSIGN(auto default_bucket,
diff --git a/content/browser/file_system_access/file_system_chooser_browsertest.cc b/content/browser/file_system_access/file_system_chooser_browsertest.cc
index 4248b0a3..1178ae5 100644
--- a/content/browser/file_system_access/file_system_chooser_browsertest.cc
+++ b/content/browser/file_system_access/file_system_chooser_browsertest.cc
@@ -11,6 +11,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/gmock_callback_support.h"
+#include "base/test/run_until.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_restrictions.h"
 #include "build/build_config.h"
@@ -2159,6 +2160,69 @@
             dialog_params_.title);
 }
 
+// Hide the WebContents and ensure the dialog is not shown.
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, DontShowWhileHidden) {
+  FakeFileSystemAccessPermissionContext permission_context;
+  static_cast<FileSystemAccessManagerImpl*>(
+      shell()
+          ->web_contents()
+          ->GetBrowserContext()
+          ->GetStoragePartition(shell()->web_contents()->GetSiteInstance())
+          ->GetFileSystemAccessEntryFactory())
+      ->SetPermissionContextForTesting(&permission_context);
+
+  GURL url = embedded_test_server()->GetURL("/title1.html");
+  ASSERT_TRUE(NavigateToURL(shell(), url));
+
+  // Record the state of the dialog.
+  SelectFileDialogRecorder recorder;
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<ObservableSelectFileDialogFactory>(&recorder));
+
+  // Hide the WebContents.
+  WebContents* wc = shell()->web_contents();
+  wc->UpdateWebContentsVisibility(content::Visibility::HIDDEN);
+
+  // JS should see the dialog as aborted.
+  EXPECT_EQ(
+      "AbortError",
+      content::EvalJs(wc, "window.showOpenFilePicker().catch(e => e.name)"));
+  // The dialog should not have been created.
+  EXPECT_EQ(recorder.state, SelectFileDialogRecorder::kNotCreated);
+}
+
+// Show the dialog then hide the WebContents and ensure the dialog is dismissed.
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, ShowThenHide) {
+  GURL url = embedded_test_server()->GetURL("/title1.html");
+  ASSERT_TRUE(NavigateToURL(shell(), url));
+
+  WebContents* wc = shell()->web_contents();
+
+  // Record the state of the dialog.
+  SelectFileDialogRecorder recorder;
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<ObservableSelectFileDialogFactory>(&recorder));
+
+  // Open the dialog and wait until it's created.
+  ASSERT_EQ(
+      42,
+      content::EvalJs(
+          wc, "window.p = self.showOpenFilePicker().catch(e => e.name); 42"));
+  ASSERT_TRUE(base::test::RunUntil([&recorder]() {
+    return recorder.state != SelectFileDialogRecorder::kNotCreated;
+  }));
+
+  // Hide the WebContents.
+  wc->UpdateWebContentsVisibility(
+      content::Visibility::HIDDEN);  // Hide the web contents.
+  ASSERT_TRUE(base::test::RunUntil([&recorder]() {
+    return recorder.state == SelectFileDialogRecorder::kDestroyed;
+  }));
+
+  // JS should see the dialog as aborted.
+  EXPECT_EQ("AbortError", content::EvalJs(wc, "p"));
+}
+
 class FileSystemChooserBackForwardCacheBrowserTest
     : public FileSystemChooserBrowserTest {
  public:
diff --git a/content/browser/file_system_access/file_system_chooser_unittest.cc b/content/browser/file_system_access/file_system_chooser_unittest.cc
index 1fa969e..a360293 100644
--- a/content/browser/file_system_access/file_system_chooser_unittest.cc
+++ b/content/browser/file_system_access/file_system_chooser_unittest.cc
@@ -30,13 +30,20 @@
 
 class FileSystemChooserTest : public RenderViewHostImplTestHarness {
  public:
-  void TearDown() override {
-    RenderViewHostImplTestHarness::TearDown();
-    ui::SelectFileDialog::SetFactory(nullptr);
+  void SetUp() override {
+    RenderViewHostImplTestHarness::SetUp();
+    web_contents_ = CreateTestWebContents(GetBrowserContext());
   }
 
+  void TearDown() override {
+    ui::SelectFileDialog::SetFactory(nullptr);
+    web_contents_.reset();
+    RenderViewHostImplTestHarness::TearDown();
+  }
+
+  WebContents* web_contents() { return web_contents_.get(); }
+
   std::vector<PathInfo> SyncShowDialog(
-      WebContents* web_contents,
       std::vector<blink::mojom::ChooseFileSystemEntryAcceptsOptionPtr> accepts,
       bool include_accepts_all,
       base::FilePath default_directory = base::FilePath(),
@@ -45,7 +52,7 @@
                            std::vector<PathInfo>>
         future;
     FileSystemChooser::CreateAndShow(
-        web_contents,
+        web_contents(),
         FileSystemChooser::Options(ui::SelectFileDialog::SELECT_OPEN_FILE,
                                    blink::mojom::AcceptsTypesInfo::New(
                                        std::move(accepts), include_accepts_all),
@@ -63,6 +70,7 @@
         browser_context, std::move(site_instance));
   }
 
+  std::unique_ptr<content::WebContents> web_contents_;
   // Must persist throughout TearDown().
   SelectFileDialogParams dialog_params_;
 };
@@ -70,7 +78,7 @@
 TEST_F(FileSystemChooserTest, EmptyAccepts) {
   ui::SelectFileDialog::SetFactory(
       std::make_unique<CancellingSelectFileDialogFactory>(&dialog_params_));
-  SyncShowDialog(/*web_contents=*/nullptr, {}, /*include_accepts_all=*/true);
+  SyncShowDialog({}, /*include_accepts_all=*/true);
 
   ASSERT_TRUE(dialog_params_.file_types);
   EXPECT_TRUE(dialog_params_.file_types->include_all_files);
@@ -86,7 +94,7 @@
 TEST_F(FileSystemChooserTest, EmptyAcceptsIgnoresIncludeAcceptsAll) {
   ui::SelectFileDialog::SetFactory(
       std::make_unique<CancellingSelectFileDialogFactory>(&dialog_params_));
-  SyncShowDialog(/*web_contents=*/nullptr, {}, /*include_accepts_all=*/false);
+  SyncShowDialog({}, /*include_accepts_all=*/false);
 
   // Should still include_all_files, even though include_accepts_all was false.
   ASSERT_TRUE(dialog_params_.file_types);
@@ -110,7 +118,7 @@
   accepts.emplace_back(blink::mojom::ChooseFileSystemEntryAcceptsOption::New(
       u"Images", std::vector<std::string>({"image/*"}),
       std::vector<std::string>({})));
-  SyncShowDialog(/*web_contents=*/nullptr, std::move(accepts),
+  SyncShowDialog(std::move(accepts),
                  /*include_accepts_all=*/true);
 
   ASSERT_TRUE(dialog_params_.file_types);
@@ -152,7 +160,7 @@
   accepts.emplace_back(blink::mojom::ChooseFileSystemEntryAcceptsOption::New(
       u"", std::vector<std::string>({}),
       std::vector<std::string>({"text", "js", "text"})));
-  SyncShowDialog(/*web_contents=*/nullptr, std::move(accepts),
+  SyncShowDialog(std::move(accepts),
                  /*include_accepts_all=*/true);
 
   ASSERT_TRUE(dialog_params_.file_types);
@@ -182,7 +190,7 @@
   accepts.emplace_back(blink::mojom::ChooseFileSystemEntryAcceptsOption::New(
       u"", std::vector<std::string>({"image/*"}),
       std::vector<std::string>({"text", "jpg"})));
... (truncated)