Android WebView: Crash on compositor context loss

Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b354724cc217e862aba2797f92f15af564c37ba3
Cr-Commit-Position: refs/heads/master@{#367371}

[boliu, 2015-11-13 04:13:48]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-11-13 04:13:49]

wdyt?

[boliu, 2015-11-13 04:13:59]

Description was changed from

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[no sievers, 2015-11-13 18:24:29]

On 2015/11/13 04:13:49, boliu wrote:
> wdyt?


For the parent compositor:

The View framework should deal with context loss and in some future draw we
should have a working context again.

So that being said, there are a couple of reasons for which we would mark the
context as lost:
- MakeCurrent fails (doesn't apply)
- SwapBuffers fails (doesn't apply)
- GL_OUT_OF_MEMORY, but only if the |lose_context_when_out_of_memory| attrib is
set:
maybe we should set this to false
- parse errors: These would be unexpected and maybe a crash report would be nice
here
- GLES2DecoderImpl::CheckResetStatus() (doesn't apply since
GLContext::WasAllocatedUsingRobustnessExtension() is false)

For the child/renderer compositor:

Now that this runs on its own thread, we can handle context lost, right?

[boliu, 2015-11-13 18:29:00]

On 2015/11/13 18:24:29, sievers wrote:
> On 2015/11/13 04:13:49, boliu wrote:
> > wdyt?
> 
> 
> For the parent compositor:
> 
> The View framework should deal with context loss and in some future draw we
> should have a working context again.
> 
> So that being said, there are a couple of reasons for which we would mark the
> context as lost:
> - MakeCurrent fails (doesn't apply)
> - SwapBuffers fails (doesn't apply)
> - GL_OUT_OF_MEMORY, but only if the |lose_context_when_out_of_memory| attrib
is
> set:
> maybe we should set this to false
> - parse errors: These would be unexpected and maybe a crash report would be
nice
> here

I've seen this. Don't remember if it was the parent or gpu thread context
though.

> - GLES2DecoderImpl::CheckResetStatus() (doesn't apply since
> GLContext::WasAllocatedUsingRobustnessExtension() is false)
> 
> For the child/renderer compositor:
> 
> Now that this runs on its own thread, we can handle context lost, right?

Yes. But until we do, having a consistent crash stack is better for crash
reporting.

[no sievers, 2015-11-13 19:17:19]

On 2015/11/13 18:29:00, boliu wrote:
> On 2015/11/13 18:24:29, sievers wrote:
> > On 2015/11/13 04:13:49, boliu wrote:
> > > wdyt?
> > 
> > 
> > For the parent compositor:
> > 
> > The View framework should deal with context loss and in some future draw we
> > should have a working context again.
> > 
> > So that being said, there are a couple of reasons for which we would mark
the
> > context as lost:
> > - MakeCurrent fails (doesn't apply)
> > - SwapBuffers fails (doesn't apply)
> > - GL_OUT_OF_MEMORY, but only if the |lose_context_when_out_of_memory| attrib
> is
> > set:
> > maybe we should set this to false
> > - parse errors: These would be unexpected and maybe a crash report would be
> nice
> > here
> 
> I've seen this. Don't remember if it was the parent or gpu thread context
> though.
> 

Ok crash reports would probably be helpful here then to gather more data.
GL_OUT_OF_MEMORY though is maybe something we would not want to cause a crash.


> > - GLES2DecoderImpl::CheckResetStatus() (doesn't apply since
> > GLContext::WasAllocatedUsingRobustnessExtension() is false)
> > 
> > For the child/renderer compositor:
> > 
> > Now that this runs on its own thread, we can handle context lost, right?
> 
> Yes. But until we do, having a consistent crash stack is better for crash
> reporting.

Sounds fine. Agreed that it's probably better to report a crash than have broken
apps that silently don't render anymore.
But it would be good to understand the current behavior exactly to make sure we
don't regress esp. with context virtualization and misbehaving webgl contexts
and what not.

[boliu, 2015-11-18 23:40:31]

PS2 as discussed in-person

[no sievers, 2015-11-18 23:50:20]

lgtm

https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
File android_webview/browser/aw_render_thread_context_provider.cc (right):

https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
android_webview/browser/aw_render_thread_context_provider.cc:62:
attribs_for_gles2.lose_context_when_out_of_memory = false;
nit: maybe put a comment that this is not our context but the framework's, so
not up to us to decide when to lose it

speaking of which: do we handle the framework allocating a new context?

[boliu, 2015-11-18 23:55:23]

On 2015/11/18 23:50:20, sievers wrote:
> lgtm
> 
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> File android_webview/browser/aw_render_thread_context_provider.cc (right):
> 
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> android_webview/browser/aw_render_thread_context_provider.cc:62:
> attribs_for_gles2.lose_context_when_out_of_memory = false;
> nit: maybe put a comment that this is not our context but the framework's, so
> not up to us to decide when to lose it
> 
> speaking of which: do we handle the framework allocating a new context?

Yes under some circumstances. There is no proper contract between webview and
framework on what should happen when context is unexpectedly lost though, so
that case is not handled

[no sievers, 2015-11-19 00:00:20]

On 2015/11/18 23:55:23, boliu wrote:
> On 2015/11/18 23:50:20, sievers wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> > File android_webview/browser/aw_render_thread_context_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> > android_webview/browser/aw_render_thread_context_provider.cc:62:
> > attribs_for_gles2.lose_context_when_out_of_memory = false;
> > nit: maybe put a comment that this is not our context but the framework's,
so
> > not up to us to decide when to lose it
> > 
> > speaking of which: do we handle the framework allocating a new context?
> 
> Yes under some circumstances. There is no proper contract between webview and
> framework on what should happen when context is unexpectedly lost though, so
> that case is not handled

But there is no contract that the context will stay the exact same context
either, right?
I think we should just make sure we handle context changes and reallocation.

[boliu, 2015-11-19 00:02:31]

On 2015/11/19 00:00:20, sievers wrote:
> On 2015/11/18 23:55:23, boliu wrote:
> > On 2015/11/18 23:50:20, sievers wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> > > File android_webview/browser/aw_render_thread_context_provider.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1441273002/diff/20001/android_webview/browser...
> > > android_webview/browser/aw_render_thread_context_provider.cc:62:
> > > attribs_for_gles2.lose_context_when_out_of_memory = false;
> > > nit: maybe put a comment that this is not our context but the framework's,
> so
> > > not up to us to decide when to lose it
> > > 
> > > speaking of which: do we handle the framework allocating a new context?
> > 
> > Yes under some circumstances. There is no proper contract between webview
and
> > framework on what should happen when context is unexpectedly lost though, so
> > that case is not handled
> 
> But there is no contract that the context will stay the exact same context
> either, right?

We kinda do implicitly..

> I think we should just make sure we handle context changes and reallocation.

[boliu, 2015-11-19 00:23:52]

boliu@chromium.org changed reviewers:
+ vmpstr@chromium.org

[boliu, 2015-11-19 00:23:52]

+vmpstr for cc

[boliu, 2015-11-23 19:36:44]

vmpstr: mild ping

[vmpstr, 2016-01-04 18:55:34]

cc lgtm, sorry for the delay

[boliu, 2016-01-04 20:11:09]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-01-04 20:11:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1441273002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-04 20:12:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1441273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1441273002/40001

[commit-bot: I haz the power, 2016-01-04 21:24:32]

Description was changed from

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-04 21:24:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-04 21:25:13]

Description was changed from

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Android WebView: Crash on compositor context loss

Android webview does not, and in some cases cannot correctly handle
context loss. Explicitly crash on context loss so they generate
consistent crash stacks.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-01-04 21:25:14]

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