From c895c6a67733145887ad8d753ec6a2d94712b98f Mon Sep 17 00:00:00 2001
From: Etienne Bergeron <etienneb@google.com>
Date: Tue, 21 Jul 2026 08:17:20 -0700
Subject: [PATCH] Fix WebContentsViewAura race condition during overlapping virtual file drops

This CL fixes a race condition where a second drag-and-drop operation
could overwrite the AsyncDropNavigationObserver of a pending drop. This
was because the observer was stored as a single member variable on
WebContentsViewAura.

To fix this, the AsyncDropNavigationObserver is moved to the
OnPerformingDropContext struct, which is unique to each drop operation.
This ensures that each pending drop has its own observer and is checked
against the correct navigation state when the async file extraction
completes.

This CL also adds:
- A regression test for the overlapping drops race condition.
- A test for WebContents destruction during extraction.
- A test for navigations in unrelated WebContents.

Bug: 503720291
Change-Id: If454907bba37039a034e00e225ca24d7327b0447
Fixed: 503720291
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8118986
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/main@{#1665502}
---

diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc
index 4c1ab7b..558846be 100644
--- a/content/browser/web_contents/web_contents_view_aura.cc
+++ b/content/browser/web_contents/web_contents_view_aura.cc
@@ -383,28 +383,6 @@
   flags = event.flags();
 }
 
-WebContentsViewAura::OnPerformingDropContext::OnPerformingDropContext(
-    RenderWidgetHostImpl* target_rwh,
-    std::unique_ptr<DropData> drop_data,
-    DropMetadata drop_metadata,
-    std::unique_ptr<ui::OSExchangeData> data,
-    base::ScopedClosureRunner drop_exit_cleanup,
-    std::optional<gfx::PointF> transformed_pt,
-    gfx::PointF screen_pt)
-    : target_rwh(target_rwh->GetWeakPtr()),
-      drop_data(std::move(drop_data)),
-      drop_metadata(drop_metadata),
-      data(std::move(data)),
-      drop_exit_cleanup(std::move(drop_exit_cleanup)),
-      transformed_pt(std::move(transformed_pt)),
-      screen_pt(screen_pt) {}
-
-WebContentsViewAura::OnPerformingDropContext::OnPerformingDropContext(
-    OnPerformingDropContext&&) = default;
-
-WebContentsViewAura::OnPerformingDropContext::~OnPerformingDropContext() =
-    default;
-
 #if BUILDFLAG(IS_WIN)
 // A web contents observer that watches for navigations while an async drop
 // operation is in progress during virtual file data retrieval and temp file
@@ -453,6 +431,31 @@
     drop_allowed_ = false;
   }
 }
+#endif  // BUILDFLAG(IS_WIN)
+
+WebContentsViewAura::OnPerformingDropContext::OnPerformingDropContext(
+    RenderWidgetHostImpl* target_rwh,
+    std::unique_ptr<DropData> drop_data,
+    DropMetadata drop_metadata,
+    std::unique_ptr<ui::OSExchangeData> data,
+    base::ScopedClosureRunner drop_exit_cleanup,
+    std::optional<gfx::PointF> transformed_pt,
+    gfx::PointF screen_pt)
+    : target_rwh(target_rwh->GetWeakPtr()),
+      drop_data(std::move(drop_data)),
+      drop_metadata(drop_metadata),
+      data(std::move(data)),
+      drop_exit_cleanup(std::move(drop_exit_cleanup)),
+      transformed_pt(std::move(transformed_pt)),
+      screen_pt(screen_pt) {}
+
+WebContentsViewAura::OnPerformingDropContext::OnPerformingDropContext(
+    OnPerformingDropContext&&) = default;
+
+WebContentsViewAura::OnPerformingDropContext::~OnPerformingDropContext() =
+    default;
+
+#if BUILDFLAG(IS_WIN)
 
 // Deletes registered temp files asynchronously when the object goes out of
 // scope (when the WebContentsViewAura is deleted on tab closure).
@@ -1531,9 +1534,6 @@
     return;
   }
 
-#if BUILDFLAG(IS_WIN)
-  async_drop_navigation_observer_.reset();
-#endif
 
   std::unique_ptr<DropData> drop_data = std::make_unique<DropData>();
   // Calling this here as event.data might become invalid inside the callback.
@@ -1789,7 +1789,7 @@
     // written to temporary files, the OnGotVirtualFilesAsTempFiles
     // callback will be invoked and the drop communicated to the renderer
     // process.
