Fix VR Shell crashing when renderer crashes or native page is shown.

Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609
Committed: https://crrev.com/be449ef46e46b55bc295942f857cde909cea8deb
Cr-Commit-Position: refs/heads/master@{#431274}

[mthiesse, 2016-11-02 17:16:09]

mthiesse@chromium.org changed reviewers:
+ bshe@chromium.org

[mthiesse, 2016-11-02 17:16:09]

PTAL

[bshe, 2016-11-02 17:39:08]

On 2016/11/02 17:16:09, mthiesse wrote:
> PTAL

lgtm

[mthiesse, 2016-11-02 17:41:08]

mthiesse@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[mthiesse, 2016-11-02 17:41:08]

+dfalcantara@chromium.org for chrome/android/java OWNERS

[gone, 2016-11-02 17:55:58]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org

[gone, 2016-11-02 17:55:58]

Swapping over to Ted because of that CVC TODO.  He might have a better idea of
what's going on there.

[Ted C, 2016-11-04 20:30:03]

tedchoc@chromium.org changed reviewers:
+ ianwen@chromium.org

[Ted C, 2016-11-04 20:30:04]

+ianwen@ for the TabContentViewParent logic.

https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:185:
// TODO(mthiesse): Investigate why this CVC is getting reparented occasionally,
usually due
A native page does not reparent a CVC.

The CVC's parent/container view will be a ContentView.  The ContentView and the
NativePages are both contained within a TabContentViewParent.  The
TabContentViewParent's lifetime is the length CVC's ownership in a tab.

The CVC in a tab can change when reparenting a tab across activities (i.e.
CustomTabs) or if a prerender occurs and we swap out the webcontents.  This
would happen if you type a URL in the omnibox of something you previously
visited (several times), waited a second, and then clicked on the item.  It is
not 100% reproducible but that does trigger a swap.

TabContentViewParent does remove the ContentView from the view hierarchy if a
native page is being shown, but the parent shouldn't change as a result of that.

[Ian Wen, 2016-11-08 00:40:52]

On 2016/11/04 20:30:04, Ted C wrote:
> +ianwen@ for the TabContentViewParent logic.
> 
>
https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
> File
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
> (right):
> 
>
https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:185:
> // TODO(mthiesse): Investigate why this CVC is getting reparented
occasionally,
> usually due
> A native page does not reparent a CVC.
> 
> The CVC's parent/container view will be a ContentView.  The ContentView and
the
> NativePages are both contained within a TabContentViewParent.  The
> TabContentViewParent's lifetime is the length CVC's ownership in a tab.
> 
> The CVC in a tab can change when reparenting a tab across activities (i.e.
> CustomTabs) or if a prerender occurs and we swap out the webcontents.  This
> would happen if you type a URL in the omnibox of something you previously
> visited (several times), waited a second, and then clicked on the item.  It is
> not 100% reproducible but that does trigger a swap.
> 
> TabContentViewParent does remove the ContentView from the view hierarchy if a
> native page is being shown, but the parent shouldn't change as a result of
that.

It's not super clear to me what restoreContentWindow() is doing. Yet what Ted
said was very accurate.

To echo what Ted said, TabContentViewParent is the parent of both current
nativepage's view as well as the ContentView. ContentView is mostly used to
receive touch event, measure view sizes, etc. And it does not draw anything
itself. ContentView will make itself to be transparent and the surface view
draws this transparent area.

CVC of a tab can be swapped during prerender or tab restore. Please refer to
Tab#swapWebContents().

[mthiesse, 2016-11-08 23:03:17]

PTAL Ted

https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2464373005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:185:
// TODO(mthiesse): Investigate why this CVC is getting reparented occasionally,
usually due
On 2016/11/04 20:30:04, Ted C wrote:
> A native page does not reparent a CVC.
> 
> The CVC's parent/container view will be a ContentView.  The ContentView and
the
> NativePages are both contained within a TabContentViewParent.  The
> TabContentViewParent's lifetime is the length CVC's ownership in a tab.
> 
> The CVC in a tab can change when reparenting a tab across activities (i.e.
> CustomTabs) or if a prerender occurs and we swap out the webcontents.  This
> would happen if you type a URL in the omnibox of something you previously
> visited (several times), waited a second, and then clicked on the item.  It is
> not 100% reproducible but that does trigger a swap.
> 
> TabContentViewParent does remove the ContentView from the view hierarchy if a
> native page is being shown, but the parent shouldn't change as a result of
that.

Thanks for the pointers. The issue seems to be
TabContentViewParent#onContentChanged, which removes the ContentView from its
parent. I've worked around this by reparenting the TabContentViewParent, rather
than the ContentView.

[Ted C, 2016-11-09 19:57:53]

https://codereview.chromium.org/2464373005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2464373005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:169:
mOriginalCVParentParent = ((ViewGroup)
contentContentView.getParent().getParent());
if instead of operating on the parent chain of the CVC, what happens if you were
to do mTab.getView() and reparent that way?

I think that is the view you want to be operating on so maybe that is the best
thing to do now?

[mthiesse, 2016-11-09 23:06:00]

Description was changed from

==========
Fix VR Shell crashing when renderer crashes.

BUG=661609
==========

to

==========
Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609
==========

[mthiesse, 2016-11-09 23:09:38]

I've added an onContentChanged listener now so that we exit VR when a native
page shows up, or a renderer crashes, so we don't get into a weird state. In the
future when we actually support native pages in VR we'll stay in VR.

https://codereview.chromium.org/2464373005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java
(right):

https://codereview.chromium.org/2464373005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:169:
mOriginalCVParentParent = ((ViewGroup)
contentContentView.getParent().getParent());
On 2016/11/09 19:57:53, Ted C wrote:
> if instead of operating on the parent chain of the CVC, what happens if you
were
> to do mTab.getView() and reparent that way?
> 
> I think that is the view you want to be operating on so maybe that is the best
> thing to do now?

Yes, that seems fine, but mTab.getView() changes on content changing, so I still
need to cache it. I also added a listener for the tab content changing, so that
we exit VR instead of getting into a weird state when a native page is shown.

[Ted C, 2016-11-10 01:15:53]

lgtm

https://codereview.chromium.org/2464373005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2464373005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:54:
private final TabObserver mTabObserver;
I try to put final variables at the top in terms of ordering.

[mthiesse, 2016-11-10 15:51:46]

https://codereview.chromium.org/2464373005/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
(right):

https://codereview.chromium.org/2464373005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:54:
private final TabObserver mTabObserver;
On 2016/11/10 01:15:53, Ted C wrote:
> I try to put final variables at the top in terms of ordering.

Done.

[mthiesse, 2016-11-10 15:53:03]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-11-10 15:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org,
tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2464373005/#ps60001
(title: "Rebase + address comment")

[commit-bot: I haz the power, 2016-11-10 15:53:20]

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-11-10 16:39:16]

Description was changed from

==========
Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609
==========

to

==========
Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609
==========

[commit-bot: I haz the power, 2016-11-10 16:39:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-10 16:41:02]

Description was changed from

==========
Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609
==========

to

==========
Fix VR Shell crashing when renderer crashes or native page is shown.

BUG=661609

Committed: https://crrev.com/be449ef46e46b55bc295942f857cde909cea8deb
Cr-Commit-Position: refs/heads/master@{#431274}
==========

[commit-bot: I haz the power, 2016-11-10 16:41:04]

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