Fix a reentrance issue when showing file picker dialog

If we have multiple file picker dialogs, we queue the callbacks in a deque
so that they will be handled sequentially. However, if one of the download
is removed while enqueued, OnConfirmationCallbackComplete() call to
ShowFilePicker() will cause an reentrance into OnConfirmationCallbackComplete().
This CL fixes the issue by posing a task to call ShowFilePicker().
Unit test is also added.

BUG=904319

Change-Id: I00d324a06977a173671a3405594371ccbdc4a5fc
Reviewed-on: https://chromium-review.googlesource.com/c/1332776Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608040}

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -986,7 +986,8 @@ void ChromeDownloadManagerDelegate::OnConfirmationCallbackComplete(
   callback.Run(result, virtual_path);
   if (!file_picker_callbacks_.empty()) {
     base::OnceClosure callback = std::move(file_picker_callbacks_.front());
-    std::move(callback).Run();
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback)));
     file_picker_callbacks_.pop_front();
   } else {
     is_file_picker_showing_ = false;
@@ -999,17 +1000,25 @@ void ChromeDownloadManagerDelegate::ShowFilePicker(
     const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback) {
   DownloadItem* download = download_manager_->GetDownloadByGuid(guid);
   if (download) {
-    DownloadFilePicker::ShowFilePicker(
-        download, suggested_path,
-        base::BindRepeating(
-            &ChromeDownloadManagerDelegate::OnConfirmationCallbackComplete,
-            weak_ptr_factory_.GetWeakPtr(), callback));
+    ShowFilePickerForDownload(download, suggested_path, callback);
   } else {
     OnConfirmationCallbackComplete(
         callback, DownloadConfirmationResult::CANCELED, base::FilePath());
   }
 }
 
+void ChromeDownloadManagerDelegate::ShowFilePickerForDownload(
+    DownloadItem* download,
+    const base::FilePath& suggested_path,
+    const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback) {
+  DCHECK(download);
+  DownloadFilePicker::ShowFilePicker(
+      download, suggested_path,
+      base::BindRepeating(
+          &ChromeDownloadManagerDelegate::OnConfirmationCallbackComplete,
+          weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
 #if defined(OS_ANDROID)
 void ChromeDownloadManagerDelegate::GenerateUniqueFileNameDone(
     gfx::NativeWindow native_window,

chrome/browser/download/chrome_download_manager_delegate.h

@@ -134,9 +134,9 @@ class ChromeDownloadManagerDelegate
   virtual safe_browsing::DownloadProtectionService*
       GetDownloadProtectionService();
 
-  // Called to show a file picker
-  virtual void ShowFilePicker(
-      const std::string& guid,
+  // Show file picker for |download|.
+  virtual void ShowFilePickerForDownload(
+      download::DownloadItem* download,
       const base::FilePath& suggested_path,
       const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback);
 
@@ -186,6 +186,12 @@ class ChromeDownloadManagerDelegate
 
   typedef std::vector<content::DownloadIdCallback> IdCallbackVector;
 
+  // Called to show a file picker for download with |guid|
+  void ShowFilePicker(
+      const std::string& guid,
+      const base::FilePath& suggested_path,
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback);
+
   // content::NotificationObserver implementation.
   void Observe(int type,
                const content::NotificationSource& source,

chrome/browser/download/chrome_download_manager_delegate_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/guid.h"
 #include "base/location.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
@@ -213,6 +214,14 @@ class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate {
     ChromeDownloadManagerDelegate::RequestConfirmation(download_item, path,
                                                        reason, callback);
   }
+
+  void ShowFilePickerForDownload(
+      DownloadItem* download,
+      const base::FilePath& path,
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback&) override {}
+
+ private:
+  friend class ChromeDownloadManagerDelegateTest;
 };
 
 class ChromeDownloadManagerDelegateTest
@@ -246,6 +255,11 @@ class ChromeDownloadManagerDelegateTest
   // method.
   bool CheckForFileExistence(DownloadItem* download);
 
+  void OnConfirmationCallbackComplete(
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback,
+      DownloadConfirmationResult result,
+      const base::FilePath& virtual_path);
+
   base::FilePath GetDefaultDownloadPath() const;
   TestChromeDownloadManagerDelegate* delegate();
   content::MockDownloadManager* download_manager();
@@ -327,9 +341,13 @@ ChromeDownloadManagerDelegateTest::CreateActiveDownloadItem(int32_t id) {
       .WillByDefault(Return(false));
   ON_CALL(*item, IsTemporary())
       .WillByDefault(Return(false));
+  std::string guid = base::GenerateGUID();
+  ON_CALL(*item, GetGuid()).WillByDefault(ReturnRefOfCopy(guid));
   content::DownloadItemUtils::AttachInfo(item.get(), profile(), web_contents());
   EXPECT_CALL(*download_manager_, GetDownload(id))
       .WillRepeatedly(Return(item.get()));
+  EXPECT_CALL(*download_manager_, GetDownloadByGuid(guid))
+      .WillRepeatedly(Return(item.get()));
   return item;
 }
 
@@ -391,6 +409,13 @@ base::FilePath ChromeDownloadManagerDelegateTest::GetDefaultDownloadPath()
   return path;
 }
 
+void ChromeDownloadManagerDelegateTest::OnConfirmationCallbackComplete(
+    const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback,
+    DownloadConfirmationResult result,
+    const base::FilePath& virtual_path) {
+  delegate_->OnConfirmationCallbackComplete(callback, result, virtual_path);
+}
+
 TestChromeDownloadManagerDelegate*
     ChromeDownloadManagerDelegateTest::delegate() {
   return delegate_.get();
@@ -711,6 +736,76 @@ TEST_F(ChromeDownloadManagerDelegateTest, WithHistoryDbNextId) {
   EXPECT_EQ(expected_ids, download_ids());
 }
 
+#if !defined(OS_ANDROID)
+namespace {
+// Verify the file picker confirmation result matches |expected_result|. Run
+// |completion_closure| on completion.
+void VerifyFilePickerConfirmation(DownloadConfirmationResult expected_result,
+                                  base::RepeatingClosure completion_closure,
+                                  DownloadConfirmationResult result,
+                                  const base::FilePath& virtual_path) {
+  ASSERT_EQ(result, expected_result);
+  base::ResetAndReturn(&completion_closure).Run();
+}
+}  // namespace
+
+// Test that it is fine to remove a download before its file picker is being
+// shown.
+TEST_F(ChromeDownloadManagerDelegateTest,
+       RemovingDownloadBeforeShowingFilePicker) {
+  GURL download_url("http://example.com/foo.txt");
+
+  std::unique_ptr<download::MockDownloadItem> download1 =
+      CreateActiveDownloadItem(0);
+  EXPECT_CALL(*download1, GetURL())
+      .Times(AnyNumber())
+      .WillRepeatedly(ReturnRef(download_url));
+  EXPECT_CALL(*download1, GetTargetDisposition())
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(DownloadItem::TARGET_DISPOSITION_PROMPT));
+
+  std::unique_ptr<download::MockDownloadItem> download2 =
+      CreateActiveDownloadItem(1);
+  EXPECT_CALL(*download2, GetURL())
+      .Times(AnyNumber())
+      .WillRepeatedly(ReturnRef(download_url));
+  EXPECT_CALL(*download2, GetTargetDisposition())
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(DownloadItem::TARGET_DISPOSITION_PROMPT));
+
+  EXPECT_CALL(*delegate(), RequestConfirmation(_, _, _, _))
+      .WillRepeatedly(Invoke(
+          delegate(),
+          &TestChromeDownloadManagerDelegate::RequestConfirmationConcrete));
+
+  base::FilePath expected_prompt_path(GetPathInDownloadDir("foo.txt"));
+  delegate()->RequestConfirmation(download1.get(), expected_prompt_path,
+                                  DownloadConfirmationReason::NAME_TOO_LONG,
+                                  base::DoNothing());
+  base::RunLoop run_loop;
+  // Verify that the second download's file picker will be canceled, because
+  // it will be removed from the DownloadManager.
+  delegate()->RequestConfirmation(
+      download2.get(), expected_prompt_path,
+      DownloadConfirmationReason::NAME_TOO_LONG,
+      base::BindRepeating(&VerifyFilePickerConfirmation,
+                          DownloadConfirmationResult::CANCELED,
+                          run_loop.QuitClosure()));
+  // Make the manager no longer return the 2nd download as if the latter is
+  // removed.
+  EXPECT_CALL(*download_manager(), GetDownloadByGuid(download2->GetGuid()))
+      .WillRepeatedly(Return(nullptr));
+  // Complete the first download, so the second download's file picker should
+  // be handled. And since the second download is removed from the manager,
+  // the file picker should be canceled.
+  OnConfirmationCallbackComplete(base::DoNothing(),
+                                 DownloadConfirmationResult::CONFIRMED,
+                                 expected_prompt_path);
+
+  run_loop.Run();
+}
+#endif  // OS_ANDROID
+
 #if defined(FULL_SAFE_BROWSING)
 namespace {
 

chrome/browser/download/download_test_file_activity_observer.cc

@@ -52,8 +52,8 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
   }
 
  protected:
-  void ShowFilePicker(
-      const std::string& guid,
+  void ShowFilePickerForDownload(
+      download::DownloadItem* download,
       const base::FilePath& suggested_path,
       const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback)
       override {