[omnibox] Fix logging of async match changes in AutocompleteController

The previous way we counted async match changes in
AutocompleteController was wrong, because I misunderstood how
AutocompleteResult::CopyOldMatches works.

Previously, CopyOldMatches would actually *move* the matches from the
old result to the new result. This operation cleared out
|last_result|. Since we determine if a match position has changed by
comparing |last_result| to the new |result_|, this overinflated the
number of changed matches.

I've verified this by adding detailed logging on my test machine.

This CL makes CopyOldMatches take a const parameter, and copy over the
old matches without mutating |last_result|. I think there's no way for
us to do this type of logging while still doing the destructive-move
from |last_result|, and I suspect doing a copy instead of a move will
have a negligible performance impact, and no memory impact.

Bug: 398135
Change-Id: I0e1c5f58c6264b5be4c3d12366ae6e3c66f2a04f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899314Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714011}

components/omnibox/browser/autocomplete_controller.cc

@@ -566,11 +566,13 @@ void AutocompleteController::UpdateResult(
     }
   }
 
+  const auto last_result_for_logging = result_.GetMatchDedupComparators();
+
   if (regenerate_result)
     result_.Reset();
 
-  AutocompleteResult last_result;
-  last_result.Swap(&result_);
+  AutocompleteResult old_matches_to_reuse;
+  old_matches_to_reuse.Swap(&result_);
 
   for (Providers::const_iterator i(providers_.begin());
        i != providers_.end(); ++i)
