Android webview fix gpu crashes after destroy

Android webview fix gpu crashes after destroy

These are some fixes to handle corner cases and races uncovered with some
Android WebView stress tests that very quickly create and destroy views.

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

[boliu, 2013-01-25 18:13:22]

Hey Daniel, this is my first pass at fixing crashes I saw while running my test.
With this patch, that test passes 100/100, but still more potential crash points
that is not discovered by the test, and I don't know if any of these fixes are
correct. Over to you for first pass, and I'll add a webview test explicitly for
stressing Create/Destroy.

[no sievers, 2013-01-25 20:24:40]

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/gpu_comman...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/gpu_comman...
content/common/gpu/gpu_command_buffer_stub.cc:429: LOG(ERROR) << "Failed to
initialize virtual GL context.";
I don't really like removing the LOG(FATAL). I think we want to make sure that
on a platform that uses virtual contexts, it always works to create a virtual
context, which basically comes down to being able to make different surfaces
current with the same context (there's a known issue on Linux to make this work
at least).
It's scary to allow a mix of contexts, where we sometimes fail to share a real
context. We then end up not knowing what we are really running and that will
make it hard to deal with share-group related bugs.

Is this change needed if we correctly fail surface creation for which we should
bail out above in line 404? Do you know why this would fail here?

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/image_tran...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/image_tran...
content/common/gpu/image_transport_surface_android.cc:27: return NULL;  // May
happen after AwContents.destroy
Usually I'd think that this implies a bug (such as a race) in the code.
Does this happen because you quickly create and destroy a compositor (removing
the surface from the tracker) before we get a chance to create the context on
the gpu thread?

Wouldn't surface->Initialize() below fail if window was NULL?

The change is fine with me though. But you might want to add a LOG(ERROR) (maybe
also for the next return).

https://codereview.chromium.org/12038095/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/12038095/diff/1/ui/gl/gl_context.cc#newcode125
ui/gl/gl_context.cc:125:
virtual_gl_api_->OnDestroyVirtualContext(virtual_context);
This change looks correct for where GLContextVirtual::Initialize() fails.

But see other comment... how does this happen?

[boliu, 2013-01-25 21:27:01]

The test I've been using to reproduce these crashes is
https://codereview.chromium.org/12038098/diff/1/android_webview/javatests/src...

(These crashes are independent of the threading issue fixed in that patch)

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/gpu_comman...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/gpu_comman...
content/common/gpu/gpu_command_buffer_stub.cc:429: LOG(ERROR) << "Failed to
initialize virtual GL context.";
On 2013/01/25 20:24:41, Daniel Sievers wrote:
> I don't really like removing the LOG(FATAL). I think we want to make sure that
> on a platform that uses virtual contexts, it always works to create a virtual
> context, which basically comes down to being able to make different surfaces
> current with the same context (there's a known issue on Linux to make this
work
> at least).
> It's scary to allow a mix of contexts, where we sometimes fail to share a real
> context. We then end up not knowing what we are really running and that will
> make it hard to deal with share-group related bugs.
> 
> Is this change needed if we correctly fail surface creation for which we
should
> bail out above in line 404? Do you know why this would fail here?

I didn't understand much of this, but sounds like this is the wrong fix. So
sounds like it should fall into line 404? I'll see what I can do. And I'll post
the stack trace for this one to the bug.

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/image_tran...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/12038095/diff/1/content/common/gpu/image_tran...
content/common/gpu/image_transport_surface_android.cc:27: return NULL;  // May
happen after AwContents.destroy
On 2013/01/25 20:24:41, Daniel Sievers wrote:
> Usually I'd think that this implies a bug (such as a race) in the code.
> Does this happen because you quickly create and destroy a compositor (removing
> the surface from the tracker) before we get a chance to create the context on
> the gpu thread?

Yes, the process is create awcontents->load about:blank->destroy, I think the
whole thing is pretty fast.

