[omnibox] Reland: Use a counter instead of a bit to detect stale results

ZeroSuggestProvider::OnMostVisitedUrlsAvailable() can be called back
multiple times asynchronously from the respective ::Start() calls.
(TopSites::GetMostVisitedURLs() will happily queue up infinite
requests.) Currently there's only a single state variable protecting
against late replies. This change adds a counter to remember the
last valid request, and adds a test to challenge it.

It also cleans up the inconsistent scoping of the enumerations.

This reland initializes the counter in question, to pass MSAN,
even though it isn't strictly important which value it starts with.

Change-Id: Ie43feffd25c83319e04fc17b32d87e4037a6e7a3
Reviewed-on: https://chromium-review.googlesource.com/973185Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544882}

components/omnibox/browser/zero_suggest_provider.cc

@@ -133,7 +133,7 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
     return;
 
   Stop(true, false);
-  result_type_running_ = ResultType::NONE;
+  result_type_running_ = NONE;
   set_field_trial_triggered(false);
   set_field_trial_triggered_in_session(false);
   permanent_text_ = input.text();
@@ -148,7 +148,7 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
     return;
 
   result_type_running_ = TypeOfResultToRun(input.current_url(), suggest_url);
-  if (result_type_running_ == ZeroSuggestProvider::NONE)
+  if (result_type_running_ == NONE)
     return;
 
   done_ = false;
