Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13813006:
Instant: Don't allow the page to set suggestions inappropriately. (Closed)
|
Created:
7 years, 8 months ago by sreeram Modified:
7 years, 8 months ago Reviewers:
samarth CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInstant: Don't allow the page to set suggestions inappropriately.
This guards against internal bug b/8572045. If |last_user_text_| is empty, it
means the user is arrowing up/down the suggestions list (as opposed to typing
text into the omnibox), in which case we reject new suggestions.
BUG=225326
R=samarth@chromium.org
TEST=See bug (the second case in comment #0; and the case in comment #2).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194062
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments #Patch Set 3 : .h comment #Patch Set 4 : Rebase #
Messages
Total messages: 13 (0 generated)
Please review.
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); 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? https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1549: // If the page is attempting to set suggestions inappropriately, reject. 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.
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); On 2013/04/09 16:11:34, samarth wrote: > 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? 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. https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1549: // If the page is attempting to set suggestions inappropriately, reject. On 2013/04/09 16:11:34, samarth wrote: > Please be more specific--both here and in the change description--about what is > "inappopriate" in this case. Done.
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); On 2013/04/09 16:30:37, sreeram wrote: > On 2013/04/09 16:11:34, samarth wrote: > > 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? > > 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. 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.
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); On 2013/04/09 20:32:15, samarth wrote: > 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. 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.
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); On 2013/04/09 20:58:24, sreeram wrote: > On 2013/04/09 20:32:15, samarth wrote: > > 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. > > 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. 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?
https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... File chrome/browser/ui/search/instant_controller.cc (right): https://codereview.chromium.org/13813006/diff/1/chrome/browser/ui/search/inst... chrome/browser/ui/search/instant_controller.cc:1136: last_user_text_.clear(); On 2013/04/09 21:49:06, samarth wrote: > But what if we want to use last_user_text_ to validate setValue() calls as well? 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_. > Also, we may want to set blue or gray text when you arrow back into the omnibox, no? 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_. > (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. 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). > (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? 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?".
Summarizing offline conversation: 1. It's true that this CL makes assumptions about how the API will be used. On the other hand, API flexibility is a myth right now, due to FinalizeInstantQuery and various DCHECKs. 2. The most important class of bugs we should be resilient to are those that involve the user navigating somewhere. However, other user visible bugs are bad too (even if not as bad). Just because we can't catch all server side bugs doesn't mean we shouldn't catch as many as we can. 3. Instead of clobbering last_user_text_, we could create a new bool that says whether blue/gray suggestions will be accepted. This is in principle a good idea, but I hate having to add "new_var_ = false;" all over the place (everywhere that last_user_text_ is set right now). 4. This situation will become considerably better when the omnibox refactor lands. Then, we can let the API be truly flexible, remove all these checks and institute only the ones we care most about (URL navigations). Let me know if I missed anything.
ping?
LGTM but please add a note in instant_controller.h documenting that |last_user_text_| is cleared when arrowing through suggestions. Thanks, Samarth
On 2013/04/12 13:07:27, samarth wrote: > LGTM but please add a note in instant_controller.h documenting that > |last_user_text_| is cleared when arrowing through suggestions. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sreeram@chromium.org/13813006/19002
Message was sent while issue was closed.
Change committed as 194062 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
