Fix flaky HoldingSpaceKeyedServiceTest.RetrieveHistory

Flakiness was caused by race conditions in checking for existence of
download files. Now, order is maintained across existence checks.

Bug: 1130904
Change-Id: I6f4d131ef1c39ef624b87e67f1f61a3704fc634f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2444441Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Auto-Submit: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#813341}

chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.cc

@@ -4,9 +4,10 @@
 
 #include "chrome/browser/ui/ash/holding_space/holding_space_downloads_delegate.h"
 
+#include <vector>
+
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
-#include "base/barrier_closure.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 #include "content/public/browser/browser_context.h"
@@ -89,41 +90,37 @@ void HoldingSpaceDownloadsDelegate::OnManagerInitialized() {
   download::SimpleDownloadManager::DownloadVector downloads;
   download_manager->GetAllDownloads(&downloads);
 
-  base::RepeatingClosure barrier_closure = base::BarrierClosure(
-      downloads.size(), std::move(downloads_restored_callback_));
+  std::vector<base::FilePath> file_paths;
 
   for (auto* download : downloads) {
     switch (download->GetState()) {
-      case download::DownloadItem::COMPLETE: {
-        if (IsRecentEnough(profile(), download)) {
-          holding_space_util::FilePathValid(
-              profile(), {download->GetFullPath(), /*requirements=*/{}},
-              base::BindOnce(
-                  [](const base::FilePath& path,
-                     base::RepeatingClosure barrier_closure,
-                     ItemDownloadedCallback callback, bool valid) {
-                    if (valid)
-                      callback.Run(path);
-                    barrier_closure.Run();
-                  },
-                  download->GetFullPath(), barrier_closure,
-                  base::BindRepeating(
-                      &HoldingSpaceDownloadsDelegate::OnDownloadCompleted,
-                      weak_factory_.GetWeakPtr())));
-        } else {
-          barrier_closure.Run();
-        }
-      } break;
+      case download::DownloadItem::COMPLETE:
+        if (IsRecentEnough(profile(), download))
+          file_paths.push_back(download->GetFullPath());
+        break;
       case download::DownloadItem::IN_PROGRESS:
         download_item_observer_.Add(download);
-        FALLTHROUGH;
+        break;
       case download::DownloadItem::CANCELLED:
       case download::DownloadItem::INTERRUPTED:
       case download::DownloadItem::MAX_DOWNLOAD_STATE:
-        barrier_closure.Run();
         break;
     }
   }
+
+  holding_space_util::PartitionFilePathsByExistence(
+      profile(), file_paths,
+      base::BindOnce(
+          [](const base::WeakPtr<HoldingSpaceDownloadsDelegate>& weak_ptr,
+             std::vector<base::FilePath> existing_file_paths,
+             std::vector<base::FilePath> non_existing_file_paths) {
+            if (weak_ptr) {
+              for (const auto& existing_file_path : existing_file_paths)
+                weak_ptr->OnDownloadCompleted(existing_file_path);
+              std::move(weak_ptr->downloads_restored_callback_).Run();
+            }
+          },
+          weak_factory_.GetWeakPtr()));
 }
 
 void HoldingSpaceDownloadsDelegate::ManagerGoingDown(

chrome/browser/ui/ash/holding_space/holding_space_util.cc

@@ -50,8 +50,7 @@ gfx::ImageSkia GetPlaceholderImage(HoldingSpaceItem::Type type,
 }  // namespace
 
 ValidityRequirement::ValidityRequirement() = default;
-ValidityRequirement::ValidityRequirement(const ValidityRequirement& other) =
-    default;
+ValidityRequirement::ValidityRequirement(const ValidityRequirement&) = default;
 ValidityRequirement::ValidityRequirement(ValidityRequirement&& other) = default;
 
 // Utilities -------------------------------------------------------------------
@@ -80,11 +79,22 @@ void FilePathValid(Profile* profile,
           std::move(callback), file_path_with_requirement.second));
 }
 
+void PartitionFilePathsByExistence(
+    Profile* profile,
+    FilePathList file_paths,
+    PartitionFilePathsByExistenceCallback callback) {
+  FilePathsWithValidityRequirements file_paths_with_requirements;
+  for (const auto& file_path : file_paths)
+    file_paths_with_requirements.push_back({file_path, /*requirements=*/{}});
+  PartitionFilePathsByValidity(profile, file_paths_with_requirements,
+                               std::move(callback));
+}
+
 void PartitionFilePathsByValidity(
     Profile* profile,
-    FilePathsWithValidityRequirements file_paths_with_requirement,
+    FilePathsWithValidityRequirements file_paths_with_requirements,
     PartitionFilePathsByValidityCallback callback) {
-  if (file_paths_with_requirement.empty()) {
+  if (file_paths_with_requirements.empty()) {
     std::move(callback).Run(/*valid_file_paths=*/{},
                             /*invalid_file_paths=*/{});
     return;
@@ -97,7 +107,7 @@ void PartitionFilePathsByValidity(
   auto* invalid_file_paths_ptr = invalid_file_paths.get();
 
   FilePathList file_paths;
-  for (const auto& file_path_with_requirement : file_paths_with_requirement)
+  for (const auto& file_path_with_requirement : file_paths_with_requirements)
     file_paths.push_back(file_path_with_requirement.first);
 
   // This `barrier_closure` will be run after verifying the existence of all
@@ -105,7 +115,7 @@ void PartitionFilePathsByValidity(
   // `invalid_file_paths` will have been populated by the time of
   // invocation.
   base::RepeatingClosure barrier_closure = base::BarrierClosure(
-      file_paths_with_requirement.size(),
+      file_paths_with_requirements.size(),
       base::BindOnce(
           [](FilePathList sorted_file_paths,
              std::unique_ptr<FilePathList> valid_file_paths,
@@ -136,7 +146,7 @@ void PartitionFilePathsByValidity(
   // Verify existence of each `file_path`. Upon successful check of existence,
   // each `file_path` should be pushed into either `valid_file_paths` or
   // `invalid_file_paths` as appropriate.
-  for (const auto& file_path_with_requirement : file_paths_with_requirement) {
+  for (const auto& file_path_with_requirement : file_paths_with_requirements) {
     FilePathValid(
         profile, file_path_with_requirement,
         base::BindOnce(

chrome/browser/ui/ash/holding_space/holding_space_util.h

@@ -47,6 +47,15 @@ void FilePathValid(Profile*,
                    FilePathWithValidityRequirement,
                    FilePathValidCallback);
 
+// Partitions `file_paths` into `existing_file_paths` and
+// `non_existing_file_paths`, returning the result via `callback`.
+using PartitionFilePathsByExistenceCallback =
+    base::OnceCallback<void(FilePathList existing_file_paths,
+                            FilePathList invalid_file_paths)>;
+void PartitionFilePathsByExistence(Profile*,
+                                   FilePathList,
+                                   PartitionFilePathsByExistenceCallback);
+
 // Partitions `file_paths` into `valid_file_paths` and
 // `invalid_file_paths`, returning the result via `callback`.
 using PartitionFilePathsByValidityCallback =