Refactor omnibox focus API into a single method.

Refactor omnibox focus API into a single method.

BUG=180095
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193014

[kmadhusu, 2013-03-26 18:56:17]

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:241:
delegate_->StopCapturingKeyStrokes(contents());
From my understanding of the bug report, we want to get rid of
StartCapturingKeyStrokes IPC and StopCapturingKeyStrokes IPC.

I would like to keep InstantPage::Delegate::StartCapturingKeyStrokes() and
InstantPage::Delegate::StopCapturingKeyStorkes() for better understanding of the
code. WDYT?

[sreeram, 2013-03-26 19:01:24]

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:241:
delegate_->StopCapturingKeyStrokes(contents());
On 2013/03/26 18:56:17, kmadhusu wrote:
> From my understanding of the bug report, we want to get rid of
> StartCapturingKeyStrokes IPC and StopCapturingKeyStrokes IPC.
> 
> I would like to keep InstantPage::Delegate::StartCapturingKeyStrokes() and
> InstantPage::Delegate::StopCapturingKeyStorkes() for better understanding of
the
> code. WDYT?

Let's not. InstantPage should be a simple message relay, and complicated logic
doesn't belong here. So, do these in InstantController instead.

[kmadhusu, 2013-03-26 21:04:00]

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/12851023/diff/5001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:241:
delegate_->StopCapturingKeyStrokes(contents());
On 2013/03/26 19:01:24, sreeram wrote:
> On 2013/03/26 18:56:17, kmadhusu wrote:
> > From my understanding of the bug report, we want to get rid of
> > StartCapturingKeyStrokes IPC and StopCapturingKeyStrokes IPC.
> > 
> > I would like to keep InstantPage::Delegate::StartCapturingKeyStrokes() and
> > InstantPage::Delegate::StopCapturingKeyStorkes() for better understanding of
> the
> > code. WDYT?
> 
> Let's not. InstantPage should be a simple message relay, and complicated logic
> doesn't belong here. So, do these in InstantController instead.

Done. Moved this logic to InstantController.

Can you tell me the use case when ShouldProcessStopCapturingKeyStrokes() or
ShouldProcessStartCapturingKeyStrokes() returns false? Is it okay to do these
checks in InstantController?

[sreeram, 2013-04-01 16:04:38]

On 2013/03/26 21:04:00, kmadhusu wrote:
> Can you tell me the use case when ShouldProcessStopCapturingKeyStrokes() or
> ShouldProcessStartCapturingKeyStrokes() returns false? Is it okay to do these
> checks in InstantController?

The Should*() stuff is a way for the subclasses of InstantPage to say which
messages should be handled. By default, InstantPage returns false for all of
them. InstantOverlay and InstantNTP do not override these methods, so the
Focus/StartCapturing/StopCapturing IPCs are ignored if they come from a contents
owned by those classes.

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1198: if
(instant_tab_->ShouldProcessFocusOmnibox())
These Should*() checks are not needed here. So, this should just read something
like this:

switch (state) {
  case OMNIBOX_FOCUS_VISIBLE:
    browser_->FocusOmnibox(true);
    break;
  case OMNIBOX_FOCUS_INVISIBLE:
    browser_->FocusOmnibox(false);
    break;
  case OMNIBOX_FOCUS_NONE:
    if (omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE)
      contents->GetView()->Focus();
    break;
}

Leave out the "default:" clause. The idea is that, if the switch doesn't handle
all the values of the enum, the compiler will complain (which is better than a
run-time failure in NOTREACHED).

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:230:
delegate_->FocusOmnibox(contents(), state);
Keep the ShouldProcessFocusOmnibox() check, like so:

