Null-check GetView before using it in GetFocusedRenderWidgetHost.

Null-check GetView before using it in GetFocusedRenderWidgetHost.

This prevents GetFocusedRenderWidgetHost from crashing in the case
where a focused subframe's renderer has crashed, and we try typing in
the same tab.

BUG=567345
Committed: https://crrev.com/732ca3a09e2da139475f36f883b99d587532dd95
Cr-Commit-Position: refs/heads/master@{#363703}

[alexmos, 2015-12-07 22:40:14]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2015-12-07 22:40:15]

Charlie, can you please take a look?  This should fix the crash you've
encountered.

[Charlie Reis, 2015-12-07 22:47:25]

Quick sanity check; otherwise looks good.

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1651: // the subframe has
focus.  Give the event to the main frame in this case.
Sanity check: What will the main frame do with it?  Will it recognize that none
of its frames had focus and thus do nothing with it?  Or will the event go to
something in the main frame?

I worry about a user typing something long into a subframe, and half of it
unexpectedly going into a different form in the main frame if the subframe
crashes while they're typing.

[alexmos, 2015-12-07 23:12:59]

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1651: // the subframe has
focus.  Give the event to the main frame in this case.
On 2015/12/07 22:47:24, Charlie Reis wrote:
> Sanity check: What will the main frame do with it?  Will it recognize that
none
> of its frames had focus and thus do nothing with it?  Or will the event go to
> something in the main frame?
> 
> I worry about a user typing something long into a subframe, and half of it
> unexpectedly going into a different form in the main frame if the subframe
> crashes while they're typing.

That's a good point.  It's going to go to the main frame.  I've tried this out,
and if you start typing into field A on the main frame, then focus field B in a
subframe and start typing, and then crash the subframe renderer, keyboard input
from that point on will go into field A on the main frame.  I agree this
behavior would be unexpected/dangerous.  

I could return nullptr instead of the main frame here, though I'd need to also
update the three call sites for GetFocusedRWH to also do a null-check and drop
the event in this case.  Does this plan sound good?

[Charlie Reis, 2015-12-07 23:16:52]

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1651: // the subframe has
focus.  Give the event to the main frame in this case.
On 2015/12/07 23:12:59, alexmos wrote:
> On 2015/12/07 22:47:24, Charlie Reis wrote:
> > Sanity check: What will the main frame do with it?  Will it recognize that
> none
> > of its frames had focus and thus do nothing with it?  Or will the event go
to
> > something in the main frame?
> > 
> > I worry about a user typing something long into a subframe, and half of it
> > unexpectedly going into a different form in the main frame if the subframe
> > crashes while they're typing.
> 
> That's a good point.  It's going to go to the main frame.  I've tried this
out,
> and if you start typing into field A on the main frame, then focus field B in
a
> subframe and start typing, and then crash the subframe renderer, keyboard
input
> from that point on will go into field A on the main frame.  I agree this
> behavior would be unexpected/dangerous.  
> 
> I could return nullptr instead of the main frame here, though I'd need to also
> update the three call sites for GetFocusedRWH to also do a null-check and drop
> the event in this case.  Does this plan sound good?

Yes, let's do that.  Thanks!

[alexmos, 2015-12-07 23:21:50]

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1505723005/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1651: // the subframe has
focus.  Give the event to the main frame in this case.
On 2015/12/07 23:16:52, Charlie Reis wrote:
> On 2015/12/07 23:12:59, alexmos wrote:
> > On 2015/12/07 22:47:24, Charlie Reis wrote:
> > > Sanity check: What will the main frame do with it?  Will it recognize that
> > none
> > > of its frames had focus and thus do nothing with it?  Or will the event go
> to
> > > something in the main frame?
> > > 
> > > I worry about a user typing something long into a subframe, and half of it
> > > unexpectedly going into a different form in the main frame if the subframe
> > > crashes while they're typing.
> > 
> > That's a good point.  It's going to go to the main frame.  I've tried this
> out,
> > and if you start typing into field A on the main frame, then focus field B
in
> a
> > subframe and start typing, and then crash the subframe renderer, keyboard
> input
> > from that point on will go into field A on the main frame.  I agree this
> > behavior would be unexpected/dangerous.  
> > 
> > I could return nullptr instead of the main frame here, though I'd need to
also
> > update the three call sites for GetFocusedRWH to also do a null-check and
drop
> > the event in this case.  Does this plan sound good?
> 
> Yes, let's do that.  Thanks!

Done.  Please take a look.

[Charlie Reis, 2015-12-07 23:27:02]

Thanks!  LGTM.

[alexmos, 2015-12-07 23:35:27]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-12-07 23:39:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505723005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505723005/20001

[commit-bot: I haz the power, 2015-12-08 00:57:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 00:57:34]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexmos, 2015-12-08 01:11:48]

The CQ bit was checked by alexmos@chromium.org

[Charlie Reis, 2015-12-08 01:12:06]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-12-08 01:14:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505723005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505723005/20001

[commit-bot: I haz the power, 2015-12-08 02:01:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-08 02:02:05]

Description was changed from

==========
Null-check GetView before using it in GetFocusedRenderWidgetHost.

This prevents GetFocusedRenderWidgetHost from crashing in the case
where a focused subframe's renderer has crashed, and we try typing in
the same tab.

BUG=567345
==========

to

==========
Null-check GetView before using it in GetFocusedRenderWidgetHost.

This prevents GetFocusedRenderWidgetHost from crashing in the case
where a focused subframe's renderer has crashed, and we try typing in
the same tab.

BUG=567345

Committed: https://crrev.com/732ca3a09e2da139475f36f883b99d587532dd95
Cr-Commit-Position: refs/heads/master@{#363703}
==========

[commit-bot: I haz the power, 2015-12-08 02:02:06]

Patchset 2 (id:??) landed as
https://crrev.com/732ca3a09e2da139475f36f883b99d587532dd95
Cr-Commit-Position: refs/heads/master@{#363703}