aura/texture transport: Keep ref to texture infos to avoid races with the UI

aura/texture transport: Keep ref to texture infos to avoid races with the UI

In some cases, the UI may destroy the textures the renderer is rendering into
before the renderer has a chance to finish.
This patch makes it so that we avoid generating a lost context event in the
page that is closing, because we can't recover correctly in the other pages in
the same renderer process.

This also adds logging so that we can audit all cases of lost context.

BUG=122868
TEST=run aura chrome, open and close tabs (from the same process), observe no
flashing and no mention of a lost context.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132638

[jonathan.backer, 2012-04-17 17:21:49]

Just want to confirm my understanding: we don't really need to nix the
GetParentInfo. It would be sufficient to 
- switch parent_stub_ to raw pointer instead of weak_ptr
- and set parent_stub_ = NULL in OnWillDestroyStub.

If this is true (or roughly true), then LGTM. Otherwise, I really don't
understand what race this CL fixes.

http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_...
File content/common/gpu/texture_image_transport_surface.cc (right):

http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_...
content/common/gpu/texture_image_transport_surface.cc:205: textures_[1].info =
NULL;
parent_stub_ = NULL so that we don't access a stale pointer in
TextureImageTransportSurface::Destroy

http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_...
File content/common/gpu/texture_image_transport_surface.h (right):

http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_...
content/common/gpu/texture_image_transport_surface.h:71:
scoped_refptr<gpu::gles2::TextureManager::TextureInfo> info;
TextureManager::Ref

http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_...
content/common/gpu/texture_image_transport_surface.h:96: GpuCommandBufferStub*
parent_stub_;
Here's my understanding: we've got a race on teardown. With a weak_ptr,
parent_stub_ is only NULLed in the destructor. However, parent_stub_ is really
invalid during its Destroy. By becoming a DestructionObserver of parent_stub_,
we can invalidate at the appropriate time.

[piman, 2012-04-17 17:28:42]

On Tue, Apr 17, 2012 at 10:21 AM, <backer@chromium.org> wrote:

> Just want to confirm my understanding: we don't really need to nix the
> GetParentInfo. It would be sufficient to
> - switch parent_stub_ to raw pointer instead of weak_ptr
> - and set parent_stub_ = NULL in OnWillDestroyStub.
>

I also want to keep a reference on the info so that we don't destroy the
service-side textures even if the UI context destroys the client-side.
That's the main aspect of this CL, the rest is just fallout (e.g. I'm
removing GetParentInfo because it's not needed any more since I keep the
info).
The race is not vs the stub getting destroyed, it's vs the textures being
destroyed.


>
> If this is true (or roughly true), then LGTM. Otherwise, I really don't
> understand what race this CL fixes.
>
>
> http://codereview.chromium.**org/10103021/diff/5001/**
>
content/common/gpu/texture_**image_transport_surface.cc<http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_image_transport_surface.cc>
> File content/common/gpu/texture_**image_transport_surface.cc (right):
>
> http://codereview.chromium.**org/10103021/diff/5001/**
>
content/common/gpu/texture_**image_transport_surface.cc#**newcode205<http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_image_transport_surface.cc#newcode205>
> content/common/gpu/texture_**image_transport_surface.cc:**205:
> textures_[1].info = NULL;
> parent_stub_ = NULL so that we don't access a stale pointer in
> TextureImageTransportSurface::**Destroy
>
> http://codereview.chromium.**org/10103021/diff/5001/**
>
content/common/gpu/texture_**image_transport_surface.h<http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_image_transport_surface.h>
> File content/common/gpu/texture_**image_transport_surface.h (right):
>
> http://codereview.chromium.**org/10103021/diff/5001/**
>
content/common/gpu/texture_**image_transport_surface.h#**newcode71<http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_image_transport_surface.h#newcode71>
> content/common/gpu/texture_**image_transport_surface.h:71:
> scoped_refptr<gpu::gles2::**TextureManager::TextureInfo> info;
> TextureManager::Ref
>
> http://codereview.chromium.**org/10103021/diff/5001/**
>
content/common/gpu/texture_**image_transport_surface.h#**newcode96<http://codereview.chromium.org/10103021/diff/5001/content/common/gpu/texture_image_transport_surface.h#newcode96>
> content/common/gpu/texture_**image_transport_surface.h:96:
> GpuCommandBufferStub* parent_stub_;
> Here's my understanding: we've got a race on teardown. With a weak_ptr,
> parent_stub_ is only NULLed in the destructor. However, parent_stub_ is
> really invalid during its Destroy. By becoming a DestructionObserver of
> parent_stub_, we can invalidate at the appropriate time.
>
>
http://codereview.chromium.**org/10103021/<http://codereview.chromium.org/101...
>

[piman, 2012-04-17 17:37:24]

https://chromiumcodereview.appspot.com/10103021/diff/5001/content/common/gpu/...
File content/common/gpu/texture_image_transport_surface.cc (right):

https://chromiumcodereview.appspot.com/10103021/diff/5001/content/common/gpu/...
content/common/gpu/texture_image_transport_surface.cc:205: textures_[1].info =
NULL;
On 2012/04/17 17:21:50, jonathan.backer wrote:
> parent_stub_ = NULL so that we don't access a stale pointer in
> TextureImageTransportSurface::Destroy

Done.

https://chromiumcodereview.appspot.com/10103021/diff/5001/content/common/gpu/...
File content/common/gpu/texture_image_transport_surface.h (right):

https://chromiumcodereview.appspot.com/10103021/diff/5001/content/common/gpu/...
content/common/gpu/texture_image_transport_surface.h:71:
scoped_refptr<gpu::gles2::TextureManager::TextureInfo> info;
On 2012/04/17 17:21:50, jonathan.backer wrote:
> TextureManager::Ref

Done.

[jonathan.backer, 2012-04-17 17:51:19]

LGTM

From offline discussion:

- we delete the textures used in DestroySharedSurfaceHandle
- without these textures, we return false in SwapBuffers and PostSubBuffer
(there may be outstanding swaps in the pipe)
- this causes a context loss which affects the whole renderer channel
(potentially multiple tabs)

This CL keeps the TextureInfo around longer so that we don't fail in SwapBuffers
and PostSubBuffer.

[commit-bot: I haz the power, 2012-04-17 18:36:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/10103021/3003

[commit-bot: I haz the power, 2012-04-17 20:36:50]

Change committed as 132638