[Remote suggestions] Retry a fetch when suggestions cleared meanwhile

This CL introduces a retry mechanism in RemoteSuggestionsProvider when
suggestions are cleared while a request is in flight.

This change does not violate the previous contract that all fetches are
triggered by the scheduler. From the point of view of the scheduler,
the original fetch and the retry seem as one single fetch.

Bug: 802179
Change-Id: I6138e16cc68d2a1f4b93576002ce241829255c18
Reviewed-on: https://chromium-review.googlesource.com/873034
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532913}

components/ntp_snippets/remote/remote_suggestions_provider_impl.cc

@@ -390,7 +390,8 @@ RemoteSuggestionsProviderImpl::RemoteSuggestionsProviderImpl(
       breaking_news_raw_data_provider_(
           std::move(breaking_news_raw_data_provider)),
       debug_logger_(debug_logger),
-      fetch_timeout_timer_(std::move(fetch_timeout_timer)) {
+      fetch_timeout_timer_(std::move(fetch_timeout_timer)),
+      request_status_(FetchRequestStatus::NONE) {
   DCHECK(debug_logger_);
   DCHECK(fetch_timeout_timer_);
   RestoreCategoriesFromPrefs();
@@ -572,6 +573,14 @@ void RemoteSuggestionsProviderImpl::FetchSuggestions(
     }
     return;
   }
+  if (request_status_ == FetchRequestStatus::NONE) {
+    // We cannot rule out concurrent requests although they are rare as the user
+    // has to trigger ReloadSuggestions() while the scheduler decides for a
+    // background fetch. Although preventing concurrent fetches would be
+    // desireable, it's not worth the effort (also see TODO() in
+    // OnFetchFinished()).
+    request_status_ = FetchRequestStatus::IN_PROGRESS;
+  }
 
   // |count_to_fetch| is actually ignored, because the server does not support
   // this functionality.
@@ -724,6 +733,19 @@ void RemoteSuggestionsProviderImpl::ClearHistory(
   // because it is not known which history entries were used for the suggestions
   // personalization.
   ClearHistoryDependentState();
+  if (request_status_ == FetchRequestStatus::IN_PROGRESS ||
+      request_status_ == FetchRequestStatus::IN_PROGRESS_NEEDS_REFETCH) {
+    request_status_ = FetchRequestStatus::IN_PROGRESS_CANCELED;
+  }
+}
+
+void RemoteSuggestionsProviderImpl::ClearCachedSuggestions() {
+  ClearCachedSuggestionsImpl();
+  if (request_status_ == FetchRequestStatus::IN_PROGRESS) {
+    // Called by external cache-cleared trigger. As this can be caused by
+    // language change, we need to refetch a potentiall ongoing fetch.
+    request_status_ = FetchRequestStatus::IN_PROGRESS_NEEDS_REFETCH;
+  }
 }
 
 void RemoteSuggestionsProviderImpl::OnSignInStateChanged(bool has_signed_in) {
@@ -897,12 +919,31 @@ void RemoteSuggestionsProviderImpl::OnFetchFinished(
     RemoteSuggestionsFetcher::OptionalFetchedCategories fetched_categories) {
   debug_logger_->Log(FROM_HERE, /*message=*/std::string());
 
+  FetchRequestStatus request_status = request_status_;
+  // TODO(jkrcal): This is potentially incorrect if there is another concurrent
+  // request in progress; when it finishes we will treat it as a standard
+  // request even though it may need to be refetched/disregarded. Even though
+  // the scheduler never triggers two concurrent requests, the user can trigger
+  // the second request via the UI. If cache/history gets cleared before neither
+  // of the two finishes, we can get outdated results afterwards. Low chance &
+  // low risk, feels safe to ignore.
+  request_status_ = FetchRequestStatus::NONE;
+
   if (!ready()) {
     // TODO(tschumann): What happens if this was a user-triggered, interactive
     // request? Is the UI waiting indefinitely now?
     return;
   }
 
+  if (request_status == FetchRequestStatus::IN_PROGRESS_NEEDS_REFETCH) {
+    // Disregard the results and start a fetch again.
+    FetchSuggestions(interactive_request, std::move(callback));
+    return;
+  } else if (request_status == FetchRequestStatus::IN_PROGRESS_CANCELED) {
+    // Disregard the results.
+    return;
+  }
+
   if (!status.IsSuccess()) {
     if (callback) {
       std::move(callback).Run(status);
@@ -1333,16 +1374,18 @@ void RemoteSuggestionsProviderImpl::ClearHistoryDependentState() {
     return;
   }
 
+  debug_logger_->Log(FROM_HERE, /*message=*/std::string());
   NukeAllSuggestions();
   remote_suggestions_scheduler_->OnHistoryCleared();
 }
 
-void RemoteSuggestionsProviderImpl::ClearCachedSuggestions() {
+void RemoteSuggestionsProviderImpl::ClearCachedSuggestionsImpl() {
   if (!initialized()) {
     clear_cached_suggestions_when_initialized_ = true;
     return;
   }
 
+  debug_logger_->Log(FROM_HERE, /*message=*/std::string());
   NukeAllSuggestions();
   remote_suggestions_scheduler_->OnSuggestionsCleared();
 }
@@ -1469,7 +1512,10 @@ void RemoteSuggestionsProviderImpl::OnStatusChanged(
         DCHECK(state_ == State::READY);
         // Clear nonpersonalized suggestions (and notify the scheduler there are
         // no suggestions).
-        ClearCachedSuggestions();
+        ClearCachedSuggestionsImpl();
+        if (request_status_ == FetchRequestStatus::IN_PROGRESS) {
+          request_status_ = FetchRequestStatus::IN_PROGRESS_NEEDS_REFETCH;
+        }
       } else {
         EnterState(State::READY);
       }
@@ -1480,7 +1526,10 @@ void RemoteSuggestionsProviderImpl::OnStatusChanged(
         DCHECK(state_ == State::READY);
         // Clear personalized suggestions (and notify the scheduler there are
         // no suggestions).
-        ClearCachedSuggestions();
+        ClearCachedSuggestionsImpl();
+        if (request_status_ == FetchRequestStatus::IN_PROGRESS) {
+          request_status_ = FetchRequestStatus::IN_PROGRESS_NEEDS_REFETCH;
+        }
       } else {
         EnterState(State::READY);
       }
@@ -1521,7 +1570,7 @@ void RemoteSuggestionsProviderImpl::EnterState(State state) {
       UpdateAllCategoryStatus(CategoryStatus::AVAILABLE);
 
       if (clear_cached_suggestions_when_initialized_) {
-        ClearCachedSuggestions();
+        ClearCachedSuggestionsImpl();
         clear_cached_suggestions_when_initialized_ = false;
       }
       if (clear_history_dependent_state_when_initialized_) {
@@ -1547,7 +1596,7 @@ void RemoteSuggestionsProviderImpl::EnterState(State state) {
         clear_history_dependent_state_when_initialized_ = false;
         ClearHistoryDependentState();
       }
-      ClearCachedSuggestions();
+      ClearCachedSuggestionsImpl();
       clear_cached_suggestions_when_initialized_ = false;
 
       if (breaking_news_raw_data_provider_ &&

components/ntp_snippets/remote/remote_suggestions_provider_impl.h

@@ -161,6 +161,8 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
                            CallsSchedulerWhenSignedIn);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
                            CallsSchedulerWhenSignedOut);
+  FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderImplTest,
+                           RestartsFetchWhenSignedInWhileFetching);
   FRIEND_TEST_ALL_PREFIXES(
       RemoteSuggestionsProviderImplTest,
       ShouldNotSetExclusiveCategoryWhenFetchingSuggestions);
@@ -206,6 +208,25 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
     COUNT
   };
 
+  // Documents the status of the ongoing request and what action should be taken
+  // on completion.
+  enum class FetchRequestStatus {
+    // There is no request in progress for remote suggestions.
+    NONE,
+
+    // There is a valid request in progress that should be treated normally on
+    // completion.
+    IN_PROGRESS,
+
+    // There is a canceled request in progress. The response should be ignored
+    // when it arrives.
+    IN_PROGRESS_CANCELED,
+
+    // There is an invalidated request in progress. On completion, we should
+    // ignore the response and initiate a new fetch (with updated parameters).
+    IN_PROGRESS_NEEDS_REFETCH
+  };
+
   struct CategoryContent {
     // The current status of the category.
     CategoryStatus status = CategoryStatus::INITIALIZING;
@@ -333,6 +354,9 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   // Clears suggestions because any history item has been removed.
   void ClearHistoryDependentState();
 
+  // Clears the cached suggestions
+  void ClearCachedSuggestionsImpl();
+
   // Clears all stored suggestions and updates the observer.
   void NukeAllSuggestions();
 
@@ -448,6 +472,12 @@ class RemoteSuggestionsProviderImpl final : public RemoteSuggestionsProvider {
   // A Timer for canceling too long fetches.
   std::unique_ptr<base::OneShotTimer> fetch_timeout_timer_;
 
+  // Keeps track of the status of the ongoing request(s) and what action should
+  // be taken on completion. Requests via Fetch() (fetching more) are _not_
+  // tracked by this variable (as they do not need any special actions on
+  // completion).
+  FetchRequestStatus request_status_;
+
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProviderImpl);
 };
 

components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc

@@ -568,6 +568,19 @@ class RemoteSuggestionsProviderImplTest : public ::testing::Test {
     std::move(snippets_callback).Run(status, std::move(fetched_categories));
   }
 
+  RemoteSuggestionsFetcher::SnippetsAvailableCallback
+  FetchSuggestionsAndGetResponseCallback(
+      RemoteSuggestionsProviderImpl* provider,
+      bool interactive_request) {
+    RemoteSuggestionsFetcher::SnippetsAvailableCallback snippets_callback;
+    EXPECT_CALL(*mock_suggestions_fetcher(), FetchSnippets(_, _))
+        .WillOnce(MoveSecondArgumentPointeeTo(&snippets_callback))
+        .RetiresOnSaturation();
+    provider->FetchSuggestions(
+        interactive_request, RemoteSuggestionsProvider::FetchStatusCallback());
+    return snippets_callback;
+  }
+
   RemoteSuggestionsFetcher::SnippetsAvailableCallback
   RefetchWhileDisplayingAndGetResponseCallback(
       RemoteSuggestionsProviderImpl* provider) {
@@ -2452,6 +2465,65 @@ TEST_F(RemoteSuggestionsProviderImplTest, CallsSchedulerWhenSignedOut) {
                             RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
 }
 
+TEST_F(RemoteSuggestionsProviderImplTest,
+       RestartsFetchWhenSignedInWhileFetching) {
+  auto provider =
+      MakeSuggestionsProviderWithoutInitializationWithStrictScheduler();
+  // Initiate the provider so that it is already READY.
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
+  WaitForSuggestionsProviderInitialization(provider.get());
+
+  // Initiate the fetch.
+  RemoteSuggestionsFetcher::SnippetsAvailableCallback snippets_callback;
+  EXPECT_CALL(*mock_suggestions_fetcher(), FetchSnippets(_, _))
+      .WillOnce(MoveSecondArgumentPointeeTo(&snippets_callback))
+      .RetiresOnSaturation();
+  provider->FetchSuggestions(/*interactive_request=*/false,
+                             RemoteSuggestionsProvider::FetchStatusCallback());
+
+  // The scheduler should be notified of clearing the suggestions.
+  EXPECT_CALL(*scheduler(), OnSuggestionsCleared());
+  provider->OnStatusChanged(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_OUT,
+                            RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN);
+
+  // Once we signal the first fetch to be finished (calling snippets_callback
+  // below), a new fetch should get triggered.
+  EXPECT_CALL(*mock_suggestions_fetcher(), FetchSnippets(_, _)).Times(1);
+  std::move(snippets_callback)
+      .Run(Status::Success(), std::vector<FetchedCategory>());
+}
+
+TEST_F(RemoteSuggestionsProviderImplTest,
+       IgnoresResultsWhenHistoryClearedWhileFetching) {
+  auto provider =
+      MakeSuggestionsProviderWithoutInitializationWithStrictScheduler();
+  // Initiate the provider so that it is already READY.
+  EXPECT_CALL(*scheduler(), OnProviderActivated());
+  WaitForSuggestionsProviderInitialization(provider.get());
+
+  // Initiate the fetch.
+  RemoteSuggestionsFetcher::SnippetsAvailableCallback snippets_callback =
+      FetchSuggestionsAndGetResponseCallback(provider.get(),
+                                             /*interactive_request=*/false);
+
+  // The scheduler should be notified of clearing the history.
+  EXPECT_CALL(*scheduler(), OnHistoryCleared());
+  provider->ClearHistory(GetDefaultCreationTime(), GetDefaultExpirationTime(),
+                         base::RepeatingCallback<bool(const GURL& url)>());
+
+  // Once the fetch finishes, the returned suggestions are ignored.
+  FetchedCategoryBuilder category_builder;
+  category_builder.SetCategory(articles_category());
+  category_builder.AddSuggestionViaBuilder(
+      RemoteSuggestionBuilder().AddId(base::StringPrintf("http://abc.com")));
+  std::vector<FetchedCategory> fetched_categories;
+  fetched_categories.push_back(category_builder.Build());
+  std::move(snippets_callback)
+      .Run(Status::Success(), std::move(fetched_categories));
+  EXPECT_THAT(observer().SuggestionsForCategory(articles_category()),
+              SizeIs(0));
+}
+
 TEST_F(RemoteSuggestionsProviderImplTest,
        ShouldExcludeKnownSuggestionsWithoutTruncatingWhenFetchingMore) {
   auto provider = MakeSuggestionsProvider(