[Suggested files] Add a second weak pointer factory

The Drive QuickAccess provider makes API calls to the backend, and each
API call should first cancel any in-flight calls. Currently we do that
by binding the call to a weak pointer, and invalidating all weak
pointers before making the next API call.

But this causes a bug: other parts of the provider rely on the same
weak pointer factory for unrelated callbacks, which then get erroneously
cancelled.

This CL adds a second weak pointer factory, and splits the work between
the two. This lets the API calls be cancelled independent of all other
calls. Note I can only find one place in the codebase using two
factories [1], but I can't think of an better fix here.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.h?l=1209

Bug: 1034842
Change-Id: I034c0fdaff580801bb147cbd1cba24d499bf0002
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2246002Reviewed-by: Rachel Wong <wrong@chromium.org>
Commit-Queue: Tony Yeoman <tby@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779186}

chrome/browser/ui/app_list/search/drive_quick_access_provider.cc

@@ -133,7 +133,7 @@ void DriveQuickAccessProvider::OnFileSystemMounted() {
   // sleep.
   GetQuickAccessItems(
       base::BindOnce(&DriveQuickAccessProvider::StartSearchController,
-                     weak_factory_.GetWeakPtr()));
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void DriveQuickAccessProvider::StartSearchController() {
@@ -187,7 +187,7 @@ void DriveQuickAccessProvider::Start(const base::string16& query) {
           &file_manager::file_tasks::FileTasksNotifier::QueryFileAvailability,
           base::Unretained(file_tasks_notifier_), result_paths,
           base::BindOnce(&DriveQuickAccessProvider::PublishResults,
-                         weak_factory_.GetWeakPtr(), result_paths)));
+                         weak_ptr_factory_.GetWeakPtr(), result_paths)));
 }
 
 void DriveQuickAccessProvider::PublishResults(
@@ -225,6 +225,9 @@ void DriveQuickAccessProvider::PublishResults(
 }
 
 void DriveQuickAccessProvider::AppListShown() {
+  // TODO(crbug.com/1034842): This call is triggered every time the app list is
+  // shown, and sends a query to the backend. We should consider adding
+  // rate-limiting here.
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   GetQuickAccessItems(base::DoNothing());
 }
@@ -236,13 +239,14 @@ void DriveQuickAccessProvider::GetQuickAccessItems(
     return;
 
   // Invalidate weak pointers for existing callbacks to the Quick Access API.
-  weak_factory_.InvalidateWeakPtrs();
+  quick_access_weak_ptr_factory_.InvalidateWeakPtrs();
   latest_fetch_start_time_ = base::TimeTicks::Now();
 
   drive_service_->GetQuickAccessItems(
       kMaxItems,
       base::BindOnce(&DriveQuickAccessProvider::OnGetQuickAccessItems,
-                     weak_factory_.GetWeakPtr(), std::move(on_done)));
+                     quick_access_weak_ptr_factory_.GetWeakPtr(),
+                     std::move(on_done)));
 }
 
 void DriveQuickAccessProvider::OnGetQuickAccessItems(
@@ -276,7 +280,7 @@ void DriveQuickAccessProvider::OnGetQuickAccessItems(
         task_runner_.get(), FROM_HERE,
         base::BindOnce(&FilterResults, drive_service_, drive_results),
         base::BindOnce(&DriveQuickAccessProvider::SetResultsCache,
-                       weak_factory_.GetWeakPtr(), std::move(on_done)));
+                       weak_ptr_factory_.GetWeakPtr(), std::move(on_done)));
   }
 }
 

chrome/browser/ui/app_list/search/drive_quick_access_provider.h

@@ -72,7 +72,13 @@ class DriveQuickAccessProvider : public SearchProvider,
   SEQUENCE_CHECKER(sequence_checker_);
 
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
-  base::WeakPtrFactory<DriveQuickAccessProvider> weak_factory_{this};
+  // Factory for general use.
+  base::WeakPtrFactory<DriveQuickAccessProvider> weak_ptr_factory_{this};
+  // Factory only for weak pointers for Drive QuickAccess API calls. Using two
+  // factories allows in-flight API calls to be cancelled independently of other
+  // tasks by invalidating only this factory's weak pointers.
+  base::WeakPtrFactory<DriveQuickAccessProvider> quick_access_weak_ptr_factory_{
+      this};
 
   DISALLOW_COPY_AND_ASSIGN(DriveQuickAccessProvider);
 };