> 
> Wouldn't surface->Initialize() below fail if window was NULL?

So I'm seeing a null pointer exception in Initialize that's freaking me out
since I basically added DCHECKs for everything before where the crash is and
nothing appeared null. I'll post the stack crash in the bug if I can't figure it
out.
> 
> The change is fine with me though. But you might want to add a LOG(ERROR)
(maybe
> also for the next return).

btw, LOG(ERROR) is equivalent to CHECK(false), did you really want to crash in
production too?

https://codereview.chromium.org/12038095/diff/1/ui/gl/gl_context.cc
File ui/gl/gl_context.cc (right):

https://codereview.chromium.org/12038095/diff/1/ui/gl/gl_context.cc#newcode125
ui/gl/gl_context.cc:125:
virtual_gl_api_->OnDestroyVirtualContext(virtual_context);
On 2013/01/25 20:24:41, Daniel Sievers wrote:
> This change looks correct for where GLContextVirtual::Initialize() fails.
> 
> But see other comment... how does this happen?

Crash stack for this is in the bug.

[boliu, 2013-01-26 01:18:00]

I'm still baffled by the crash in android egl...I tried flashing to jb, but
still happens, suggesting this is chromium code

https://codereview.chromium.org/12038095/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12038095/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/compositor_impl_android.cc:135:
SetWindowSurface(NULL);
This was my attempt at fixing the crash inside android egl library, didn't work
:(

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:435: return;
Daniel...would this be better? I can't get it to fall into line 404...but I
copied the failure code from there...

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/image_t...
File content/common/gpu/image_transport_surface_android.cc (right):

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/image_t...
content/common/gpu/image_transport_surface_android.cc:27: if
(!surface->Initialize())
I moved the DCHECK into Initialize() to make it return false.

Also you just new-ed surface, I don't think it's possible for surface.get() to
be null.

[boliu, 2013-01-26 03:56:08]

I'm pretty sure Patch Set 3 fixes all the errors I saw. (Coded it up before
going home, then left it to run for 1000 iterations. Ended up crashing the
dalvik vm a few times..., but nothing from libwebviewchromium.so). You can
review it on monday.

[no sievers, 2013-01-28 19:46:15]