@@ -584,7 +586,7 @@ void AutocompleteController::UpdateResult(
 
   // Sort the matches and trim to a small number of "best" matches.
   const AutocompleteMatch* preserve_default_match = nullptr;
-  if (!in_start_ && !regenerate_result && last_default_match &&
+  if (!in_start_ && last_default_match &&
       base::FeatureList::IsEnabled(
           omnibox::kOmniboxPreserveDefaultMatchAgainstAsyncUpdate)) {
     preserve_default_match = &last_default_match.value();
@@ -600,12 +602,15 @@ void AutocompleteController::UpdateResult(
   if (!done_) {
     // This conditional needs to match the conditional in Start that invokes
     // StartExpireTimer.
-    result_.CopyOldMatches(input_, &last_result, template_url_service_);
+    result_.TransferOldMatches(input_, &old_matches_to_reuse,
+                               template_url_service_);
   }
 
   // Log metrics for how many matches are asynchronously changed.
-  if (!in_start_)
-    AutocompleteResult::LogAsynchronousUpdateMetrics(last_result, result_);
+  if (!in_start_) {
+    AutocompleteResult::LogAsynchronousUpdateMetrics(last_result_for_logging,
+                                                     result_);
+  }
 
   UpdateKeywordDescriptions(&result_);
   UpdateAssociatedKeywords(&result_);

components/omnibox/browser/autocomplete_result.cc

@@ -107,7 +107,7 @@ AutocompleteResult::AutocompleteResult() {
 
 AutocompleteResult::~AutocompleteResult() {}
 
-void AutocompleteResult::CopyOldMatches(
+void AutocompleteResult::TransferOldMatches(
     const AutocompleteInput& input,
     AutocompleteResult* old_matches,
     TemplateURLService* template_url_service) {
@@ -755,17 +755,24 @@ size_t AutocompleteResult::EstimateMemoryUsage() const {
   return res;
 }
 
+std::vector<AutocompleteResult::MatchDedupComparator>
+AutocompleteResult::GetMatchDedupComparators() const {
+  std::vector<MatchDedupComparator> comparators;
+  for (const auto& match : *this)
+    comparators.push_back(AutocompleteResult::GetMatchComparisonFields(match));
+  return comparators;
+}
+
 // static
 void AutocompleteResult::LogAsynchronousUpdateMetrics(
-    const AutocompleteResult& old_result,
+    const std::vector<MatchDedupComparator>& old_result,
     const AutocompleteResult& new_result) {
   constexpr char kAsyncMatchChangeHistogramName[] =
-      "Omnibox.MatchStability.AsyncMatchChange";
+      "Omnibox.MatchStability.AsyncMatchChange2";
 
   size_t min_size = std::min(old_result.size(), new_result.size());
   for (size_t i = 0; i < min_size; ++i) {
-    if (GetMatchComparisonFields(old_result.match_at(i)) !=
-        GetMatchComparisonFields(new_result.match_at(i))) {
+    if (old_result[i] != GetMatchComparisonFields(new_result.match_at(i))) {
       base::UmaHistogramExactLinear(kAsyncMatchChangeHistogramName, i,
                                     kMaxAutocompletePositionValue);
     }

components/omnibox/browser/autocomplete_result.h

@@ -28,6 +28,7 @@ class AutocompleteResult {
  public:
   typedef ACMatches::const_iterator const_iterator;
   typedef ACMatches::iterator iterator;
+  using MatchDedupComparator = std::pair<GURL, bool>;
 
   // Max number of matches we'll show from the various providers.
   static size_t GetMaxMatches(bool is_zero_suggest = false);
@@ -35,12 +36,11 @@ class AutocompleteResult {
   AutocompleteResult();
   ~AutocompleteResult();
 
-  // Copies matches from |old_matches| to provide a consistant result set. See
-  // comments in code for specifics. Will clear |old_matches| if this result is
-  // empty().
-  void CopyOldMatches(const AutocompleteInput& input,
-                      AutocompleteResult* old_matches,
-                      TemplateURLService* template_url_service);
+  // Moves matches from |old_matches| to provide a consistent result set.
+  // |old_matches| is mutated during this, and should not be used afterwards.
+  void TransferOldMatches(const AutocompleteInput& input,
+                          AutocompleteResult* old_matches,
+                          TemplateURLService* template_url_service);
 
   // Adds a new set of matches to the result set.  Does not re-sort.  Calls
   // PossiblySwapContentsAndDescriptionForURLSuggestion(input)" on all added
@@ -149,9 +149,13 @@ class AutocompleteResult {
   // See base/trace_event/memory_usage_estimator.h for more info.
   size_t EstimateMemoryUsage() const;
 
+  // Get a list of comparators used for deduping for the matches in this result.
+  std::vector<MatchDedupComparator> GetMatchDedupComparators() const;
+
   // Logs metrics for when |new_result| replaces |old_result| asynchronously.
+  // |old_result| a list of the comparators for the old matches.
   static void LogAsynchronousUpdateMetrics(
-      const AutocompleteResult& old_result,
+      const std::vector<MatchDedupComparator>& old_result,
       const AutocompleteResult& new_result);
 
  private:
@@ -209,7 +213,7 @@ class AutocompleteResult {
   // collapse similar URLs if necessary, and whether the match is a calculator
   // suggestion, because we don't want to dedupe them against URLs that simply
   // happen to go to the same destination.
-  static std::pair<GURL, bool> GetMatchComparisonFields(
+  static MatchDedupComparator GetMatchComparisonFields(
       const AutocompleteMatch& match);
 
   // This method reduces the number of navigation suggestions to that of

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -149,10 +149,13 @@ class AutocompleteResultTest : public testing::Test {
                            size_t expected_count);
 
   // Creates an AutocompleteResult from |last| and |current|. The two are
-  // merged by |CopyOldMatches| and compared by |AssertResultMatches|.
-  void RunCopyOldMatchesTest(const TestData* last, size_t last_size,
-                             const TestData* current, size_t current_size,
-                             const TestData* expected, size_t expected_size);
+  // merged by |TransferOldMatches| and compared by |AssertResultMatches|.
+  void RunTransferOldMatchesTest(const TestData* last,
+                                 size_t last_size,
+                                 const TestData* current,
+                                 size_t current_size,
+                                 const TestData* expected,
+                                 size_t expected_size);
 
   // Returns a (mock) AutocompleteProvider of given |provider_id|.
   MockAutocompleteProvider* GetProvider(int provider_id) {
@@ -214,10 +217,12 @@ void AutocompleteResultTest::AssertResultMatches(
   }
 }
 
-void AutocompleteResultTest::RunCopyOldMatchesTest(
-    const TestData* last, size_t last_size,
-    const TestData* current, size_t current_size,
-    const TestData* expected, size_t expected_size) {
+void AutocompleteResultTest::RunTransferOldMatchesTest(const TestData* last,
+                                                       size_t last_size,
+                                                       const TestData* current,
+                                                       size_t current_size,
+                                                       const TestData* expected,
+                                                       size_t expected_size) {
   AutocompleteInput input(base::ASCIIToUTF16("a"),
                           metrics::OmniboxEventProto::OTHER,
                           TestSchemeClassifier());
@@ -233,8 +238,8 @@ void AutocompleteResultTest::RunCopyOldMatchesTest(
   AutocompleteResult current_result;
   current_result.AppendMatches(input, current_matches);
   current_result.SortAndCull(input, template_url_service_.get());
-  current_result.CopyOldMatches(input, &last_result,
-                                template_url_service_.get());
+  current_result.TransferOldMatches(input, &last_result,
+                                    template_url_service_.get());
 
   AssertResultMatches(current_result, expected, expected_size);
 }
@@ -273,7 +278,7 @@ TEST_F(AutocompleteResultTest, Swap) {
 
 // Tests that if the new results have a lower max relevance score than last,
 // any copied results have their relevance shifted down.
-TEST_F(AutocompleteResultTest, CopyOldMatches) {
+TEST_F(AutocompleteResultTest, TransferOldMatches) {
   TestData last[] = {
     { 0, 1, 1000, true },
     { 1, 1, 500,  true },
@@ -286,15 +291,15 @@ TEST_F(AutocompleteResultTest, CopyOldMatches) {
     { 1, 1, 399,  true },
   };
 
-  ASSERT_NO_FATAL_FAILURE(RunCopyOldMatchesTest(last, base::size(last), current,
-                                                base::size(current), result,
-                                                base::size(result)));
+  ASSERT_NO_FATAL_FAILURE(RunTransferOldMatchesTest(
+      last, base::size(last), current, base::size(current), result,
+      base::size(result)));
 }
 
 // Tests that if the new results have a lower max relevance score than last,
 // any copied results have their relevance shifted down when the allowed to
 // be default constraint comes into play.
-TEST_F(AutocompleteResultTest, CopyOldMatchesAllowedToBeDefault) {
+TEST_F(AutocompleteResultTest, TransferOldMatchesAllowedToBeDefault) {
   TestData last[] = {
     { 0, 1, 1300,  true },
     { 1, 1, 1200,  true },
@@ -312,13 +317,13 @@ TEST_F(AutocompleteResultTest, CopyOldMatchesAllowedToBeDefault) {
     { 2, 1, 899,  true },
   };
 
-  ASSERT_NO_FATAL_FAILURE(RunCopyOldMatchesTest(last, base::size(last), current,
-                                                base::size(current), result,
-                                                base::size(result)));
+  ASSERT_NO_FATAL_FAILURE(RunTransferOldMatchesTest(
+      last, base::size(last), current, base::size(current), result,
+      base::size(result)));
 }
 
 // Tests that matches are copied correctly from two distinct providers.
-TEST_F(AutocompleteResultTest, CopyOldMatchesMultipleProviders) {
+TEST_F(AutocompleteResultTest, TransferOldMatchesMultipleProviders) {
   TestData last[] = {
     { 0, 1, 1300, false },
     { 1, 2, 1250, true  },
@@ -341,14 +346,15 @@ TEST_F(AutocompleteResultTest, CopyOldMatchesMultipleProviders) {
     { 4, 1, 499,  false  },
   };
 
-  ASSERT_NO_FATAL_FAILURE(RunCopyOldMatchesTest(last, base::size(last), current,
-                                                base::size(current), result,
-                                                base::size(result)));
+  ASSERT_NO_FATAL_FAILURE(RunTransferOldMatchesTest(
+      last, base::size(last), current, base::size(current), result,
+      base::size(result)));
 }
 
 // Tests that matches are copied correctly from two distinct providers when
 // one provider doesn't have a current legal default match.
-TEST_F(AutocompleteResultTest, CopyOldMatchesWithOneProviderWithoutDefault) {
+TEST_F(AutocompleteResultTest,
+       TransferOldMatchesWithOneProviderWithoutDefault) {
   TestData last[] = {
     { 0, 2, 1250, true  },
     { 1, 2, 1150, true  },
@@ -369,9 +375,9 @@ TEST_F(AutocompleteResultTest, CopyOldMatchesWithOneProviderWithoutDefault) {
     { 7, 1, 500,  true  },
   };
 
-  ASSERT_NO_FATAL_FAILURE(RunCopyOldMatchesTest(last, base::size(last), current,
-                                                base::size(current), result,
-                                                base::size(result)));
+  ASSERT_NO_FATAL_FAILURE(RunTransferOldMatchesTest(
+      last, base::size(last), current, base::size(current), result,
+      base::size(result)));
 }
 
 // Tests that matches with empty destination URLs aren't treated as duplicates
@@ -844,6 +850,7 @@ TEST_F(AutocompleteResultTest, LogAsynchronousUpdateMetrics) {
   last_result.AppendMatches(input, last_matches);
   for (auto& match : last_result)
     match.ComputeStrippedDestinationURL(input, template_url_service_.get());
+  const auto last_comparators = last_result.GetMatchDedupComparators();
 
   ACMatches current_matches;
   PopulateAutocompleteMatches(current, base::size(current), &current_matches);
@@ -856,12 +863,13 @@ TEST_F(AutocompleteResultTest, LogAsynchronousUpdateMetrics) {
   base::HistogramTester histograms;
 
   // Do the logging.
-  AutocompleteResult::LogAsynchronousUpdateMetrics(last_result, current_result);
+  AutocompleteResult::LogAsynchronousUpdateMetrics(last_comparators,
+                                                   current_result);
 
   // Expect the default match, third match, and last two matches to be logged
   // as changed, and nothing else.
   EXPECT_THAT(
-      histograms.GetAllSamples("Omnibox.MatchStability.AsyncMatchChange"),
+      histograms.GetAllSamples("Omnibox.MatchStability.AsyncMatchChange2"),
       testing::ElementsAre(base::Bucket(0, 1), base::Bucket(2, 1),
                            base::Bucket(3, 1), base::Bucket(4, 1)));
 }

tools/metrics/histograms/histograms.xml

@@ -96330,6 +96330,21 @@ uploading your change for review.
 </histogram>
 
 <histogram name="Omnibox.MatchStability.AsyncMatchChange" units="position"
+    expires_after="2019-11-10">
+  <obsolete>
+    Deprecated 2019-11-10. Replaced by Omnibox.MatchStability.AsyncMatchChange2.
+  </obsolete>
+  <owner>tommycli@chromium.org</owner>
+  <owner>jdonnelly@chromium.org</owner>
+  <summary>
+    This histogram was buggy and the data should be disregarded.
+
+    The original intent was to count asynchronous match updates for each
+    position. The implementation had a bug, and it overcounted.
+  </summary>
+</histogram>
+
+<histogram name="Omnibox.MatchStability.AsyncMatchChange2" units="position"
     expires_after="2020-03-01">
   <owner>tommycli@chromium.org</owner>
   <owner>jdonnelly@chromium.org</owner>