Instant: Don't allow the page to set suggestions inappropriately.

chrome/browser/ui/search/instant_controller.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/search/instant_controller.h"
 
 #include "base/metrics/histogram.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
@@ skipping 1115 matching lines @@
       !last_omnibox_text_.empty();
   if (!can_use_instant_tab && !can_use_overlay)
     return;
 
   if (suggestion.behavior == INSTANT_COMPLETE_REPLACE) {
     // We don't get an Update() when changing the omnibox due to a REPLACE
     // suggestion (so that we don't inadvertently cause the overlay to change
     // what it's showing, as the user arrows up/down through the page-provided
     // suggestions). So, update these state variables here.
     last_omnibox_text_ = suggestion.text;
+    last_user_text_.clear();

[samarth, 2013/04/09 16:11:34]

IIRC, when you arrow back up into the omnibox, we don't get an Update(), so
won't last_user_text_ be wrong at that point?

[sreeram, 2013/04/09 16:30:37]

When you arrow back into the omnibox, you get a setValue() (so it's still
temporary text). |last_user_text_| won't get updated until the user starts
typing again.

[samarth, 2013/04/09 20:32:15]

Sorry if I'm missing something but I still don't see how this is safe.  We use
last_user_text_ to validate suggestions in FixSuggestions below, and this will
clear it while we're arrowing through.

[sreeram, 2013/04/09 20:58:24]

FixSuggestions is only called for gray/blue suggestions, not for setValue()
suggestions. So, as you arrow up/down, the page calls setValue(), but we don't
need last_user_text_ to validate setValue() (indeed, we don't do any validation
whatsoever).

Before the next time the page can legally set gray/blue text, the user must type
something in the omnibox, at which point, last_user_text_ will be set correctly.

[samarth, 2013/04/09 21:49:06]

But what if we want to use last_user_text_ to validate setValue() calls as well?
 We don't yet but may very soon.  Also, we may want to set blue or gray text
when you arrow back into the omnibox, no?

So actually I have two issues here:

(1) Based on my understanding of what |last_user_text_| is meant to be, it seems
wrong and bug-prone to clobber it like this without a corresponding Update(). If
you convince me that last_user_text_ really has no meaning once setValue() has
been called, then I'm ok with it. But I thought omnibox edit keeps track of it
still in that case and I'd rather not diverge our state.

(2) This fix in general is making a lot of specific assumptions about exactly
what sequence of calls the page will be making, which I'm not happy about. I
know we're always treading the line between making the API flexible and
protecting Chrome from server-side bugs. My view on this is that we can never
truly protect Chrome from all possible server-side issues so in general, I'd
like to only protect against really bad and annoying breakages (e.g. I type in a
URL, hit enter and it navigates) and keep the API flexible otherwise. I don't
think this bug falls into that category. Thoughts?

[sreeram, 2013/04/09 22:11:58]

last_user_text_ is not needed for validating setValue(). If you are thinking
about something like "suggestion->query_ == last_user_text_", that's not
necessary. Chrome knows when to accept setValue() and when not to, regardless of
what the actual last_user_text_ is. I.e., if we decide to validate setValue(),
we'll do so without looking at the actual last_user_text_.
I am not sure we'd want to do that. Current (non-1993) omnibox doesn't do that.
I'd argue that it would be somewhat surprising as well (sudden appearance and
disappearance of gray/blue text as you arrow up/down).

Assuming we still want to do it, we need the support of the OmniboxEditModel
anyway, so that it reverts the temporary text and sets back the user_text along
with gray/blue completion, which gives us an opportunity to restore
last_user_text_.
Yes, indeed, that's how I'm treating last_user_text_: Once setValue() has been
called, it loses meaning until the next Update(). This fits with the omnibox's
treatment: setValue() sets temporary text (last_omnibox_text_), so user text is
ignored until the temporary text is cleared (or becomes implicitly accepted as
the user text).
I think we should build Chrome to be as resilient to server bugs as possible.
We've gone back and forth on this. In the beginning, we tried to put in a lot of
checks to prevent the page doing this or that. Those checks were often in
response to bugs (such as the non-monotonicity in completeserver responses,
e.g.: "jet blue", "vacations to go"). Over time, we found that those checks got
in the way of the page wanting to do things (such as going from a navsuggest to
a gray text completion as the user typed along; i.e., in some cases, the
non-monotonicity was desired). So we loosened those checks, and we are back to
bugs caused by the server again.

If this happens in production, after launch, it will be really bad. It's easy to
say "such server issues will be fixed in a hurry", but it will still make us
look really bad (apart from messing with the user experience). And once it
happens more than a couple of times, we'll have people asking the obvious
question: "Why didn't you guys just put more checks and balances into Chrome?".

     last_suggestion_ = InstantSuggestion();
     last_match_was_search_ = suggestion.type == INSTANT_SUGGESTION_SEARCH;
     LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
         "ReplaceSuggestion text='%s' type=%d",
         UTF16ToUTF8(suggestion.text).c_str(), suggestion.type));
     browser_->SetInstantSuggestion(suggestion);
   } else {
     if (FixSuggestion(&suggestion)) {
       last_suggestion_ = suggestion;
       if (suggestion.type == INSTANT_SUGGESTION_SEARCH &&
@@ skipping 392 matching lines @@
     ntp_->SendMostVisitedItems(items);
   if (instant_tab_)
     instant_tab_->SendMostVisitedItems(items);
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_SENT_MOST_VISITED_ITEMS,
       content::Source<InstantController>(this),
       content::NotificationService::NoDetails());
 }
 
 bool InstantController::FixSuggestion(InstantSuggestion* suggestion) const {
+  // If the page is attempting to set suggestions inappropriately, reject.

[samarth, 2013/04/09 16:11:34]

Please be more specific--both here and in the change description--about what is
"inappopriate" in this case.

More generally, I wish we had a consolidated view of what the API allows. What I
mean is instead of just having random second-guessing sprinkled through
InstantController, it'd be nice to have it part of the API: you can control the
omnibox like a dumb input field using these functions but with the following
restrictions.

[sreeram, 2013/04/09 16:30:37]

Done.

+  if (last_user_text_.empty())
+    return false;
+
   // If the page is trying to set inline autocompletion in verbatim mode,
   // instead try suggesting the exact omnibox text. This makes the omnibox
   // interpret user text as an URL if possible while preventing unwanted
   // autocompletion during backspacing.
   if (suggestion->behavior == INSTANT_COMPLETE_NOW && last_verbatim_)
     suggestion->text = last_omnibox_text_;
 
   // Suggestion text should be a full URL for URL suggestions, or the
   // completion of a query for query suggestions.
   if (suggestion->type == INSTANT_SUGGESTION_URL) {
@@ skipping 54 matching lines @@
     return false;
 
   // If this is not window startup, switch.
   // TODO(shishir): This is not completely reliable. Find a better way to detect
   // startup time.
   if (browser_->GetActiveWebContents())
     return true;
 
   return chrome::IsAggressiveLocalNTPFallbackEnabled();
 }
-