-    async_drop_navigation_observer_ =
+    drop_context.navigation_observer =
         std::make_unique<AsyncDropNavigationObserver>(web_contents_);
     ui::OSExchangeData* data_ptr = drop_context.data.get();
     data_ptr->GetVirtualFilesAsTempFiles(base::BindOnce(
@@ -1920,13 +1920,13 @@
     OnPerformingDropContext drop_context,
     const std::vector<std::pair<base::FilePath, base::FilePath>>&
         filepaths_and_names) {
-  if (!async_drop_navigation_observer_) {
+  if (!drop_context.navigation_observer) {
     return;
   }
 
   if (!filepaths_and_names.empty()) {
     std::unique_ptr<AsyncDropNavigationObserver> drop_observer(
-        std::move(async_drop_navigation_observer_));
+        std::move(drop_context.navigation_observer));
 
     RenderWidgetHostImpl* target_rwh = drop_context.target_rwh.get();
 
diff --git a/content/browser/web_contents/web_contents_view_aura.h b/content/browser/web_contents/web_contents_view_aura.h
index 6bcab47..679a14f 100644
--- a/content/browser/web_contents/web_contents_view_aura.h
+++ b/content/browser/web_contents/web_contents_view_aura.h
@@ -96,6 +96,10 @@
     int flags;
   };
 
+#if BUILDFLAG(IS_WIN)
+  class AsyncDropNavigationObserver;
+#endif
+
   // A structure used to keep drop context for asynchronously finishing a
   // drop operation.  This is required because some drop event data gets
   // cleared out once PerformDropCallback() returns.
@@ -120,6 +124,11 @@
     base::ScopedClosureRunner drop_exit_cleanup;
     std::optional<gfx::PointF> transformed_pt;
     gfx::PointF screen_pt;
+#if BUILDFLAG(IS_WIN)
+    // Watches for navigations that complete while virtual file retrieval is in
+    // progress so that this drop can be disallowed if the page changes.
+    std::unique_ptr<AsyncDropNavigationObserver> navigation_observer;
+#endif
   };
 
   friend class WebContentsViewAuraTest;
@@ -132,6 +141,14 @@
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest, DragDropVirtualFiles);
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest,
                            DragDropVirtualFilesOriginateFromRenderer);
+  FRIEND_TEST_ALL_PREFIXES(
+      WebContentsViewAuraTest,
+      DragDropVirtualFilesNavigationObservedAcrossOverlappingDrops);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest,
+                           DragDropVirtualFiles_DestroyDuringExtraction);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest,
+                           DragDropVirtualFiles_UnrelatedNavigation);
+
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest, DragDropUrlData);
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest, DragDropOnOopif);
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest,
@@ -370,8 +387,6 @@
                                   /*display name*/ base::FilePath>>&
           filepaths_and_names);
 
-  class AsyncDropNavigationObserver;
-  std::unique_ptr<AsyncDropNavigationObserver> async_drop_navigation_observer_;
 
   class AsyncDropTempFileDeleter;
   std::unique_ptr<AsyncDropTempFileDeleter> async_drop_temp_file_deleter_;
diff --git a/content/browser/web_contents/web_contents_view_aura_unittest.cc b/content/browser/web_contents/web_contents_view_aura_unittest.cc
index ce85bc8d..e9446a7e 100644
--- a/content/browser/web_contents/web_contents_view_aura_unittest.cc
+++ b/content/browser/web_contents/web_contents_view_aura_unittest.cc
@@ -15,12 +15,14 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/single_thread_task_runner.h"
+#include "base/test/bind.h"
 #include "base/test/scoped_command_line.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/content_features.h"
+#include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #include "testing/gtest/include/gtest/gtest.h"

diff --git a/content/browser/web_contents/web_contents_view_aura_unittest.cc b/content/browser/web_contents/web_contents_view_aura_unittest.cc
index ce85bc8d..e9446a7e 100644
--- a/content/browser/web_contents/web_contents_view_aura_unittest.cc
+++ b/content/browser/web_contents/web_contents_view_aura_unittest.cc
@@ -15,12 +15,14 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/single_thread_task_runner.h"
+#include "base/test/bind.h"
 #include "base/test/scoped_command_line.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/content_features.h"
+#include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_renderer_host.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/client/aura_constants.h"
@@ -128,6 +130,36 @@
   std::optional<ui::mojom::DragEventSource> last_source_;
 };
 