@@ -158,26 +158,25 @@ void ZeroSuggestProvider::Start(const AutocompleteInput& input,
   // suggestions, if based on local browsing history.
   MaybeUseCachedSuggestions();
 
-  if (result_type_running_ == ZeroSuggestProvider::MOST_VISITED) {
+  if (result_type_running_ == MOST_VISITED) {
     most_visited_urls_.clear();
     scoped_refptr<history::TopSites> ts = client()->GetTopSites();
     if (!ts) {
       done_ = true;
-      result_type_running_ = ResultType::NONE;
+      result_type_running_ = NONE;
       return;
     }
 
     ts->GetMostVisitedURLs(
         base::Bind(&ZeroSuggestProvider::OnMostVisitedUrlsAvailable,
-                   weak_ptr_factory_.GetWeakPtr()),
+                   weak_ptr_factory_.GetWeakPtr(), most_visited_request_num_),
         false);
     return;
   }
 
-  const std::string current_url =
-      result_type_running_ == ZeroSuggestProvider::DEFAULT_SERP_FOR_URL
-          ? current_query_
-          : std::string();
+  const std::string current_url = result_type_running_ == DEFAULT_SERP_FOR_URL
+                                      ? current_query_
+                                      : std::string();
   // Create a request for suggestions with |this| as the fetcher delegate.
   client()
       ->GetContextualSuggestionsService(/*create_if_necessary=*/true)
@@ -201,7 +200,11 @@ void ZeroSuggestProvider::Stop(bool clear_cached_results,
   if (contextual_suggestions_service != nullptr) {
     contextual_suggestions_service->StopCreatingContextualSuggestionsRequest();
   }
+  // TODO(krb): It would allow us to remove some guards if we could also cancel
+  // the TopSites::GetMostVisitedURLs request.
   done_ = true;
+  result_type_running_ = NONE;
+  ++most_visited_request_num_;
 
   if (clear_cached_results) {
     // We do not call Clear() on |results_| to retain |verbatim_relevance|
@@ -214,8 +217,6 @@ void ZeroSuggestProvider::Stop(bool clear_cached_results,
     current_title_.clear();
     most_visited_urls_.clear();
   }
-
-  result_type_running_ = ZeroSuggestProvider::NONE;
 }
 
 void ZeroSuggestProvider::DeleteMatch(const AutocompleteMatch& match) {
@@ -250,7 +251,7 @@ ZeroSuggestProvider::ZeroSuggestProvider(
     : BaseSearchProvider(AutocompleteProvider::TYPE_ZERO_SUGGEST, client),
       history_url_provider_(history_url_provider),
       listener_(listener),
-      result_type_running_(ResultType::NONE),
+      result_type_running_(NONE),
       weak_ptr_factory_(this) {
   // Record whether contextual zero suggest is possible for this user / profile.
   const TemplateURLService* template_url_service =
@@ -320,7 +321,8 @@ void ZeroSuggestProvider::OnURLFetchComplete(const net::URLFetcher* source) {
       UpdateResults(SearchSuggestionParser::ExtractJsonData(source));
   fetcher_.reset();
   done_ = true;
-  result_type_running_ = ZeroSuggestProvider::NONE;
+  result_type_running_ = NONE;
+  ++most_visited_request_num_;
   listener_->OnProviderUpdate(results_updated);
 }
 
@@ -332,7 +334,7 @@ bool ZeroSuggestProvider::UpdateResults(const std::string& json_data) {
 
   // When running the personalized service, we want to store suggestion
   // responses if non-empty.
-  if (result_type_running_ == ResultType::DEFAULT_SERP && !json_data.empty()) {
+  if (result_type_running_ == DEFAULT_SERP && !json_data.empty()) {
     client()->GetPrefs()->SetString(omnibox::kZeroSuggestCachedResults,
                                     json_data);
 
@@ -391,13 +393,17 @@ AutocompleteMatch ZeroSuggestProvider::NavigationToMatch(
 }
 
 void ZeroSuggestProvider::OnMostVisitedUrlsAvailable(
+    size_t orig_request_num,
     const history::MostVisitedURLList& urls) {
-  if (result_type_running_ != ResultType::MOST_VISITED)
+  if (result_type_running_ != MOST_VISITED ||
+      orig_request_num != most_visited_request_num_) {
     return;
+  }
   most_visited_urls_ = urls;
   done_ = true;
   ConvertResultsToAutocompleteMatches();
-  result_type_running_ = ResultType::NONE;
+  result_type_running_ = NONE;
+  ++most_visited_request_num_;
   listener_->OnProviderUpdate(true);
 }
 
@@ -432,7 +438,7 @@ void ZeroSuggestProvider::ConvertResultsToAutocompleteMatches() {
   UMA_HISTOGRAM_COUNTS("ZeroSuggest.AllResults", num_results);
 
   // Show Most Visited results after ZeroSuggest response is received.
-  if (result_type_running_ == ResultType::MOST_VISITED) {
+  if (result_type_running_ == MOST_VISITED) {
     if (!current_url_match_.destination_url.is_valid())
       return;
     matches_.push_back(current_url_match_);
@@ -515,7 +521,7 @@ bool ZeroSuggestProvider::AllowZeroSuggestSuggestions(
 }
 
 void ZeroSuggestProvider::MaybeUseCachedSuggestions() {
-  if (result_type_running_ != ZeroSuggestProvider::DEFAULT_SERP)
+  if (result_type_running_ != DEFAULT_SERP)
     return;
 
   std::string json_data =
@@ -564,24 +570,23 @@ ZeroSuggestProvider::ResultType ZeroSuggestProvider::TypeOfResultToRun(
 
   // Check if zero suggestions are allowed in the current context.
   if (!AllowZeroSuggestSuggestions(current_url))
-    return ResultType::NONE;
+    return NONE;
 
   if (OmniboxFieldTrial::InZeroSuggestPersonalizedFieldTrial())
     return PersonalizedServiceShouldFallBackToMostVisited(
                client()->GetPrefs(), client()->IsAuthenticated(),
                template_url_service)
-               ? ResultType::MOST_VISITED
-               : ResultType::DEFAULT_SERP;
+               ? MOST_VISITED
+               : DEFAULT_SERP;
 
   if (OmniboxFieldTrial::InZeroSuggestMostVisitedWithoutSerpFieldTrial() &&
       client()
           ->GetTemplateURLService()
           ->IsSearchResultsPageFromDefaultSearchProvider(current_url))
-    return ResultType::NONE;
+    return NONE;
 
   if (OmniboxFieldTrial::InZeroSuggestMostVisitedFieldTrial())
-    return ResultType::MOST_VISITED;
+    return MOST_VISITED;
 
-  return can_send_current_url ? ResultType::DEFAULT_SERP_FOR_URL
-                              : ResultType::NONE;
+  return can_send_current_url ? DEFAULT_SERP_FOR_URL : NONE;
 }

components/omnibox/browser/zero_suggest_provider.h

@@ -131,7 +131,8 @@ class ZeroSuggestProvider : public BaseSearchProvider,
   // When the user is in the Most Visited field trial, we ask the TopSites
   // service for the most visited URLs. It then calls back to this function to
   // return those |urls|.
-  void OnMostVisitedUrlsAvailable(const history::MostVisitedURLList& urls);
+  void OnMostVisitedUrlsAvailable(size_t request_num,
+                                  const history::MostVisitedURLList& urls);
 
   // When the user is in the contextual omnibox suggestions field trial, we ask
   // the ContextualSuggestionsService for a fetcher to retrieve recommendations.
@@ -163,6 +164,9 @@ class ZeroSuggestProvider : public BaseSearchProvider,
   // When the provider is not running, the result type is set to NONE.
   ResultType result_type_running_;
 
+  // For reconciling asynchronous requests for most visited URLs.
+  size_t most_visited_request_num_ = 0;
+
   // The URL for which a suggestion fetch is pending.
   std::string current_query_;
 

components/omnibox/browser/zero_suggest_provider_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "components/omnibox/browser/zero_suggest_provider.h"
 
+#include <list>
 #include <map>
 #include <memory>
 #include <string>
@@ -86,11 +87,19 @@ class FakeEmptyTopSites : public history::TopSites {
   // RefcountedKeyedService:
   void ShutdownOnUIThread() override {}
 
+  // Only runs a single callback, so that the test can specify a different
+  // set per call.
+  void RunACallback(const history::MostVisitedURLList& urls) {
+    DCHECK(!callbacks.empty());
+    callbacks.front().Run(urls);
+    callbacks.pop_front();
+  }
+
+ protected:
   // A test-specific field for controlling when most visited callback is run
   // after top sites have been requested.
-  GetMostVisitedURLsCallback mv_callback;
+  std::list<GetMostVisitedURLsCallback> callbacks;
 
- protected:
   ~FakeEmptyTopSites() override {}
 
   DISALLOW_COPY_AND_ASSIGN(FakeEmptyTopSites);
@@ -99,7 +108,7 @@ class FakeEmptyTopSites : public history::TopSites {
 void FakeEmptyTopSites::GetMostVisitedURLs(
     const GetMostVisitedURLsCallback& callback,
     bool include_forced_urls)  {
-  mv_callback = callback;
+  callbacks.push_back(callback);
 }
 
 class FakeAutocompleteProviderClient : public MockAutocompleteProviderClient {
@@ -281,7 +290,7 @@ TEST_F(ZeroSuggestProviderTest, TestMostVisitedCallback) {
   provider_->Start(input, false);
   EXPECT_TRUE(provider_->matches().empty());
   scoped_refptr<history::TopSites> top_sites = client_->GetTopSites();
-  static_cast<FakeEmptyTopSites*>(top_sites.get())->mv_callback.Run(urls);
+  static_cast<FakeEmptyTopSites*>(top_sites.get())->RunACallback(urls);
   // Should have verbatim match + most visited url match.
   EXPECT_EQ(2U, provider_->matches().size());
   provider_->Stop(false, false);
@@ -291,8 +300,23 @@ TEST_F(ZeroSuggestProviderTest, TestMostVisitedCallback) {
   EXPECT_TRUE(provider_->matches().empty());
   // Most visited results arriving after Stop() has been called, ensure they
   // are not displayed.
-  static_cast<FakeEmptyTopSites*>(top_sites.get())->mv_callback.Run(urls);
+  static_cast<FakeEmptyTopSites*>(top_sites.get())->RunACallback(urls);
+  EXPECT_TRUE(provider_->matches().empty());
+
+  history::MostVisitedURLList urls2;
+  urls2.push_back(history::MostVisitedURL(GURL("http://bar.com/"),
+                                          base::ASCIIToUTF16("Bar")));
+  urls2.push_back(history::MostVisitedURL(GURL("http://zinga.com/"),
+                                          base::ASCIIToUTF16("Zinga")));
+  provider_->Start(input, false);
+  provider_->Stop(false, false);
+  provider_->Start(input, false);
+  static_cast<FakeEmptyTopSites*>(top_sites.get())->RunACallback(urls);
+  // Stale results should get rejected.
   EXPECT_TRUE(provider_->matches().empty());
+  static_cast<FakeEmptyTopSites*>(top_sites.get())->RunACallback(urls2);
+  EXPECT_FALSE(provider_->matches().empty());
+  provider_->Stop(false, false);
 }
 
 TEST_F(ZeroSuggestProviderTest, TestMostVisitedNavigateToSearchPage) {
@@ -326,7 +350,7 @@ TEST_F(ZeroSuggestProviderTest, TestMostVisitedNavigateToSearchPage) {
   EXPECT_TRUE(provider_->matches().empty());
   // Most visited results arriving after a new request has been started.
   scoped_refptr<history::TopSites> top_sites = client_->GetTopSites();
-  static_cast<FakeEmptyTopSites*>(top_sites.get())->mv_callback.Run(urls);
+  static_cast<FakeEmptyTopSites*>(top_sites.get())->RunACallback(urls);
   EXPECT_TRUE(provider_->matches().empty());
 }