https://codereview.chromium.org/12038095/diff/6001/content/browser/renderer_h...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/12038095/diff/6001/content/browser/renderer_h...
content/browser/renderer_host/compositor_impl_android.cc:135:
SetWindowSurface(NULL);
On 2013/01/26 01:18:00, boliu wrote:
> This was my attempt at fixing the crash inside android egl library, didn't
work
> :(

Yes, this SetWindowSurface() codepath here is not used for WebView. The
difference is that WebView creates EGL window surfaces for the renderer-side
compositor (to render into a SurfaceTexture).

For non-WebView, we add an ANativeWindow to the tracker here which is the actual
window surface for the browser compositor.

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/gpu_com...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/12038095/diff/6001/content/common/gpu/gpu_com...
content/common/gpu/gpu_command_buffer_stub.cc:435: return;
On 2013/01/26 01:18:00, boliu wrote:
> Daniel...would this be better? I can't get it to fall into line 404...but I
> copied the failure code from there...

I'm surprised the failure here did not go away with holding the extra reference
on the ANativeWindow.

But properly failing here is good. I'd say LOG() instead of DLOG().

https://codereview.chromium.org/12038095/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/12038095/diff/13001/content/browser/renderer_...
content/browser/renderer_host/surface_texture_transport_client_android.cc:56:
ANativeWindow_acquire(window_);
Now we are leaking this extra ref.
Note that CreateSurface already returns the window with one ref.

Also, we need to handle the other codepath (compositor_impl_android.cc).

Since we already correctly maintain references on the UI thread we only need to
fix the race between UI and GPU thread during surface lookup.

I suggest we just acquire an extra ref on the gpu thread in
GpuSurfaceTracker::GetNativeWidget() where we hold a lock.
(Consider renaming it to AcquireNativeWidget()).

We can release the ref after the call to surface->Initialize(), because having
created a window surface from the ANativeWindow now holds an appropriate ref to
the underlying window.

[boliu, 2013-01-28 20:47:09]

https://codereview.chromium.org/12038095/diff/13001/content/browser/renderer_...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/12038095/diff/13001/content/browser/renderer_...
content/browser/renderer_host/surface_texture_transport_client_android.cc:56:
ANativeWindow_acquire(window_);
On 2013/01/28 19:46:15, Daniel Sievers wrote:
> I suggest we just acquire an extra ref on the gpu thread in
> GpuSurfaceTracker::GetNativeWidget() where we hold a lock.
> (Consider renaming it to AcquireNativeWidget()).
> 

GpuSurfaceTracker is not an android only class. Is there a general abstraction
for gfx::AcceleratedWidget that I can use?

[no sievers, 2013-01-28 20:55:06]

https://codereview.chromium.org/12038095/diff/13001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/12038095/diff/13001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:227: if (!window_) {
if (window_ == kNullAcceleratedWidget)

[no sievers, 2013-01-28 23:05:03]

https://codereview.chromium.org/12038095/diff/10009/content/browser/gpu/gpu_s...
File content/browser/gpu/gpu_surface_tracker.cc (right):

https://codereview.chromium.org/12038095/diff/10009/content/browser/gpu/gpu_s...
content/browser/gpu/gpu_surface_tracker.cc:11: #endif  // defined(OS_ANDROID)
I *think* this might belong above including logging.h, because it's an external
header.

https://codereview.chromium.org/12038095/diff/10009/content/browser/gpu/gpu_s...
content/browser/gpu/gpu_surface_tracker.cc:106: gfx::AcceleratedWidget
GpuSurfaceTracker::GetNativeWidget(int surface_id) {
Could you rename this to AcquireNativeWidget() so it hints at the fact that this
bumps the ref?

You should only have to update this class, the GpuSurfaceLookup interface, and
the one Android call site.

https://codereview.chromium.org/12038095/diff/10009/content/browser/renderer_...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/12038095/diff/10009/content/browser/renderer_...
content/browser/renderer_host/surface_texture_transport_client_android.cc:35: if
(surface_id_)
parentheses

[boliu, 2013-01-28 23:42:12]

apatrick: Could you take a look at the android fixes in this patch?

https://codereview.chromium.org/12038095/diff/10009/content/browser/gpu/gpu_s...
File content/browser/gpu/gpu_surface_tracker.cc (right):

https://codereview.chromium.org/12038095/diff/10009/content/browser/gpu/gpu_s...
content/browser/gpu/gpu_surface_tracker.cc:11: #endif  // defined(OS_ANDROID)
On 2013/01/28 23:05:03, Daniel Sievers wrote:
> I *think* this might belong above including logging.h, because it's an
external
> header.

Done.

https://codereview.chromium.org/12038095/diff/10009/content/browser/renderer_...
File content/browser/renderer_host/surface_texture_transport_client_android.cc
(right):

https://codereview.chromium.org/12038095/diff/10009/content/browser/renderer_...
content/browser/renderer_host/surface_texture_transport_client_android.cc:35: if
(surface_id_)
On 2013/01/28 23:05:03, Daniel Sievers wrote:
> parentheses

Done.

[no sievers, 2013-01-29 18:46:07]

LGTM. Needs Al for OWNERS. 

These are some fixes to handle corner cases and races uncovered with some
webview stress tests that very quickly create and destroy views.

[apatrick_chromium, 2013-01-30 20:47:05]

lgtm

[commit-bot: I haz the power, 2013-01-30 20:48:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/12038095/12008

[commit-bot: I haz the power, 2013-01-30 23:47:23]

Change committed as 179737