if (contents()->IsActiveEntry(...)) {
  OnInstantSupportDetermined(...);
  if (ShouldProcessFocusOmnibox())
    delegate_->FocusOmnibox(contents(), state);
}

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:70: // whether the omnibox focus state.
Delete "whether".

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_tab.h (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_tab.h:26: virtual bool
ShouldProcessStopCapturingKeyStrokes() OVERRIDE;
Revert this change. Remove ShouldProcess{Start,Stop}... Leave
ShouldProcessFocusOmnibox() in the private section.

[kmadhusu, 2013-04-02 02:06:50]

sreeram@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.cc:1198: if
(instant_tab_->ShouldProcessFocusOmnibox())
On 2013/04/01 16:04:38, sreeram wrote:
> These Should*() checks are not needed here. So, this should just read
something
> like this:
> 
> switch (state) {
>   case OMNIBOX_FOCUS_VISIBLE:
>     browser_->FocusOmnibox(true);
>     break;
>   case OMNIBOX_FOCUS_INVISIBLE:
>     browser_->FocusOmnibox(false);
>     break;
>   case OMNIBOX_FOCUS_NONE:
>     if (omnibox_focus_state_ == OMNIBOX_FOCUS_INVISIBLE)
>       contents->GetView()->Focus();
>     break;
> }
> 
> Leave out the "default:" clause. The idea is that, if the switch doesn't
handle
> all the values of the enum, the compiler will complain (which is better than a
> run-time failure in NOTREACHED).

Done.

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.cc:230:
delegate_->FocusOmnibox(contents(), state);
On 2013/04/01 16:04:38, sreeram wrote:
> Keep the ShouldProcessFocusOmnibox() check, like so:
> 
> if (contents()->IsActiveEntry(...)) {
>   OnInstantSupportDetermined(...);
>   if (ShouldProcessFocusOmnibox())
>     delegate_->FocusOmnibox(contents(), state);
> }

Done. Removed the other Should*() functions.

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_page.h (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_page.h:70: // whether the omnibox focus state.
On 2013/04/01 16:04:38, sreeram wrote:
> Delete "whether".

Done.

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_tab.h (right):

https://codereview.chromium.org/12851023/diff/6009/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_tab.h:26: virtual bool
ShouldProcessStopCapturingKeyStrokes() OVERRIDE;
On 2013/04/01 16:04:38, sreeram wrote:
> Revert this change. Remove ShouldProcess{Start,Stop}... Leave
> ShouldProcessFocusOmnibox() in the private section.

Done.

[kmadhusu, 2013-04-05 21:10:26]

sreeram@: ping.

Thanks.

[sreeram, 2013-04-05 21:14:31]

LGTM

Sorry this had fallen off my radar.

[kmadhusu, 2013-04-05 21:22:00]

jln@: Please do the OWNER's check for chrome/common/* changes.

sky@: Please do the OWNER's check for chrome/browser/ui/* and
chrome/chrome_common.gypi.

Thanks.

[jln (very slow on Chromium), 2013-04-05 21:32:44]

Fairly minor issue, but we should check more tightly any IPC parameter.

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
I'm surprised the compiler didn't complain about default: here.

Please, add a default: statement and a "CHECK(false);  // An attacker may have
sent a malicious state index".

Don't make it a DCHECK or a NOTREACHED(), perhaps even add a comment that this
should be a CHECK so that no-one is tempted to change it.

[sreeram, 2013-04-05 21:47:12]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 21:32:45, Julien Tinnes wrote:
> I'm surprised the compiler didn't complain about default: here.

All the enum values are explicitly handled here, so that's why the compiler
doesn't complain. I think it's probably undefined behaviour (note: I Am Not A
Standards Expert) if |state| is not one of those values, and the compiler is
free to do whatever it wants.

This is in fact, the recommended way to write switch() statements for enums. If
somebody later adds a new value for the enum without updating this code, the
compiler will complain that it's not being handled here.

> Please, add a default: statement and a "CHECK(false);  // An attacker may have
> sent a malicious state index".
> 
> Don't make it a DCHECK or a NOTREACHED(), perhaps even add a comment that this
> should be a CHECK so that no-one is tempted to change it.

This will defeat the compile time check mentioned above, but I see what you are
saying about the IPC having untrusted values. Is it really a problem though? Say
|state| was some arbitrary value. Assuming the compiler doesn't cause the
program to crash-and-burn due to undefined behaviour, the worst that happens is
that this method does nothing. I think that's a fine tradeoff (given the ability
to catch at compile time any new value being added to the enum).

[jln (very slow on Chromium), 2013-04-05 21:53:59]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 21:47:13, sreeram wrote:

> This will defeat the compile time check mentioned above, but I see what you
are
> saying about the IPC having untrusted values. Is it really a problem though?
Say
> |state| was some arbitrary value. Assuming the compiler doesn't cause the
> program to crash-and-burn due to undefined behaviour, the worst that happens
is
> that this method does nothing. I think that's a fine tradeoff (given the
ability
> to catch at compile time any new value being added to the enum).

