Fix an DOS issue using download prompt dialog

This CL changes the behavior to show dialog one by one,
not showing all of them at once.
This CL also fixes an issue that DownloadFilePicker is not
deleted in some cases.

BUG=868619

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

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -300,6 +300,7 @@ ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
       disk_access_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
            base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
+      is_file_picker_showing_(false),
       weak_ptr_factory_(this) {
 #if defined(OS_ANDROID)
   location_dialog_bridge_.reset(new DownloadLocationDialogBridgeImpl);
@@ -961,10 +962,49 @@ void ChromeDownloadManagerDelegate::RequestConfirmation(
 #else   // !OS_ANDROID
   // Desktop Chrome displays a file picker for all confirmation needs. We can do
   // better.
-  DownloadFilePicker::ShowFilePicker(download, suggested_path, callback);
+  if (is_file_picker_showing_) {
+    file_picker_callbacks_.emplace_back(
+        base::BindOnce(&ChromeDownloadManagerDelegate::ShowFilePicker,
+                       weak_ptr_factory_.GetWeakPtr(), download->GetGuid(),
+                       suggested_path, callback));
+  } else {
+    is_file_picker_showing_ = true;
+    ShowFilePicker(download->GetGuid(), suggested_path, callback);
+  }
 #endif  // !OS_ANDROID
 }
 
+void ChromeDownloadManagerDelegate::OnConfirmationCallbackComplete(
+    const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback,
+    DownloadConfirmationResult result,
+    const base::FilePath& virtual_path) {
+  callback.Run(result, virtual_path);
+  if (!file_picker_callbacks_.empty()) {
+    base::OnceClosure callback = std::move(file_picker_callbacks_.front());
+    std::move(callback).Run();
+    file_picker_callbacks_.pop_front();
+  } else {
+    is_file_picker_showing_ = false;
+  }
+}
+
+void ChromeDownloadManagerDelegate::ShowFilePicker(
+    const std::string& guid,
+    const base::FilePath& suggested_path,
+    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));
+  } else {
+    OnConfirmationCallbackComplete(
+        callback, DownloadConfirmationResult::CANCELED, base::FilePath());
+  }
+}
+
 #if defined(OS_ANDROID)
 void ChromeDownloadManagerDelegate::GenerateUniqueFileNameDone(
     gfx::NativeWindow native_window,

chrome/browser/download/chrome_download_manager_delegate.h

@@ -7,6 +7,7 @@
 
 #include <stdint.h>
 
+#include <deque>
 #include <memory>
 #include <string>
 #include <vector>
@@ -133,6 +134,12 @@ class ChromeDownloadManagerDelegate
   virtual safe_browsing::DownloadProtectionService*
       GetDownloadProtectionService();
 
+  // Called to show a file picker
+  virtual void ShowFilePicker(
+      const std::string& guid,
+      const base::FilePath& suggested_path,
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback);
+
   // DownloadTargetDeterminerDelegate. Protected for testing.
   void NotifyExtensions(download::DownloadItem* download,
                         const base::FilePath& suggested_virtual_path,
@@ -162,6 +169,12 @@ class ChromeDownloadManagerDelegate
       DownloadController::DownloadCancelReason reason);
 #endif
 
+  // Called when the file picker returns the confirmation result.
+  void OnConfirmationCallbackComplete(
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback,
+      DownloadConfirmationResult result,
+      const base::FilePath& virtual_path);
+
   // So that test classes that inherit from this for override purposes
   // can call back into the DownloadManager.
   content::DownloadManager* download_manager_;
@@ -257,6 +270,12 @@ class ChromeDownloadManagerDelegate
   CrxInstallerMap crx_installers_;
 #endif
 
+  // Outstanding callbacks to open file selection dialog.
+  std::deque<base::OnceClosure> file_picker_callbacks_;
+
+  // Whether a file picker dialog is showing.
+  bool is_file_picker_showing_;
+
   content::NotificationRegistrar registrar_;
 
   base::WeakPtrFactory<ChromeDownloadManagerDelegate> weak_ptr_factory_;

chrome/browser/download/download_file_picker.cc

@@ -63,15 +63,23 @@ DownloadFilePicker::DownloadFilePicker(DownloadItem* item,
   should_record_file_picker_result_ = !prefs->PromptForDownload();
 
   WebContents* web_contents = content::DownloadItemUtils::GetWebContents(item);
-  if (!web_contents || !web_contents->GetNativeView())
+  if (!web_contents || !web_contents->GetNativeView()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&DownloadFilePicker::FileSelectionCanceled,
+                                  base::Unretained(this), nullptr));
     return;
+  }
 
   select_file_dialog_ = ui::SelectFileDialog::Create(
       this, std::make_unique<ChromeSelectFilePolicy>(web_contents));
   // |select_file_dialog_| could be null in Linux. See CreateSelectFileDialog()
   // in shell_dialog_linux.cc.
-  if (!select_file_dialog_.get())
+  if (!select_file_dialog_.get()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&DownloadFilePicker::FileSelectionCanceled,
+                                  base::Unretained(this), nullptr));
     return;
+  }
 
   ui::SelectFileDialog::FileTypeInfo file_type_info;
   // Platform file pickers, notably on Mac and Windows, tend to break

chrome/browser/download/download_test_file_activity_observer.cc

@@ -52,17 +52,20 @@ class DownloadTestFileActivityObserver::MockDownloadManagerDelegate
   }
 
  protected:
-  void RequestConfirmation(download::DownloadItem* item,
-                           const base::FilePath& suggested_path,
-                           DownloadConfirmationReason reason,
-                           const ConfirmationCallback& callback) override {
+  void ShowFilePicker(
+      const std::string& guid,
+      const base::FilePath& suggested_path,
+      const DownloadTargetDeterminerDelegate::ConfirmationCallback& callback)
+      override {
     file_chooser_displayed_ = true;
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(callback,
-                                  (file_chooser_enabled_
-                                       ? DownloadConfirmationResult::CONFIRMED
-                                       : DownloadConfirmationResult::CANCELED),
-                                  suggested_path));
+        FROM_HERE,
+        base::BindOnce(
+            &MockDownloadManagerDelegate::OnConfirmationCallbackComplete,
+            base::Unretained(this), callback,
+            (file_chooser_enabled_ ? DownloadConfirmationResult::CONFIRMED
+                                   : DownloadConfirmationResult::CANCELED),
+            suggested_path));
   }
 
   void OpenDownload(download::DownloadItem* item) override {}