From 79649f9623c5d46ec339b1c1bc34bc4d490d56c1 Mon Sep 17 00:00:00 2001
From: Yaw Frempong <yawfrempong@google.com>
Date: Thu, 30 Jul 2026 14:02:44 -0700
Subject: [PATCH] [Downloads] Defer completion while content check is pending

DownloadItemImplDelegate::ShouldCompleteDownload() is the gate
DownloadItemImpl consults before renaming a download to its final path.
The base implementation returned true unconditionally.
InProgressDownloadManager inherits this stub, so on Android when a
download with danger type MAYBE_DANGEROUS_CONTENT is resumed in
minimal-browser mode it could reach RenameAndAnnotate() before its
content check ran (that danger type returns false for IsDangerous()).

Make the base implementation return false for the content-check-pending
danger types so the download stays IN_PROGRESS until a delegate that can
run the check takes over. DownloadManagerImpl already overrides this
method, so full-browser behaviour is unchanged.

Reviewed in https://crrev.com/i/9585676

Bug: 497986036
Change-Id: I391289156e8aa68eeaeadbfc6cd4f886aba906d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8164176
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Yaw Frempong <yawfrempong@google.com>
Cr-Commit-Position: refs/heads/main@{#1671401}
---
diff --git a/components/download/internal/common/download_item_impl_delegate.cc b/components/download/internal/common/download_item_impl_delegate.cc
index 53c3082..797254c 100644
--- a/components/download/internal/common/download_item_impl_delegate.cc
+++ b/components/download/internal/common/download_item_impl_delegate.cc
@@ -48,7 +48,17 @@
 bool DownloadItemImplDelegate::ShouldCompleteDownload(
     DownloadItemImpl* download,
     base::OnceClosure complete_callback) {
-  return true;
+  // The default delegate has no way to run a content check, so defer
+  // completion of downloads whose danger type still indicates a pending
+  // verdict until a delegate that can resolve it takes over.
+  switch (download->GetDangerType()) {
+    case DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT:
+    case DOWNLOAD_DANGER_TYPE_ASYNC_SCANNING:
+    case DOWNLOAD_DANGER_TYPE_ASYNC_LOCAL_PASSWORD_SCANNING:
+      return false;
+    default:
+      return true;
+  }
 }
 
 bool DownloadItemImplDelegate::ShouldOpenDownload(
diff --git a/components/download/internal/common/download_item_impl_unittest.cc b/components/download/internal/common/download_item_impl_unittest.cc
index e368d7e..bd52c17 100644
--- a/components/download/internal/common/download_item_impl_unittest.cc
+++ b/components/download/internal/common/download_item_impl_unittest.cc
@@ -27,6 +27,7 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/test/gmock_move_support.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/run_until.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread.h"
@@ -2015,6 +2016,37 @@
   EXPECT_EQ(DownloadItem::COMPLETE, item->GetState());
 }
 
+// The default DownloadItemImplDelegate (used when no embedder-level delegate
+// is attached, e.g. by InProgressDownloadManager) must defer completion of a
+// download whose content check is still pending so the file is not renamed to
+// its final path before the check resolves.
+TEST_F(DownloadItemTest,
+       DefaultDelegateDefersCompletionForPendingContentCheck) {
+  DownloadItemImpl* item = CreateDownloadItem();
+  MockDownloadFile* download_file =
+      DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT);
+  EXPECT_FALSE(item->IsDangerous());
+  EXPECT_EQ(DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+            item->GetDangerType());
+
+  EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload_(item, _))
+      .WillRepeatedly([&](DownloadItemImpl* download, base::OnceClosure& cb) {
+        return mock_delegate()
+            ->DownloadItemImplDelegate::ShouldCompleteDownload(download,
+                                                               std::move(cb));
+      });
+  EXPECT_CALL(*download_file, RenameAndAnnotate(_, _, _, _, _, _, _)).Times(0);
+  item->DestinationObserverAsWeakPtr()->DestinationCompleted(
+      0, std::unique_ptr<crypto::SecureHash>());
+  ASSERT_TRUE(base::test::RunUntil([&]() { return item->AllDataSaved(); }));
+
+  EXPECT_TRUE(item->AllDataSaved());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_EQ(DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+            item->GetDangerType());
+  CleanupItem(item, download_file, DownloadItem::IN_PROGRESS);
+}
+
 TEST_F(DownloadItemTest, CopyDownload) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file =

diff --git a/components/download/internal/common/download_item_impl_unittest.cc b/components/download/internal/common/download_item_impl_unittest.cc
index e368d7e..bd52c17 100644
--- a/components/download/internal/common/download_item_impl_unittest.cc
+++ b/components/download/internal/common/download_item_impl_unittest.cc
@@ -27,6 +27,7 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/test/gmock_move_support.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/run_until.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread.h"
@@ -2015,6 +2016,37 @@
   EXPECT_EQ(DownloadItem::COMPLETE, item->GetState());
 }
 
+// The default DownloadItemImplDelegate (used when no embedder-level delegate
+// is attached, e.g. by InProgressDownloadManager) must defer completion of a
+// download whose content check is still pending so the file is not renamed to
+// its final path before the check resolves.
+TEST_F(DownloadItemTest,
+       DefaultDelegateDefersCompletionForPendingContentCheck) {
+  DownloadItemImpl* item = CreateDownloadItem();
+  MockDownloadFile* download_file =
+      DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT);
+  EXPECT_FALSE(item->IsDangerous());
+  EXPECT_EQ(DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+            item->GetDangerType());
+
+  EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload_(item, _))
+      .WillRepeatedly([&](DownloadItemImpl* download, base::OnceClosure& cb) {
+        return mock_delegate()
+            ->DownloadItemImplDelegate::ShouldCompleteDownload(download,
+                                                               std::move(cb));
+      });
+  EXPECT_CALL(*download_file, RenameAndAnnotate(_, _, _, _, _, _, _)).Times(0);
+  item->DestinationObserverAsWeakPtr()->DestinationCompleted(
+      0, std::unique_ptr<crypto::SecureHash>());
+  ASSERT_TRUE(base::test::RunUntil([&]() { return item->AllDataSaved(); }));
+
+  EXPECT_TRUE(item->AllDataSaved());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+  EXPECT_EQ(DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+            item->GetDangerType());
+  CleanupItem(item, download_file, DownloadItem::IN_PROGRESS);
+}
+
 TEST_F(DownloadItemTest, CopyDownload) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file =