+#if BUILDFLAG(IS_WIN)
+// An OSExchangeDataProvider that exposes virtual files but lets the test
+// control when the temp-file retrieval callback is invoked.
+class DeferredVirtualFileProvider : public ui::OSExchangeDataProviderWin {
+ public:
+  using TempFilesCallback = base::OnceCallback<void(
+      const std::vector<std::pair<base::FilePath, base::FilePath>>&)>;
+
+  bool HasVirtualFilenames() const override { return true; }
+
+  std::optional<std::vector<ui::FileInfo>> GetVirtualFilenames()
+      const override {
+    return std::vector<ui::FileInfo>{
+        {base::FilePath(FILE_PATH_LITERAL("temp.tmp")),
+         base::FilePath(FILE_PATH_LITERAL("file.txt"))}};
+  }
+
+  void GetVirtualFilesAsTempFiles(TempFilesCallback callback) const override {
+    pending_callback_ = std::move(callback);
+  }
+
+  TempFilesCallback TakePendingCallback() {
+    return std::move(pending_callback_);
+  }
+
+ private:
+  mutable TempFilesCallback pending_callback_;
+};
+#endif  // BUILDFLAG(IS_WIN)
+
 }  // namespace
 
 class WebContentsViewAuraTest : public RenderViewHostTestHarness {
@@ -637,6 +669,147 @@
   }
 }
 