It's all about review-ability. It's easy to assert that nothing will go wrong
for security if you have a CHECK(). Otherwise, a security reviewer, often not
familiar at all with your code, will have a really hard time assessing that.
Also it could regress.

Undefined behavior, or hard-to-define behavior is the enemy for security.

In this particular case, not a huge deal. But a goal of these new IPC security
review is to help developers understand the paranoid mindset :)

[sreeram, 2013-04-05 22:03:37]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
Okay, how about we don't add the "default:" clause, to help catch errors at
compile time, but also document the situation for the reasons you mentioned.
Something like:
// An attacker may send a bogus |state| value, but we won't do anything in that
// case, since we only handle the following explicitly listed values.
(@kmadhusu will likely come up with better wording)

[kmadhusu, 2013-04-05 22:08:50]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 22:03:38, sreeram wrote:
> Okay, how about we don't add the "default:" clause, to help catch errors at
> compile time, but also document the situation for the reasons you mentioned.
> Something like:
> // An attacker may send a bogus |state| value, but we won't do anything in
that
> // case, since we only handle the following explicitly listed values.
> (@kmadhusu will likely come up with better wording)

sreeram@: How about adding a COMPILE_ASSERT macro in the default case? By doing
that, we can address both the concerns.

Ref:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[jln (very slow on Chromium), 2013-04-05 22:32:43]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 22:08:51, kmadhusu wrote:
> On 2013/04/05 22:03:38, sreeram wrote:
> > Okay, how about we don't add the "default:" clause, to help catch errors at
> > compile time, but also document the situation for the reasons you mentioned.
> > Something like:
> > // An attacker may send a bogus |state| value, but we won't do anything in
> that
> > // case, since we only handle the following explicitly listed values.
> > (@kmadhusu will likely come up with better wording)
> 
> sreeram@: How about adding a COMPILE_ASSERT macro in the default case? By
doing
> that, we can address both the concerns.
> 
> Ref:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

This wouldn't work and needs to be a run-time check. The problem is that we're
sending an IPC, so "type checking" would need to happen at run-time.
Unfortunately, the IPC run-time checking isn't as strict as it should.

What's more, in this situation a cast from an int will happen implicitly and the
compiler would in theory be at liberty to optimize out the default: case.

Adding Tom in case he has some thoughts about how we could improve this at the
IPC layer.

[sreeram, 2013-04-05 22:38:48]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
So it turns out enum variables are explicitly allowed to have any value that
their underlying type allows. I.e., if this enum is stored as an int, all int
values (-2^31 to 2^31-1) are legal.

So there's no undefined behaviour here. Given that, what's the point of a CHECK?
Say an evil renderer sent us an IPC with a bogus |state|. Why should we crash
the browser? I'd be in favour of killing the renderer, if anything. I'm also
fine with the "ignore it, do nothing, keep calm and carry on" approach so far in
this CL.

[jln (very slow on Chromium), 2013-04-05 22:43:50]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 22:38:48, sreeram wrote:
> So it turns out enum variables are explicitly allowed to have any value that
> their underlying type allows. I.e., if this enum is stored as an int, all int
> values (-2^31 to 2^31-1) are legal.
> 
> So there's no undefined behaviour here. Given that, what's the point of a
CHECK?
> Say an evil renderer sent us an IPC with a bogus |state|. Why should we crash
> the browser? I'd be in favour of killing the renderer, if anything. I'm also
> fine with the "ignore it, do nothing, keep calm and carry on" approach so far
in
> this CL.

As explained above, it's much harder to assess the security implications if we
just "carry on". Also this can regress and become a security issue if we do
"nothing" in this function. It's very easy to expect this function to never to
nothing.

A CHECK here won't have any run-time impact, can catch bugs and can also catch
an attacker, what's not to like?

To re-iterate, I'm not particularly worried about this particular case, but it's
good design / practice.

