From 598baf3ab1ec91a2bccc732ccb29cda4172d1bcd Mon Sep 17 00:00:00 2001
From: Bo Liu <[email protected]>
Date: Mon, 16 Dec 2024 09:11:29 -0800
Subject: [PATCH] Disconnect file chooser on tab deactivation

The file select dialog generally does not have any indication
which tab or origin made the request. This can cause user confusion if
user or page manage to switch tabs while file select dialog is showing.

This is a small general fix to mitigate this by disconnecting (but not
dismissing) the dialog if a tab switch happens.
This CL is desktop-only.

Add a test to verify behavior, and also add a kill switch.

Bug: 380789337
Change-Id: I9864bbe7e49b3ca3fb3fc72ffc2b52cbeace43ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6088620
Commit-Queue: Bo Liu <[email protected]>
Reviewed-by: David Pennington <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1396797}
---
diff --git a/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.cc b/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.cc
new file mode 100644
index 0000000..f302989
--- /dev/null
+++ b/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.cc
@@ -0,0 +1,30 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.h"
+
+#include "base/functional/bind.h"
+#include "content/public/browser/web_contents.h"
+
+namespace tabs {
+
+DisconnectFileChooserOnBackgroundController::
+    DisconnectFileChooserOnBackgroundController(TabInterface& tab)
+    : will_enter_background_subscription_(
+          tab.RegisterWillEnterBackground(base::BindRepeating(
+              &DisconnectFileChooserOnBackgroundController::WillEnterBackground,
+              base::Unretained(this)))) {}
+
+DisconnectFileChooserOnBackgroundController::
+    ~DisconnectFileChooserOnBackgroundController() = default;
+
+void DisconnectFileChooserOnBackgroundController::WillEnterBackground(
+    TabInterface* tab) {
+  content::WebContents* web_contents = tab->GetContents();
+  if (web_contents) {
+    web_contents->DisconnectFileSelectListenerIfAny();
+  }
+}
+
+}  // namespace tabs
diff --git a/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.h b/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.h
new file mode 100644
index 0000000..29cf6be
--- /dev/null
+++ b/chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.h
@@ -0,0 +1,30 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_UI_TABS_DISCONNECT_FILE_CHOOSER_ON_BACKGROUND_CONTROLLER_H_
+#define CHROME_BROWSER_UI_TABS_DISCONNECT_FILE_CHOOSER_ON_BACKGROUND_CONTROLLER_H_
+
+#include "base/callback_list.h"
+#include "chrome/browser/ui/tabs/public/tab_interface.h"
+
+namespace tabs {
+
+// File select dialogs generally do not have any indicator for which tab
+// requested it, which can cause user confusion if the active tab changes for
+// some reason. As a small and quick fix, disconnect the listener when the tab
+// deactivates.
+class DisconnectFileChooserOnBackgroundController {
+ public:
+  explicit DisconnectFileChooserOnBackgroundController(TabInterface& tab);
+  ~DisconnectFileChooserOnBackgroundController();
+
+ private:
+  void WillEnterBackground(TabInterface* tab);
+
+  base::CallbackListSubscription will_enter_background_subscription_;
+};
+
+}  // namespace tabs
+
+#endif  // CHROME_BROWSER_UI_TABS_DISCONNECT_FILE_CHOOSER_ON_BACKGROUND_CONTROLLER_H_
diff --git a/chrome/browser/ui/tabs/features.cc b/chrome/browser/ui/tabs/features.cc
index ae50c257..980167e 100644
--- a/chrome/browser/ui/tabs/features.cc
+++ b/chrome/browser/ui/tabs/features.cc
@@ -10,6 +10,11 @@
 
 namespace tabs {
 
+// Kill switch for disconnecting file select dialog when tab is deactivated.
+BASE_FEATURE(kDisconnectFileChooserOnTabDeactivateKillSwitch,
+             "DisconnectFileChooserOnTabDeactivateKillSwitch",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 // Splits pinned and unpinned tabs into separate TabStrips.
 // https://crbug.com/1346019
 BASE_FEATURE(kSplitTabStrip,
diff --git a/chrome/browser/ui/tabs/features.h b/chrome/browser/ui/tabs/features.h
index 5033f7b..2c35d93 100644
--- a/chrome/browser/ui/tabs/features.h
+++ b/chrome/browser/ui/tabs/features.h
@@ -11,6 +11,8 @@
 
 // TODO(346837232) move all flags to this file.
 
+BASE_DECLARE_FEATURE(kDisconnectFileChooserOnTabDeactivateKillSwitch);
+
 BASE_DECLARE_FEATURE(kSplitTabStrip);
 
 BASE_DECLARE_FEATURE(kScrollableTabStrip);
diff --git a/chrome/browser/ui/tabs/public/tab_features.h b/chrome/browser/ui/tabs/public/tab_features.h
index efe1b8d..e19b3380 100644
--- a/chrome/browser/ui/tabs/public/tab_features.h
+++ b/chrome/browser/ui/tabs/public/tab_features.h
@@ -73,6 +73,7 @@
 
 namespace tabs {
 
+class DisconnectFileChooserOnBackgroundController;
 class TabInterface;
 class TabDialogManager;
 
@@ -245,6 +246,9 @@
   std::unique_ptr<passage_embeddings::EmbedderTabObserver>
       embedder_tab_observer_;
 
+  std::unique_ptr<DisconnectFileChooserOnBackgroundController>
+      disconnect_file_chooser_on_background_controller_;
+
   // Must be the last member.
   base::WeakPtrFactory<TabFeatures> weak_factory_{this};
 };
diff --git a/chrome/browser/ui/tabs/tab_features.cc b/chrome/browser/ui/tabs/tab_features.cc
index f8f9f2a4..f72cc5c 100644
--- a/chrome/browser/ui/tabs/tab_features.cc
+++ b/chrome/browser/ui/tabs/tab_features.cc
@@ -31,6 +31,8 @@
 #include "chrome/browser/ui/browser_actions.h"
 #include "chrome/browser/ui/commerce/commerce_ui_tab_helper.h"
 #include "chrome/browser/ui/lens/lens_overlay_controller.h"
+#include "chrome/browser/ui/tabs/disconnect_file_chooser_on_background_controller.h"
+#include "chrome/browser/ui/tabs/features.h"
 #include "chrome/browser/ui/tabs/public/tab_dialog_manager.h"
 #include "chrome/browser/ui/tabs/public/tab_interface.h"
 #include "chrome/browser/ui/tabs/saved_tab_groups/collaboration_messaging_tab_data.h"
@@ -202,6 +204,12 @@
           favicon::ContentFaviconDriver::FromWebContents(tab.GetContents()));
 
   task_manager::WebContentsTags::CreateForTabContents(tab.GetContents());
+
+  if (base::FeatureList::IsEnabled(
+          tabs::kDisconnectFileChooserOnTabDeactivateKillSwitch)) {
+    disconnect_file_chooser_on_background_controller_ =
+        std::make_unique<DisconnectFileChooserOnBackgroundController>(tab);
+  }
 }
 
 TabFeatures::TabFeatures() = default;
diff --git a/content/browser/web_contents/file_chooser_impl.cc b/content/browser/web_contents/file_chooser_impl.cc
index a85b27a..64a4910 100644
--- a/content/browser/web_contents/file_chooser_impl.cc
+++ b/content/browser/web_contents/file_chooser_impl.cc
@@ -209,6 +209,7 @@
   listener_impl_ = nullptr;
   if (!render_frame_host()) {
     std::move(callback_).Run(nullptr);
+    weak_factory_.InvalidateWeakPtrs();
     return;
   }
   storage::FileSystemContext* file_system_context = nullptr;
@@ -235,11 +236,14 @@
     }
   }
   std::move(callback_).Run(FileChooserResult::New(std::move(files), base_dir));
