android: Fix ContentViewCore and RenderWidgetHostViewAndroid lifetime issue

android: Fix ContentViewCore and RenderWidgetHostViewAndroid lifetime issue

https://codereview.chromium.org/26753005/ might have introduced crashes that
trace to SetContentViewCore.  That previous patch assumed that ContentViewCore
(CVC) always outlives its associated RenderWidgetHostView.  In this patch, we
are trying to ensure that the RWHVA pointer to CVC is nulled when CVC is
destroyed.

Note the following:
- WebContents owns ContentViewCore and WebContentsView.
- CVC and WebContentsView are implicitly destroyed (no direct call to clean-up
code).  So ContentViewCore cannot refer to WebContents or WebContentsView in
its destructor.

Therefore, we use the observer of when WebContents is destroyed to start the
clean up on CVC.


BUG=335165, 324341
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247878

[powei, 2014-01-21 20:21:37]

ptal. Thanks!

[powei, 2014-01-23 01:08:22]

ping

[powei, 2014-01-23 01:08:46]

+fqian@

[Ted C, 2014-01-23 01:28:42]

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1682: void
ContentViewCoreImpl::OnDetachedFromWebContents(WebContents* web_contents) {
Looking more at this, why don't you do this in ContentViewCoreImpl's destructor?

ContentViewCoreImpl is owned by the WebContents and will deleted when it is.  If
you look at ContentViewCoreImpl::ContentViewUserData, that sets up the
relationship and should be ensuring this is deleted before the web contents.

Maybe that would simplify this a bit?

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1686:
static_cast<WebContentsViewAndroid*>(GetWebContents()->GetView());
+2

[powei, 2014-01-23 01:38:02]

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1682: void
ContentViewCoreImpl::OnDetachedFromWebContents(WebContents* web_contents) {
On 2014/01/23 01:28:42, Ted C wrote:
> Looking more at this, why don't you do this in ContentViewCoreImpl's
destructor?
> 
> ContentViewCoreImpl is owned by the WebContents and will deleted when it is. 
If
> you look at ContentViewCoreImpl::ContentViewUserData, that sets up the
> relationship and should be ensuring this is deleted before the web contents.
> 
> Maybe that would simplify this a bit?

My first instinct was to put it in the destructor as well, but that did not
work.

I think the order is that the destructor of WebContentsImpl is called, then
ContentViewUserData goes out of scope and CVC gets deleted.

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1686:
static_cast<WebContentsViewAndroid*>(GetWebContents()->GetView());
On 2014/01/23 01:28:42, Ted C wrote:
> +2

Done.

[Ted C, 2014-01-23 02:00:15]

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1682: void
ContentViewCoreImpl::OnDetachedFromWebContents(WebContents* web_contents) {
On 2014/01/23 01:38:02, powei wrote:
> On 2014/01/23 01:28:42, Ted C wrote:
> > Looking more at this, why don't you do this in ContentViewCoreImpl's
> destructor?
> > 
> > ContentViewCoreImpl is owned by the WebContents and will deleted when it is.

> If
> > you look at ContentViewCoreImpl::ContentViewUserData, that sets up the
> > relationship and should be ensuring this is deleted before the web contents.
> > 
> > Maybe that would simplify this a bit?
> 
> My first instinct was to put it in the destructor as well, but that did not
> work.
> 
> I think the order is that the destructor of WebContentsImpl is called, then
> ContentViewUserData goes out of scope and CVC gets deleted.  

Hmm...that is unfortunate.  If that is the case, I'd rather make this class (or
even WebContentsViewAndroid) a WebContentsObserver instead of piggy backing on
the java bridge class.  There is actually no requirement that this class have a
WebContentsObserverAndroid attached to it (granted they always do), but I think
it would be cleaner for one of these two classes to maintain the state
internally instead of relying on a hidden property.

[powei, 2014-01-23 02:38:40]

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:1682: void
ContentViewCoreImpl::OnDetachedFromWebContents(WebContents* web_contents) {
On 2014/01/23 02:00:15, Ted C wrote:
> On 2014/01/23 01:38:02, powei wrote:
> > On 2014/01/23 01:28:42, Ted C wrote:
> > > Looking more at this, why don't you do this in ContentViewCoreImpl's
> > destructor?
> > > 
> > > ContentViewCoreImpl is owned by the WebContents and will deleted when it
is.
> 
> > If
> > > you look at ContentViewCoreImpl::ContentViewUserData, that sets up the
> > > relationship and should be ensuring this is deleted before the web
contents.
> > > 
> > > Maybe that would simplify this a bit?
> > 
> > My first instinct was to put it in the destructor as well, but that did not
> > work.
> > 
> > I think the order is that the destructor of WebContentsImpl is called, then
> > ContentViewUserData goes out of scope and CVC gets deleted.  
> 
> Hmm...that is unfortunate.  If that is the case, I'd rather make this class
(or
> even WebContentsViewAndroid) a WebContentsObserver instead of piggy backing on
> the java bridge class.  There is actually no requirement that this class have
a
> WebContentsObserverAndroid attached to it (granted they always do), but I
think
> it would be cleaner for one of these two classes to maintain the state
> internally instead of relying on a hidden property.

Done.  Turns out ContentViewCoreImpl is already a WebContentsObserver.  Color me
embarrassed.

[Ted C, 2014-01-23 02:39:38]

On 2014/01/23 02:38:40, powei wrote:
>
https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/133073012/diff/50001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:1682: void
> ContentViewCoreImpl::OnDetachedFromWebContents(WebContents* web_contents) {
> On 2014/01/23 02:00:15, Ted C wrote:
> > On 2014/01/23 01:38:02, powei wrote:
> > > On 2014/01/23 01:28:42, Ted C wrote:
> > > > Looking more at this, why don't you do this in ContentViewCoreImpl's
> > > destructor?
> > > > 
> > > > ContentViewCoreImpl is owned by the WebContents and will deleted when it
> is.
> > 
> > > If
> > > > you look at ContentViewCoreImpl::ContentViewUserData, that sets up the
> > > > relationship and should be ensuring this is deleted before the web
> contents.
> > > > 
> > > > Maybe that would simplify this a bit?
> > > 
> > > My first instinct was to put it in the destructor as well, but that did
not
> > > work.
> > > 
> > > I think the order is that the destructor of WebContentsImpl is called,
then
> > > ContentViewUserData goes out of scope and CVC gets deleted.  
> > 
> > Hmm...that is unfortunate.  If that is the case, I'd rather make this class
> (or
> > even WebContentsViewAndroid) a WebContentsObserver instead of piggy backing
on
> > the java bridge class.  There is actually no requirement that this class
have
> a
> > WebContentsObserverAndroid attached to it (granted they always do), but I
> think
> > it would be cleaner for one of these two classes to maintain the state
> > internally instead of relying on a hidden property.
> 
> Done.  Turns out ContentViewCoreImpl is already a WebContentsObserver.  Color
me
> embarrassed.

lgtm (but I'd like sievers to take a look and make sure i'm not being crazy)

[powei, 2014-01-28 02:27:12]

ping sievers@.  Would really appreciate you taking a look.  Thanks!

[Feng Qian, 2014-01-28 16:03:33]

On 2014/01/28 02:27:12, powei wrote:
> ping sievers@.  Would really appreciate you taking a look.  Thanks!

Powei, the last patch set misses some files. Did you forget to include them?

[powei, 2014-01-28 18:42:55]

On 2014/01/28 16:03:33, Feng Qian wrote:
> On 2014/01/28 02:27:12, powei wrote:
> > ping sievers@.  Would really appreciate you taking a look.  Thanks!
> 
> Powei, the last patch set misses some files. Did you forget to include them?

Not quite.  Ted pointed out that ContentViewCoreImpl is a WebContentsObserver,
which allowed me to simplify the solution by just writing an override function
in CVCImpl.

[powei, 2014-01-30 00:35:42]

Offline ok from sievers@.  Landing, thanks all.

[commit-bot: I haz the power, 2014-01-30 00:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/133073012/180001

[commit-bot: I haz the power, 2014-01-30 07:43:42]

Change committed as 247878