[sreeram, 2013-04-05 22:53:58]

On 2013/04/05 22:43:50, Julien Tinnes wrote:
> As explained above, it's much harder to assess the security implications if we
> just "carry on".

I understand the point about "harder to assess security implications". Which is
why I recommended adding that comment. So, the comment says, "We've considered
that the IPC value is insecure, and we are documenting our response, which is to
ignore it".

> Also this can regress and become a security issue if we do
> "nothing" in this function.

I don't follow this part. Can you give an example of a hypothetical regression
here? (Given that anybody modifying this method would see the above-mentioned
comment.)

> A CHECK here won't have any run-time impact, can catch bugs and can also catch
> an attacker, what's not to like?

It crashes the browser. That's a pretty heavy-handed way to deal with a rogue
renderer. As I said, I think the right thing to do is to crash the renderer (or
at least, divest ourselves of any contact with it, for example by doing
instant_tab_.reset()).

> To re-iterate, I'm not particularly worried about this particular case, but
it's
> good design / practice.

I agree about the good design / practice, and appreciate your thoughtful
comments on this. I don't mean to bikeshed this issue further. I just don't want
us to crash unnecessarily when, IMHO, we have better alternatives (ignoring or
crashing the renderer).

[jln (very slow on Chromium), 2013-04-05 23:02:11]

On 2013/04/05 22:53:58, sreeram wrote:
> On 2013/04/05 22:43:50, Julien Tinnes wrote:
> > As explained above, it's much harder to assess the security implications if
we
> > just "carry on".
> 
> I understand the point about "harder to assess security implications". Which
is
> why I recommended adding that comment. So, the comment says, "We've considered
> that the IPC value is insecure, and we are documenting our response, which is
to
> ignore it".

Ok, I think I missed that. That's fine with me as well. Right now unfortunately
we can't help malicious renderers to DoS the browser, but it's not a nice
position to be in.

If that's clearly documented, I'm happy with it.

> > Also this can regress and become a security issue if we do
> > "nothing" in this function.
> 
> I don't follow this part. Can you give an example of a hypothetical regression
> here? (Given that anybody modifying this method would see the above-mentioned
> comment.)

Again, missed the part about the comment. Typical regression with enums like
this is someone creating an array indexed by the enum. Rogue index becomes
memory corruption.

[kmadhusu, 2013-04-05 23:51:37]

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/12851023/diff/20001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1189: }
On 2013/04/05 22:43:50, Julien Tinnes wrote:
> On 2013/04/05 22:38:48, sreeram wrote:
> > So it turns out enum variables are explicitly allowed to have any value that
> > their underlying type allows. I.e., if this enum is stored as an int, all
int
> > values (-2^31 to 2^31-1) are legal.
> > 
> > So there's no undefined behaviour here. Given that, what's the point of a
> CHECK?
> > Say an evil renderer sent us an IPC with a bogus |state|. Why should we
crash
> > the browser? I'd be in favour of killing the renderer, if anything. I'm also
> > fine with the "ignore it, do nothing, keep calm and carry on" approach so
far
> in
> > this CL.
> 
> As explained above, it's much harder to assess the security implications if we
> just "carry on". Also this can regress and become a security issue if we do
> "nothing" in this function. It's very easy to expect this function to never to
> nothing.
> 
> A CHECK here won't have any run-time impact, can catch bugs and can also catch
> an attacker, what's not to like?
> 
> To re-iterate, I'm not particularly worried about this particular case, but
it's
> good design / practice.

Added a detailed comment. PTAL.

[sreeram, 2013-04-05 23:55:15]

SLGTM

[jln (very slow on Chromium), 2013-04-06 00:15:09]

chrome/common/render_messages.h lgtm

[kmadhusu, 2013-04-08 18:40:45]

sky@/ben@: Please do the OWNER's check for chrome/browser/ui/* and
chrome/chrome_common.gypi.

Thanks.

[Ben Goodger (Google), 2013-04-08 21:21:33]

lgtm

[commit-bot: I haz the power, 2013-04-08 21:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/12851023/36001

[commit-bot: I haz the power, 2013-04-09 05:16:07]

Change committed as 193014