+  weak_factory_.InvalidateWeakPtrs();
 }
 
 void FileChooserImpl::FileSelectionCanceled() {
   listener_impl_ = nullptr;
   std::move(callback_).Run(nullptr);
+
+  weak_factory_.InvalidateWeakPtrs();
 }
 
 RenderFrameHostImpl* FileChooserImpl::render_frame_host() {
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index a8577b4..c2f0752 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -11614,6 +11614,12 @@
   return target_network_;
 }
 
+void WebContentsImpl::DisconnectFileSelectListenerIfAny() {
+  if (active_file_chooser_) {
+    active_file_chooser_->FileSelectionCanceled();
+  }
+}
+
 // static
 void WebContentsImpl::UpdateAttributionSupportAllRenderers() {
   for (WebContentsImpl* web_contents : GetAllWebContents()) {
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 8213039..22d9e683 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -1002,6 +1002,7 @@
   BackForwardTransitionAnimationManager*
   GetBackForwardTransitionAnimationManager() override;
   net::handles::NetworkHandle GetTargetNetwork() override;
+  void DisconnectFileSelectListenerIfAny() override;
 
   void GetMediaCaptureRawDeviceIdsOpened(
       blink::mojom::MediaStreamType type,
diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
index 0de63e45..e9c6a19 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -1644,6 +1644,10 @@
   // be bound.
   virtual net::handles::NetworkHandle GetTargetNetwork() = 0;
 
+  // Disconnect any outstanding `FileSelectListener` so that any calls will be
+  // no-op.
+  virtual void DisconnectFileSelectListenerIfAny() = 0;
+
  private:
   // This interface should only be implemented inside content.
   friend class WebContentsImpl;
diff --git a/content/shell/browser/shell.h b/content/shell/browser/shell.h
index a9e656c..8d0aba9d 100644
--- a/content/shell/browser/shell.h
+++ b/content/shell/browser/shell.h
@@ -203,6 +203,10 @@
   // Counts both RunFileChooser and EnumerateDirectory.
   size_t run_file_chooser_count() const { return run_file_chooser_count_; }
 
+  FileSelectListener* held_file_chooser_listener() const {
+    return held_file_chooser_listener_.get();
+  }
+
  private:
   class DevToolsWebContentsObserver;
 
diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 4197d93..58ed054 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/content/browser/web_contents/web_contents_impl_browsertest.cc
@@ -3190,6 +3190,44 @@
 }
 
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       DisconnectFileChooserListener) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  shell()->set_hold_file_chooser();
+
+  GURL url = embedded_test_server()->GetURL("/click-noreferrer-links.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+
+  WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell()->web_contents());
+  auto [chooser, remote] =
+      FileChooserImpl::CreateForTesting(wc->GetPrimaryMainFrame());
+  base::WeakPtr<FileChooserImpl> chooser_weak_ptr = chooser->GetWeakPtr();
+
+  // Request file chooser.
+  base::RunLoop run_loop;
+  base::OnceClosure quit_closure = run_loop.QuitClosure();
+  blink::mojom::FileChooserResultPtr result_received;
+  remote->OpenFileChooser(blink::mojom::FileChooserParams::New(),
+                          base::BindLambdaForTesting(
+                              [&](blink::mojom::FileChooserResultPtr result) {
+                                result_received = std::move(result);
+                                std::move(quit_closure).Run();
+                              }));
+  remote.FlushForTesting();
+  EXPECT_EQ(shell()->run_file_chooser_count(), 1u);
+
+  // Disconnect listener.
+  wc->DisconnectFileSelectListenerIfAny();
+
+  // Send result from listener, which now should be ignored.
+  shell()->held_file_chooser_listener()->FileSelected(
+      {}, base::FilePath(), blink::mojom::FileChooserParams::Mode::kOpen);
+  run_loop.Run();
+
+  // Check that request was cancelled, ie returned null result.
+  EXPECT_FALSE(result_received);
+}
+
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
                        FrameDetachInCopyDoesNotCrash) {
   ASSERT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(NavigateToURL(
diff --git a/chrome/browser/ui/tabs/BUILD.gn b/chrome/browser/ui/tabs/BUILD.gn
index bd18f822..972471eb 100644
--- a/chrome/browser/ui/tabs/BUILD.gn
+++ b/chrome/browser/ui/tabs/BUILD.gn
@@ -288,6 +288,8 @@
     public_deps = [ "//chrome/browser:browser_public_dependencies" ]
 
     sources = [
+      "disconnect_file_chooser_on_background_controller.cc",
+      "disconnect_file_chooser_on_background_controller.h",
       "features.cc",
       "hover_tab_selector.cc",
       "pinned_tab_codec.cc",

diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 4197d93..58ed054 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/content/browser/web_contents/web_contents_impl_browsertest.cc
@@ -3190,6 +3190,44 @@
 }
 
 IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       DisconnectFileChooserListener) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  shell()->set_hold_file_chooser();
+
+  GURL url = embedded_test_server()->GetURL("/click-noreferrer-links.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url));
+
+  WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell()->web_contents());
+  auto [chooser, remote] =
+      FileChooserImpl::CreateForTesting(wc->GetPrimaryMainFrame());
+  base::WeakPtr<FileChooserImpl> chooser_weak_ptr = chooser->GetWeakPtr();
+
+  // Request file chooser.
+  base::RunLoop run_loop;
+  base::OnceClosure quit_closure = run_loop.QuitClosure();
+  blink::mojom::FileChooserResultPtr result_received;
+  remote->OpenFileChooser(blink::mojom::FileChooserParams::New(),
+                          base::BindLambdaForTesting(
+                              [&](blink::mojom::FileChooserResultPtr result) {
+                                result_received = std::move(result);
+                                std::move(quit_closure).Run();
+                              }));
+  remote.FlushForTesting();
+  EXPECT_EQ(shell()->run_file_chooser_count(), 1u);
+
+  // Disconnect listener.
+  wc->DisconnectFileSelectListenerIfAny();
+
+  // Send result from listener, which now should be ignored.
+  shell()->held_file_chooser_listener()->FileSelected(
+      {}, base::FilePath(), blink::mojom::FileChooserParams::Mode::kOpen);
+  run_loop.Run();
+
+  // Check that request was cancelled, ie returned null result.
+  EXPECT_FALSE(result_received);
+}
+
+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
                        FrameDetachInCopyDoesNotCrash) {
   ASSERT_TRUE(embedded_test_server()->Start());
   EXPECT_TRUE(NavigateToURL(