+// Ensures that when two virtual file drops overlap, navigations during the
+// first drop's extraction are correctly respected and disallow the drop.
+// This is a regression test for https://crbug.com/503720291.
+TEST_F(WebContentsViewAuraTest,
+       DragDropVirtualFilesNavigationObservedAcrossOverlappingDrops) {
+  WebContentsViewAura* view = GetView();
+
+  // First virtual file drop.
+  auto first_provider = std::make_unique<DeferredVirtualFileProvider>();
+  DeferredVirtualFileProvider* first_provider_ptr = first_provider.get();
+  auto first_data =
+      std::make_unique<ui::OSExchangeData>(std::move(first_provider));
+  ui::DropTargetEvent first_event(*first_data.get(), kClientPt, kScreenPt,
+                                  ui::DragDropTypes::DRAG_COPY);
+  view->OnDragEntered(first_event);
+  ASSERT_NE(nullptr, view->current_drag_data_);
+
+  std::optional<bool> first_drop_allowed;
+  view->RegisterDropCallbackForTesting(base::BindLambdaForTesting(
+      [&](RenderWidgetHostImpl*, const DropData&, const gfx::PointF&,
+          const gfx::PointF&, int,
+          bool drop_allowed) { first_drop_allowed = drop_allowed; }));
+
+  ui::mojom::DragOperation output_drag_op = ui::mojom::DragOperation::kNone;
+  auto first_drop_cb = view->GetDropCallback(first_event);
+  ASSERT_TRUE(first_drop_cb);
+  std::move(first_drop_cb)
+      .Run(std::move(first_data), output_drag_op,
+           /*drag_image_layer_owner=*/nullptr);
+  DeferredVirtualFileProvider::TempFilesCallback first_temp_files_cb =
+      first_provider_ptr->TakePendingCallback();
+  ASSERT_TRUE(first_temp_files_cb);
+
+  // A navigation completes while the first drop's temp-file retrieval is
+  // pending.
+  NavigateAndCommit(GURL("https://b.test/"));
+
+  // A second virtual file drag enters and is dropped while the first drop's
+  // temp-file retrieval is still pending.
+  auto second_provider = std::make_unique<DeferredVirtualFileProvider>();
+  DeferredVirtualFileProvider* second_provider_ptr = second_provider.get();
+  auto second_data =
+      std::make_unique<ui::OSExchangeData>(std::move(second_provider));
+  ui::DropTargetEvent second_event(*second_data.get(), kClientPt, kScreenPt,
+                                   ui::DragDropTypes::DRAG_COPY);
+  view->OnDragEntered(second_event);
+  ASSERT_NE(nullptr, view->current_drag_data_);
+
+  auto second_drop_cb = view->GetDropCallback(second_event);
+  ASSERT_TRUE(second_drop_cb);
+  std::move(second_drop_cb)
+      .Run(std::move(second_data), output_drag_op,
+           /*drag_image_layer_owner=*/nullptr);
+  DeferredVirtualFileProvider::TempFilesCallback second_temp_files_cb =
+      second_provider_ptr->TakePendingCallback();
+  ASSERT_TRUE(second_temp_files_cb);
+
+  // Temp-file retrieval for the first drop completes.
+  std::move(first_temp_files_cb)
+      .Run({{base::FilePath(FILE_PATH_LITERAL("first.tmp")),
+             base::FilePath(FILE_PATH_LITERAL("file.txt"))}});
+
+  // The first drop must be disallowed because the page navigated after that
+  // drop was initiated, regardless of any later drag activity.
+  ASSERT_TRUE(first_drop_allowed.has_value());
+  EXPECT_FALSE(first_drop_allowed.value());
+}
+
+TEST_F(WebContentsViewAuraTest, DragDropVirtualFiles_DestroyDuringExtraction) {
+  WebContentsViewAura* view = GetView();
+
+  // First virtual file drop.
+  auto provider = std::make_unique<DeferredVirtualFileProvider>();
+  DeferredVirtualFileProvider* provider_ptr = provider.get();
+  auto data = std::make_unique<ui::OSExchangeData>(std::move(provider));
+  ui::DropTargetEvent event(*data.get(), kClientPt, kScreenPt,
+                            ui::DragDropTypes::DRAG_COPY);
+  view->OnDragEntered(event);
+
+  auto drop_cb = view->GetDropCallback(event);
+  ASSERT_TRUE(drop_cb);
+  ui::mojom::DragOperation output_drag_op = ui::mojom::DragOperation::kNone;
+  std::move(drop_cb).Run(std::move(data), output_drag_op,
+                         /*drag_image_layer_owner=*/nullptr);
+  DeferredVirtualFileProvider::TempFilesCallback temp_files_cb =
+      provider_ptr->TakePendingCallback();
+  ASSERT_TRUE(temp_files_cb);
+
+  // Destroy WebContents (and thus WebContentsViewAura) while extraction is
+  // pending.
+  DeleteContents();
+
+  // Now run the callback. It should not crash.
+  std::move(temp_files_cb)
+      .Run({{base::FilePath(FILE_PATH_LITERAL("first.tmp")),
+             base::FilePath(FILE_PATH_LITERAL("file.txt"))}});
+}
+
+TEST_F(WebContentsViewAuraTest, DragDropVirtualFiles_UnrelatedNavigation) {
+  WebContentsViewAura* view = GetView();
+
+  NavigateAndCommit(GURL("https://a.test/"));
+
+  // First virtual file drop.
+  auto provider = std::make_unique<DeferredVirtualFileProvider>();
+  DeferredVirtualFileProvider* provider_ptr = provider.get();
+  auto data = std::make_unique<ui::OSExchangeData>(std::move(provider));
+  ui::DropTargetEvent event(*data.get(), kClientPt, kScreenPt,
+                            ui::DragDropTypes::DRAG_COPY);
+  view->OnDragEntered(event);
+
+  std::optional<bool> drop_allowed;
+  view->RegisterDropCallbackForTesting(base::BindLambdaForTesting(
+      [&](RenderWidgetHostImpl*, const DropData&, const gfx::PointF&,
+          const gfx::PointF&, int, bool allowed) { drop_allowed = allowed; }));
+
+  auto drop_cb = view->GetDropCallback(event);
+  ASSERT_TRUE(drop_cb);
+  ui::mojom::DragOperation output_drag_op = ui::mojom::DragOperation::kNone;
+  std::move(drop_cb).Run(std::move(data), output_drag_op,
+                         /*drag_image_layer_owner=*/nullptr);
+  DeferredVirtualFileProvider::TempFilesCallback temp_files_cb =
+      provider_ptr->TakePendingCallback();
+  ASSERT_TRUE(temp_files_cb);
+
+  // Create and navigate an unrelated WebContents.
+  std::unique_ptr<WebContents> unrelated_contents = CreateTestWebContents();
+  content::NavigationSimulator::NavigateAndCommitFromBrowser(
+      unrelated_contents.get(), GURL("https://unrelated.test/"));
+
+  // Temp-file retrieval for the drop completes.
+  std::move(temp_files_cb)
+      .Run({{base::FilePath(FILE_PATH_LITERAL("first.tmp")),
+             base::FilePath(FILE_PATH_LITERAL("file.txt"))}});
+
+  // The drop should still be allowed because the navigation was in an unrelated
+  // WebContents.
+  ASSERT_TRUE(drop_allowed.has_value());
+  EXPECT_TRUE(drop_allowed.value());
+}
+
 TEST_F(WebContentsViewAuraTest, DragDropVirtualFilesOriginateFromRenderer) {
   WebContentsViewAura* view = GetView();
   auto data = std::make_unique<ui::OSExchangeData>();