[omnibox] Fix crash from logging selected_index == kNoMatch (-1)

This CL made it much more likely that OmniboxPopupModel::selected_line()
could return kNoMatch (-1):
https://chromium-review.googlesource.com/c/chromium/src/+/1906882

But when we put kNoMatch into a size_t index, we get the maximum value
size_t, i.e. 0xffffffff. That just overruns the results array and reads
garbage memory.

Instead we should ignore the popup when we are selecting a match that's
not from the popup (no default match case).

This CL does that, and adds a DCHECK to OmniboxLog to prevent this from
happening again.

Bug: 1034946
Change-Id: I989c139c9a9b9525555424987124627130f0a5d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1989517Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729801}

components/omnibox/browser/omnibox_edit_model.cc

@@ -767,15 +767,21 @@ void OmniboxEditModel::OpenMatch(AutocompleteMatch match,
     elapsed_time_since_user_first_modified_omnibox = default_time_delta;
     elapsed_time_since_last_change_to_default_match = default_time_delta;
   }
-  // If the popup is closed or this is a paste-and-go action (meaning the
-  // contents of the dropdown are ignored regardless), we record for logging
-  // purposes a selected_index of 0 and a suggestion list as having a single
-  // entry of the match used.
-  const bool dropdown_ignored = !popup_open || !pasted_text.empty();
+
+  // In some unusual cases, we ignore result() and instead log a fake result set
+  // with a single element (|match|) and selected_index of 0. For these cases:
+  //  1. If the popup is closed (there is no result set).
+  //  2. If the index is out of bounds. This should only happen if |index| is
+  //     kNoMatch, which can happen if the default search provider is disabled.
+  //  3. If this is a paste-and-go action (meaning the contents of the dropdown
+  //     are ignored regardless).
+  const bool dropdown_ignored =
+      !popup_open || index >= result().size() || !pasted_text.empty();
   ACMatches fake_single_entry_matches;
   fake_single_entry_matches.push_back(match);
   AutocompleteResult fake_single_entry_result;
   fake_single_entry_result.AppendMatches(input_, fake_single_entry_matches);
+
   OmniboxLog log(
       input_.from_omnibox_focus() ? base::string16() : input_text,
       just_deleted_text_, input_.type(), is_keyword_selected(),

components/omnibox/browser/omnibox_edit_model.h

@@ -215,7 +215,11 @@ class OmniboxEditModel {
       WindowOpenDisposition disposition,
       base::TimeTicks match_selection_timestamp = base::TimeTicks());
 
-  // Asks the browser to load the item at |index|, with the given properties.
+  // Asks the browser to load |match|. |index| is only used for logging, and
+  // can be kNoMatch if the popup was closed, or if none of the suggestions
+  // in the popup were used (in the unusual no-default-match case). In that
+  // case, an artificial result set with only |match| will be logged.
+  //
   // OpenMatch() needs to know the original text that drove this action.  If
   // |pasted_text| is non-empty, this is a Paste-And-Go/Search action, and
   // that's the relevant input text.  Otherwise, the relevant input text is

components/omnibox/browser/omnibox_log.cc

@@ -4,6 +4,8 @@
 
 #include "components/omnibox/browser/omnibox_log.h"
 
+#include "components/omnibox/browser/autocomplete_result.h"
+
 OmniboxLog::OmniboxLog(
     const base::string16& text,
     bool just_deleted_text,
@@ -36,6 +38,10 @@ OmniboxLog::OmniboxLog(
       completed_length(completed_length),
       elapsed_time_since_last_change_to_default_match(
           elapsed_time_since_last_change_to_default_match),
-      result(result) {}
+      result(result) {
+  DCHECK(selected_index < result.size())
+      << "The selected index should always be valid. See comments on "
+         "OmniboxLog::selected_index.";
+}
 
 OmniboxLog::~OmniboxLog() {}