Fix for crash in testWindows test

Fix for crash in testWindows test

Avoid using Web Contents after it maybe deleted.

BUG=b/8341990

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

[Leandro Graciá Gil, 2013-03-08 11:46:56]

https://codereview.chromium.org/12668003/diff/1/android_webview/browser/brows...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12668003/diff/1/android_webview/browser/brows...
android_webview/browser/browser_view_renderer_impl.cc:161: // TODO(joth):
Allowing a NULL param is probably not needed, see b/8341990
The destructor is calling with a NULL param to release any previously registered
callback. If I remember correctly this was actually one of your review
suggestions. Also, this workaround would prevent removing any frame metadata
update callbacks from old ContentViewCores. Are you sure this is the right way
to do this?

[joth, 2013-03-08 18:15:56]

On 8 March 2013 03:46, <leandrogracia@chromium.org> wrote:

>
> https://codereview.chromium.**org/12668003/diff/1/android_**
>
webview/browser/browser_view_**renderer_impl.cc<https://codereview.chromium.org/12668003/diff/1/android_webview/browser/browser_view_renderer_impl.cc>
> File android_webview/browser/**browser_view_renderer_impl.cc (right):
>
> https://codereview.chromium.**org/12668003/diff/1/android_**
>
webview/browser/browser_view_**renderer_impl.cc#newcode161<https://codereview.chromium.org/12668003/diff/1/android_webview/browser/browser_view_renderer_impl.cc#newcode161>
> android_webview/browser/**browser_view_renderer_impl.cc:**161: //
>
> TODO(joth): Allowing a NULL param is probably not needed, see b/8341990
> The destructor is calling with a NULL param to release any previously
> registered callback. If I remember correctly this was actually one of
> your review suggestions.


yeah, I was making the suggestion in the context of having a dedicated
ContentViewCoreObserver interface, in the same pattern as
WebContentsObsever, which supports mutual deregistration. i.e., it doesn't
matter which of WebContents or WebContentsObsever get deleted first, it
will automatically do the Right Thing.

With a callback this isn't really possible -- we need a second callback to
indicate "this ContentViewCore is going away", or make one connection into
a weak reference.




> Also, this workaround would prevent removing
> any frame metadata update callbacks from old ContentViewCores. Are you
> sure this is the right way to do this?
>
>
We have two choices: use callbacks and cover heavily with comments about
the relative life-times of objects and correct destruction order (i.e. as
your patch originally existed before I got involved!) or use full-scale
observer with mutual de-registration (ContentViewCoreObserver or weak
references)

[Leandro Graciá Gil, 2013-03-08 18:23:28]

On 2013/03/08 18:15:56, joth wrote:
> On 8 March 2013 03:46, <mailto:leandrogracia@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/12668003/diff/1/android_**
> >
>
webview/browser/browser_view_**renderer_impl.cc<https://codereview.chromium.org/12668003/diff/1/android_webview/browser/browser_view_renderer_impl.cc>
> > File android_webview/browser/**browser_view_renderer_impl.cc (right):
> >
> > https://codereview.chromium.**org/12668003/diff/1/android_**
> >
>
webview/browser/browser_view_**renderer_impl.cc#newcode161<https://codereview.chromium.org/12668003/diff/1/android_webview/browser/browser_view_renderer_impl.cc#newcode161>
> > android_webview/browser/**browser_view_renderer_impl.cc:**161: //
> >
> > TODO(joth): Allowing a NULL param is probably not needed, see b/8341990
> > The destructor is calling with a NULL param to release any previously
> > registered callback. If I remember correctly this was actually one of
> > your review suggestions.
> 
> 
> yeah, I was making the suggestion in the context of having a dedicated
> ContentViewCoreObserver interface, in the same pattern as
> WebContentsObsever, which supports mutual deregistration. i.e., it doesn't
> matter which of WebContents or WebContentsObsever get deleted first, it
> will automatically do the Right Thing.
> 
> With a callback this isn't really possible -- we need a second callback to
> indicate "this ContentViewCore is going away", or make one connection into
> a weak reference.
> 
> 
> 
> 
> > Also, this workaround would prevent removing
> > any frame metadata update callbacks from old ContentViewCores. Are you
> > sure this is the right way to do this?
> >
> >
> We have two choices: use callbacks and cover heavily with comments about
> the relative life-times of objects and correct destruction order (i.e. as
> your patch originally existed before I got involved!) or use full-scale
> observer with mutual de-registration (ContentViewCoreObserver or weak
> references)

In that case, feel free to go for the option you think would be most
appropriate. I just wanted to make sure we were keeping the de-registration
point in mind.

[joth, 2013-03-09 03:11:30]

OK I added the mutual deregistration, as adding UserData looks like it will be
useful for mikhail's patch (https://codereview.chromium.org/12697002/) too.

PTAL

[Leandro Graciá Gil, 2013-03-11 12:07:53]

LGTM, just one question.

https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:216: web_contents_ = NULL;
Should we un-observe in view_renderer_host_?

[joth, 2013-03-11 16:18:31]

https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
File android_webview/browser/browser_view_renderer_impl.cc (right):

https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
android_webview/browser/browser_view_renderer_impl.cc:216: web_contents_ = NULL;
On 2013/03/11 12:07:54, Leandro Graciá Gil wrote:
> Should we un-observe in view_renderer_host_?

No, not really. view_renderer_host_ is a WebContentsObserver so it automatically
gets unobserved when the WebContents departs.
My instinct is this would all be clearer if we hold onto content_view_core_ as a
member instead of web contents, as that's what's set as our input, but that's
going to be a bigger change again so I'm leaving it out for now (as this is a 
flaky test fixer)

[commit-bot: I haz the power, 2013-03-11 16:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/12668003/11001

[Leandro Graciá Gil, 2013-03-11 16:31:20]

On 2013/03/11 16:18:31, joth wrote:
>
https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
> File android_webview/browser/browser_view_renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/12668003/diff/11001/android_webview/browser/b...
> android_webview/browser/browser_view_renderer_impl.cc:216: web_contents_ =
NULL;
> On 2013/03/11 12:07:54, Leandro Graciá Gil wrote:
> > Should we un-observe in view_renderer_host_?
> 
> No, not really. view_renderer_host_ is a WebContentsObserver so it
automatically
> gets unobserved when the WebContents departs.
> My instinct is this would all be clearer if we hold onto content_view_core_ as
a
> member instead of web contents, as that's what's set as our input, but that's
> going to be a bigger change again so I'm leaving it out for now (as this is a 
> flaky test fixer)

Actually, once we migrate the SW invalidation path we'll probably find more
reasons to hold on ContentViewCore rather than WebContents: we shouldn't need to
verify the process/render id anymore as invalidation would come via
ContentViewCore too.

Migrating the SW invalidation path is currently blocked on crbug.com/176947. I'm
looking into providing a workaround that would also be useful to determine when
exactly to call PictureListeners.

[commit-bot: I haz the power, 2013-03-11 18:13:10]

Change committed as 187319