Check for null return from GetNativeView().

Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
Committed: https://crrev.com/079861f265916d3b9d6323a2f4cf07cdba864a05
Cr-Commit-Position: refs/heads/master@{#417288}

[wjmaclean, 2016-09-07 13:52:34]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2016-09-07 13:52:47]

Description was changed from

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
==========

to

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
==========

[wjmaclean, 2016-09-07 13:52:47]

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

[commit-bot: I haz the power, 2016-09-07 13:52:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wjmaclean, 2016-09-07 13:53:10]

wjmaclean@chromium.org changed reviewers:
+ jochen@chromium.org

[wjmaclean, 2016-09-07 13:53:33]

jochen@ and creis@ ... ptal?

[jochen (gone - plz use gerrit), 2016-09-07 14:27:29]

is it possible to trigger the crash in a test?

[wjmaclean, 2016-09-07 14:29:27]

On 2016/09/07 14:27:29, jochen wrote:
> is it possible to trigger the crash in a test?

I'm not sure yet, but will try to find a meaningful test later today (after
perf).

[commit-bot: I haz the power, 2016-09-07 14:39:10]

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

[commit-bot: I haz the power, 2016-09-07 14:39:10]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-07 17:23:58]

Not sure what to suggest on the test.  I tried to repro it locally on a Dev
Pixel by opening the onscreen keyboard while I had a page open with OOPIFs, but
it didn't crash.

As a result, we don't have known repro steps for this, but it's a top browser
crash on ChromeOS (#5).  I think it's worth landing a fix ASAP.

https://codereview.chromium.org/2322513002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_keyboard_ui.cc (right):

https://codereview.chromium.org/2322513002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_keyboard_ui.cc:190: return true;
I would suggest putting the null check in KeyboardUIContent::InitInsets instead.
 There's another caller of ShouldWindowOverscroll which passes
GetContainerWindow instead of GetNativeView, and we wouldn't want to paper over
a bug there if there was one.

[Charlie Reis, 2016-09-07 17:25:44]

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

[Charlie Reis, 2016-09-07 17:25:44]

Adding Ben in case he has thoughts on how we might repro this crash.  (I'm not
certain that KeyboardUIContent::InitInsets is about the onscreen keyboard, so
maybe my repro steps weren't correct.)

[wjmaclean, 2016-09-07 17:41:05]

Ptal? I'll continue to investigate how we might test this.

https://codereview.chromium.org/2322513002/diff/1/chrome/browser/ui/ash/chrom...
File chrome/browser/ui/ash/chrome_keyboard_ui.cc (right):

https://codereview.chromium.org/2322513002/diff/1/chrome/browser/ui/ash/chrom...
chrome/browser/ui/ash/chrome_keyboard_ui.cc:190: return true;
On 2016/09/07 17:23:58, Charlie Reis wrote:
> I would suggest putting the null check in KeyboardUIContent::InitInsets
instead.
>  There's another caller of ShouldWindowOverscroll which passes
> GetContainerWindow instead of GetNativeView, and we wouldn't want to paper
over
> a bug there if there was one.

Done.

I also added a check in UpdateInsetsForWindow(), which is also a caller of
ShouldWindowOverscroll(), even though it seems less likely to be directly called
with a window* obtained from GetNativeView().

[wjmaclean, 2016-09-07 17:41:11]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 17:41:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-09-07 17:48:56]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2016-09-07 17:48:56]

On a side-note: do we want iframes to do this kind of scroll? or should only the
main page do that?

[wjmaclean, 2016-09-07 18:06:00]

On 2016/09/07 17:48:56, sadrul wrote:
> On a side-note: do we want iframes to do this kind of scroll? or should only
the
> main page do that?

There is another CL in-flight that is adding the ability for
RWHVCF::GetNativeView() to return a meaningful value if it is connected via a
CrossProcessFrameConnector, so once that lands any calls to
RWHVCF::GetNativeView() probably indicate that the associated iframe has no
content to scroll anyways. So I don't think this is an issue, but I'm not 100%
sure. I'm guessing this mostly happens during initialization anyways, i.e. it
might be mostly a startup issue.

Thanks for your comments offline, as I didn't realize this was related to the
on-screen keyboard ... it may be we'll need enable the onscreen keyboard to
reproduce.

[wjmaclean, 2016-09-07 18:24:25]

