[Android] Speculative fix for crash during overscroll effect creation

[Android] Speculative fix for crash during overscroll effect creation

Crash reports indicate that the WindowAndroid's Compositor may be NULL
during |RenderWidgetHostViewAndroid::SetContentViewCore()|. While this
is a somewhat surprising result, it is in theory possible and should
probably not be fatal. Avoid creating the overscroll effect in this
case, instead creating it after the compositor is (re-)attached.

BUG=407208
Committed: https://crrev.com/7936233f8fb1bc6a969b24474d0f32d03dd832af
Cr-Commit-Position: refs/heads/master@{#291835}

[jdduke (slow), 2014-08-25 19:57:36]

aelias@: I can't repro this issue, and I'm not sure what else to do short of
performing the NULL checks and returning.  We have other DCHECKs for
GetWindowAndroid()/GetCompositor() at other, critical parts of the pipeline, so
I'm inclined to move forward with this for M38 (looks like the #2 crash).

[aelias_OOO_until_Jul13, 2014-08-25 20:06:49]

Hmm, strictly speaking we should also create the overscroll effect when the
Compositor is attached to the ContentViewCore, if it wasn't created here.  Less
harmful than the crash, but... in general with this kind of issue it's better to
get to the bottom of it.  Daniel, do you understand what scenario this might
happen in?

[jdduke (slow), 2014-08-25 20:11:35]

On 2014/08/25 20:06:49, aelias wrote:
> Hmm, strictly speaking we should also create the overscroll effect when the
> Compositor is attached to the ContentViewCore, if it wasn't created here. 
Less
> harmful than the crash, but... in general with this kind of issue it's better
to
> get to the bottom of it.  Daniel, do you understand what scenario this might
> happen in?

Hmm, I guess I could listen to |On{Attach|Detach}Compositor()|, sievers@ do you
have any thoughts here?

[no sievers, 2014-08-25 20:37:37]

https://codereview.chromium.org/503813003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/503813003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1637:
content_view_core_->GetWindowAndroid() &&
 content_view_core_->GetWindowAndroid() == NULL seems like an unexpected bug,
see content_view_core_impl.cc:

ContentViewCoreImpl::ContentViewCoreImpl()
...
DCHECK(window_android_);

and it cannot change.

[jdduke (slow), 2014-08-25 20:40:04]

https://codereview.chromium.org/503813003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/503813003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_android.cc:1637:
content_view_core_->GetWindowAndroid() &&
On 2014/08/25 20:37:36, sievers wrote:
>  content_view_core_->GetWindowAndroid() == NULL seems like an unexpected bug,
> see content_view_core_impl.cc:
> 
> ContentViewCoreImpl::ContentViewCoreImpl()
> ...
> DCHECK(window_android_);
> 
> and it cannot change.

I figured, so I'll check for a null compositor here, then in
|OnAttachCompositor()| recreate the effect if necessary?

[jdduke (slow), 2014-08-25 21:23:55]

OK, per offline discussion with sievers@, we think this may be a case of racy
shutdown access to the compositor. I've made it so we check for a NULL
compositor (still assuming validity of WindowAndroid), but we also listen to
OnCompositorAttached for completeness.

[aelias_OOO_until_Jul13, 2014-08-25 21:45:52]

On 2014/08/25 21:23:55, jdduke wrote:
> OK, per offline discussion with sievers@, we think this may be a case of racy
> shutdown access to the compositor. I've made it so we check for a NULL
> compositor (still assuming validity of WindowAndroid), but we also listen to
> OnCompositorAttached for completeness.

OK.  I think the !overscroll_effect_ branch should either be in both places or
neither.

[no sievers, 2014-08-25 21:48:52]

lgtm

https://codereview.chromium.org/503813003/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/503813003/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:1660: void
RenderWidgetHostViewAndroid::OnAttachCompositor() {
I just realized that these callbacks are a bit flaky anyways, since we call
RemoveObserver() when the view is hidden.

[jdduke (slow), 2014-08-25 22:55:21]

On 2014/08/25 21:45:52, aelias wrote:
> On 2014/08/25 21:23:55, jdduke wrote:
> > OK, per offline discussion with sievers@, we think this may be a case of
racy
> > shutdown access to the compositor. I've made it so we check for a NULL
> > compositor (still assuming validity of WindowAndroid), but we also listen to
> > OnCompositorAttached for completeness.
> 
> OK.  I think the !overscroll_effect_ branch should either be in both places or
> neither.

Done (the former).

[jdduke (slow), 2014-08-25 22:55:31]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:56:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/503813003/40001

[commit-bot: I haz the power, 2014-08-26 01:51:32]

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

[commit-bot: I haz the power, 2014-08-26 01:51:33]

Failed to commit the patch.

[jdduke (slow), 2014-08-26 02:13:29]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-26 02:15:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/503813003/40001

[commit-bot: I haz the power, 2014-08-26 02:17:15]

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

[commit-bot: I haz the power, 2014-08-26 02:17:16]

Failed to commit the patch.

[jdduke (slow), 2014-08-26 04:10:06]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-08-26 04:10:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jdduke@chromium.org/503813003/40001

[commit-bot: I haz the power, 2014-08-26 04:20:48]

Committed patchset #3 (40001) as 3965c72849fd5d92f22df447bd2688aff9162e00

[commit-bot: I haz the power, 2014-09-10 02:40:19]

Patchset 3 (id:??) landed as
https://crrev.com/7936233f8fb1bc6a969b24474d0f32d03dd832af
Cr-Commit-Position: refs/heads/master@{#291835}