On 2016/09/07 18:06:00, wjmaclean wrote:
> On 2016/09/07 17:48:56, sadrul wrote:
> > On a side-note: do we want iframes to do this kind of scroll? or should only
> the
> > main page do that?
> 
> There is another CL in-flight that is adding the ability for
> RWHVCF::GetNativeView() to return a meaningful value if it is connected via a
> CrossProcessFrameConnector, so once that lands any calls to
> RWHVCF::GetNativeView() probably indicate that the associated iframe has no
> content to scroll anyways. So I don't think this is an issue, but I'm not 100%
> sure. I'm guessing this mostly happens during initialization anyways, i.e. it
> might be mostly a startup issue.
> 
> Thanks for your comments offline, as I didn't realize this was related to the
> on-screen keyboard ... it may be we'll need enable the onscreen keyboard to
> reproduce.

So I tried this on a CrOS build with the onscreen keyboard enabled, using the
Hangouts extension. I haven't been able to repro the crash yet, but I did notice
that we get

[10712:10712:0907/141826:ERROR:render_widget_host_view_base.cc(355)] Not
implemented reached in virtual void
content::RenderWidgetHostViewBase::SetInsets(const gfx::Insets &)

when I invoke the keyboard from within the extension; I suspect this means we
should be implementing SetInsets() for RenderWidgetHostViewChildFrame? There's
no support for insets here at all, though I suspect that it wouldn't be hard to
add, based on what I'm seeing in RenderWidgetHostViewAura.

[commit-bot: I haz the power, 2016-09-07 18:35:20]

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

[commit-bot: I haz the power, 2016-09-07 18:35:23]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2016-09-07 18:36:59]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 18:37:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 19:32:42]

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

[commit-bot: I haz the power, 2016-09-07 19:32:43]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-09-07 20:27:16]

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
File ui/keyboard/content/keyboard_ui_content.cc (right):

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
ui/keyboard/content/keyboard_ui_content.cc:168: return;
I think we probably shouldn't include this change.  There's no crashes that come
through this path (via OnWindowBoundsChanged), and it's not clear to me if it's
possible to crash this way.

In the interest of having a minimal change to merge, we should probably stick to
the InitInsets case below.

(All the non-InitInsets crashes come via ui::GestureRecognizerImpl, which is
likely a separate, pre-existing bug.)

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
ui/keyboard/content/keyboard_ui_content.cc:251: return;
This needs to be a continue and not an early return, right?  We still want to
iterate over the rest of them.

[wjmaclean, 2016-09-07 20:40:49]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2016-09-07 20:40:57]

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
File ui/keyboard/content/keyboard_ui_content.cc (right):

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
ui/keyboard/content/keyboard_ui_content.cc:168: return;
On 2016/09/07 20:27:16, Charlie Reis wrote:
> I think we probably shouldn't include this change.  There's no crashes that
come
> through this path (via OnWindowBoundsChanged), and it's not clear to me if
it's
> possible to crash this way.
> 
> In the interest of having a minimal change to merge, we should probably stick
to
> the InitInsets case below.
> 
> (All the non-InitInsets crashes come via ui::GestureRecognizerImpl, which is
> likely a separate, pre-existing bug.)

Done.

https://codereview.chromium.org/2322513002/diff/40001/ui/keyboard/content/key...
ui/keyboard/content/keyboard_ui_content.cc:251: return;
On 2016/09/07 20:27:16, Charlie Reis wrote:
> This needs to be a continue and not an early return, right?  We still want to
> iterate over the rest of them.

Ooops, I saw the enclosing if() but missed the while().

Done.

[commit-bot: I haz the power, 2016-09-07 20:41:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-09-07 20:42:21]

Thanks!  LGTM.

[commit-bot: I haz the power, 2016-09-07 21:30:08]

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

[commit-bot: I haz the power, 2016-09-07 21:30:09]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-08 12:45:57]

lgtm

[sadrul, 2016-09-08 14:00:50]

stamp lgtm

[wjmaclean, 2016-09-08 14:30:15]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2016-09-08 14:30:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 14:34:17]

Description was changed from

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
==========

to

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
==========

[commit-bot: I haz the power, 2016-09-08 14:34:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 14:36:01]

Description was changed from

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726
==========

to

==========
Check for null return from GetNativeView().

At present many (all?) consumers of
RenderWidgetHostViewBase::GetNativeView() assume it always returns non-
null, but this may not be a safe assumption if the view is a
RenderWidgetHostViewChildFrame. This CL temporarily fixes a crash when
ChromeOS hits this case, while we review the behaviour of the consumers
of this function, and while we review the implementation in
RenderWidgetHostViewChildFrame.

BUG=644294, 644726

Committed: https://crrev.com/079861f265916d3b9d6323a2f4cf07cdba864a05
Cr-Commit-Position: refs/heads/master@{#417288}
==========

[commit-bot: I haz the power, 2016-09-08 14:36:02]

Patchset 4 (id:??) landed as
https://crrev.com/079861f265916d3b9d6323a2f4cf07cdba864a05
Cr-Commit-Position: refs/heads/master@{#417288}