Let canvas decide how to handle the case of resource lost reported by client: 2D part

Let canvas 2D decide how to handle the case of resource lost reported by client

We must clear and release some internal references even if the mailbox resource
is treated as lost from client code, so that no memory leaking happens.


BUG=390960
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178736

[Hongbo Min, 2014-07-03 08:22:48]

junov@, danakj@

This CL is trying to let canvas handle the resource lost by itself, not just
ignoring to call mailboxReleased by WebExternalTextureLayerClient.

The chromium part can be found at https://codereview.chromium.org/369823002

Could you please take a review? Thanks.

[dshwang, 2014-07-03 08:36:58]

https://codereview.chromium.org/369043003/diff/1/Source/platform/graphics/gpu...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: // TODO(hmin): Handle the
resource lost case.
I'm also curious about this case.
When compositor loses its context, it reports |lostResource|. However, mailbox
of WebGL is always created and deleted by WebGL.
Dose WebGL also lose its context always when compositor loses context?

In addition, this callback releases mailbox as follows;
 WaitSyncPoint(sync_point)
 DeleteTexture(texture)
When WebGL loses context, above operations become no-op, right?

[Hongbo Min, 2014-07-03 09:11:19]

https://codereview.chromium.org/369043003/diff/1/Source/platform/graphics/gpu...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/1/Source/platform/graphics/gpu...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: // TODO(hmin): Handle the
resource lost case.
On 2014/07/03 08:36:58, dshwang wrote:
> I'm also curious about this case.
> When compositor loses its context, it reports |lostResource|. However, mailbox
> of WebGL is always created and deleted by WebGL.
> Dose WebGL also lose its context always when compositor loses context?
> 
> In addition, this callback releases mailbox as follows;
>  WaitSyncPoint(sync_point)
>  DeleteTexture(texture)
> When WebGL loses context, above operations become no-op, right?

Thanks for your comments! The CL is updated with the webgl handling.

[Hongbo Min, 2014-07-03 09:29:28]

Ops...the buildbots are all red, how do I make buildbot green if it also
requires changes from chromium part?

[dshwang, 2014-07-03 09:52:13]

On 2014/07/03 09:11:19, Hongbo Min wrote:
> Thanks for your comments! The CL is updated with the webgl handling.

Thank you for changing, but I just asked questions to all developers, not
intended to change your code.
And patch set 1 is better to delete remained resources for your CL's purpose.

On 2014/07/03 09:29:28, Hongbo Min wrote:
> Ops...the buildbots are all red, how do I make buildbot green if it also
> requires changes from chromium part?

It's because WebExternalTextureLayerClient implementation in chromium side still
implements previous API without new boolean parameter.
To land this kind of change affecting both Blink and Chromium, burdensome ifdef
technique and chromium-blink-chromium-blink commits combination are required.

[Justin Novosad, 2014-07-03 13:31:52]

No need for burdensome ifdef stuff.
I suggest landing the blink side first. To do that you you can use a default
parameter value in WebExternalTextureLayerClient::mailboxReleased, so that it
can compile against chromium.

[Hongbo Min, 2014-07-03 14:24:34]

On 2014/07/03 13:31:52, junov wrote:
> No need for burdensome ifdef stuff.
> I suggest landing the blink side first. To do that you you can use a default
> parameter value in WebExternalTextureLayerClient::mailboxReleased, so that it
> can compile against chromium.

Done! thanks.

[Justin Novosad, 2014-07-03 14:50:32]

https://codereview.chromium.org/369043003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/60001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:508: if (reallyLostResource) {
I think this condition could just be "if (contextLost)".

The lostResource parameter may not be necessary.

[Hongbo Min, 2014-07-03 14:55:29]

https://codereview.chromium.org/369043003/diff/60001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/60001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:508: if (reallyLostResource) {
On 2014/07/03 14:50:32, junov wrote:
> I think this condition could just be "if (contextLost)".
> 
> The lostResource parameter may not be necessary.

Done.

[Hongbo Min, 2014-07-03 14:59:03]

Although the lostResource flag is forwarded to the canvas impl through
mailboxReleased, it seems the actual impl doesn't depends on the lostResource,
instead, the isContextLost is enough to determine if the resource is really
lost.

[danakj, 2014-07-03 15:09:03]

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
why is this bool never used? If it's true just delete the texture and don't
recycle it.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.h:63: virtual void
mailboxReleased(const blink::WebExternalTextureMailbox&, bool) OVERRIDE;
variable name for a "bool" please, it's name is not implied by its type

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:279: void
DrawingBuffer::mailboxReleased(const blink::WebExternalTextureMailbox& mailbox,
bool lostResource)
why is this new variable not used?

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.h:157: virtual void
mailboxReleased(const blink::WebExternalTextureMailbox&, bool) OVERRIDE;
var name

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBufferTest.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBufferTest.cpp:490:
m_drawingBuffer->mailboxReleased(mailbox, false);
no tests that pass true?

https://codereview.chromium.org/369043003/diff/80001/public/platform/WebExter...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/369043003/diff/80001/public/platform/WebExter...
public/platform/WebExternalTextureLayerClient.h:48: virtual void
mailboxReleased(const WebExternalTextureMailbox&, bool = false) = 0;
can you give this a variable name, variable names are better than comments.

[dshwang, 2014-07-03 16:20:17]

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:279: void
DrawingBuffer::mailboxReleased(const blink::WebExternalTextureMailbox& mailbox,
bool lostResource)
On 2014/07/03 15:09:02, danakj wrote:
> why is this new variable not used?

After reading https://code.google.com/p/chromium/issues/detail?id=390960#c6, I
understand this situation.

If (lostResource == false && !m_context->isContextLost()), it's normal case
If (lostResource == true && m_context->isContextLost()), gpu process crashed.
If (lostResource == true && !m_context->isContextLost()), now render process is
being shutdown.
If (lostResource == false && m_context->isContextLost()), webgl context is
evicted or javascript kill webgl context.

So
If (lostResource == true && m_context->isContextLost()), we can return early.
But calling mailboxReleasedWhileDestructionInProgress(mailbox) would be better
although all GL calls is no-op, because
mailboxReleasedWhileDestructionInProgress() cleans its member variables.
If (lostResource == true && !m_context->isContextLost()), we need to remove
mailbox on shutdown process, so call
mailboxReleasedWhileDestructionInProgress(), as danakj@ mentioned.
If (lostResource == false && m_context->isContextLost()), we can return early,
but calling mailboxReleasedWhileDestructionInProgress(mailbox) would be better.

In conclusion, all abnormal cases need to remove resources directly.

[Hongbo Min, 2014-07-07 04:30:28]

CL is updated with resolving some nits.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
On 2014/07/03 15:09:02, danakj wrote:
> why is this bool never used? If it's true just delete the texture and don't
> recycle it.

See comments in DrawingBuffer.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/Canvas2DLayerBridge.h:63: virtual void
mailboxReleased(const blink::WebExternalTextureMailbox&, bool) OVERRIDE;
On 2014/07/03 15:09:02, danakj wrote:
> variable name for a "bool" please, it's name is not implied by its type

Done.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:279: void
DrawingBuffer::mailboxReleased(const blink::WebExternalTextureMailbox& mailbox,
bool lostResource)
Thanks your summary, dshwang@.

The reason why lostResource is not used is straightforward, the lostResource can
not determine if a resource is recycled or not, only m_context->isContextLost
can determine.

The lostResource flag is true in case of :
1. The GPU process crashes, or
2. The render thread/process is shutting down, but at this point, it is not
receiving the resource reclaim ACK message for the most recent frame resources
from browser-side compositor for some reasons, e.g. browser compositor becomes
invisible

In the above 2 cases, there is no need to recycle the resource, but we need to
clear the internal reference to ref-counted object held by mailbox. However, we
can not decide if the GPU texture resource is recycled via lostResource flag.

If the context is lost indicating by isContextLost, we don't need to recycle GPU
texture resource, but of course, we need to clear the internal reference.


On 2014/07/03 16:20:17, dshwang wrote:
> On 2014/07/03 15:09:02, danakj wrote:
> > why is this new variable not used?
> 
> After reading https://code.google.com/p/chromium/issues/detail?id=390960#c6, I
> understand this situation.
> 
> If (lostResource == false && !m_context->isContextLost()), it's normal case
> If (lostResource == true && m_context->isContextLost()), gpu process crashed.
> If (lostResource == true && !m_context->isContextLost()), now render process
is
> being shutdown.
> If (lostResource == false && m_context->isContextLost()), webgl context is
> evicted or javascript kill webgl context.
> 
> So
> If (lostResource == true && m_context->isContextLost()), we can return early.
> But calling mailboxReleasedWhileDestructionInProgress(mailbox) would be better
> although all GL calls is no-op, because
> mailboxReleasedWhileDestructionInProgress() cleans its member variables.
> If (lostResource == true && !m_context->isContextLost()), we need to remove
> mailbox on shutdown process, so call
> mailboxReleasedWhileDestructionInProgress(), as danakj@ mentioned.
> If (lostResource == false && m_context->isContextLost()), we can return early,
> but calling mailboxReleasedWhileDestructionInProgress(mailbox) would be
better.
> 
> In conclusion, all abnormal cases need to remove resources directly.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.h:157: virtual void
mailboxReleased(const blink::WebExternalTextureMailbox&, bool) OVERRIDE;
On 2014/07/03 15:09:02, danakj wrote:
> var name 

Done.

https://codereview.chromium.org/369043003/diff/80001/public/platform/WebExter...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/369043003/diff/80001/public/platform/WebExter...
public/platform/WebExternalTextureLayerClient.h:48: virtual void
mailboxReleased(const WebExternalTextureMailbox&, bool = false) = 0;
On 2014/07/03 15:09:02, danakj wrote:
> can you give this a variable name, variable names are better than comments.

Done.

[dshwang, 2014-07-07 09:18:14]

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
&& !m_context->isContextLost()) {
As my explanation is applied to code, this if statement should be
 if (m_destructionInProgress || m_context->isContextLost() || lostResource)

All three case needs to clear internal members as well as texture if possible.

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:292: if
(!m_context->isContextLost())
remove

[Hongbo Min, 2014-07-07 09:39:47]

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
&& !m_context->isContextLost()) {
On 2014/07/07 09:18:14, dshwang wrote:
> As my explanation is applied to code, this if statement should be
>  if (m_destructionInProgress || m_context->isContextLost() || lostResource)
> 
> All three case needs to clear internal members as well as texture if possible.

Thanks for your comments. It seems mailboxReleasedWhileDestructionInProgress
requires a valid context to perform mailbox deletion. It needs to call
makeContextCurrent firstly, right?

[dshwang, 2014-07-07 10:09:43]

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
&& !m_context->isContextLost()) {
afaik, when we use invalid context like TexImage2D, it becomes no-op. So IMO
calling mailboxReleasedWhileDestructionInProgress is fine when the context is
invalid.
danakj@, am I right?

[Hongbo Min, 2014-07-07 14:38:58]

On 2014/07/07 10:09:43, dshwang wrote:
>
https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/369043003/diff/80001/Source/platform/graphics...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if
(m_destructionInProgress
> && !m_context->isContextLost()) {
> afaik, when we use invalid context like TexImage2D, it becomes no-op. So IMO
> calling mailboxReleasedWhileDestructionInProgress is fine when the context is
> invalid.
> danakj@, am I right?

The problem here is, if we already know the context is lost, why do we need to
invoke GL operation on an invalid context? In case of context is lost, the
effective part called from mailboxReleasedWhileDestructionInProgress is to
remove the mailbox from the mailbox list, so I update CL with removing it
directly from the list if context is lost.

[danakj, 2014-07-07 15:44:02]

On Mon, Jul 7, 2014 at 10:38 AM, <hongbo.min@intel.com> wrote:

> On 2014/07/07 10:09:43, dshwang wrote:
>
> https://codereview.chromium.org/369043003/diff/80001/
> Source/platform/graphics/gpu/DrawingBuffer.cpp
>
>> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
>>
>
>
> https://codereview.chromium.org/369043003/diff/80001/
> Source/platform/graphics/gpu/DrawingBuffer.cpp#newcode281
>
>> Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if
>>
> (m_destructionInProgress
>
>> && !m_context->isContextLost()) {
>> afaik, when we use invalid context like TexImage2D, it becomes no-op. So
>> IMO
>> calling mailboxReleasedWhileDestructionInProgress is fine when the
>> context is
>> invalid.
>> danakj@, am I right?
>>
>
> The problem here is, if we already know the context is lost, why do we
> need to
> invoke GL operation on an invalid context? In case of context is lost, the
> effective part called from mailboxReleasedWhileDestructionInProgress is to
> remove the mailbox from the mailbox list, so I update CL with removing it
> directly from the list if context is lost.
>

It's fine to make use of a lost context, and it keeps the code simpler to
always clean up after yourself rather than avoid cleaning up in some cases.
I agree with dongseong we should just always clean up.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Hongbo Min, 2014-07-08 09:45:10]

@danakj, @dshwang, CL is updated. Could you please have further review? thanks!

[Hongbo Min, 2014-07-08 09:56:38]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
|| m_context->isContextLost() || lostResource) {
dshwang@, does it cover the cases you mentioned?

[dshwang, 2014-07-08 12:26:41]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
|| m_context->isContextLost() || lostResource) {
yes, it covers all. thanks.

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
this line is not necessary. do you add it to clarify?

[Hongbo Min, 2014-07-08 14:19:35]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
On 2014/07/08 12:26:40, dshwang wrote:
> this line is not necessary. do you add it to clarify?

It ensures the reference to DrawingBuffer is definitely cleared before it is
removed from the list.

[dshwang, 2014-07-08 15:07:05]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
It's not important part but I want to ask why?
On removing it from the list, the destructor of DrawingBuffer is called, so
m_parentDrawingBuffer is cleared automatically.

[danakj, 2014-07-08 15:19:41]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
I don't see this method using lostResource still? How come?

[Hongbo Min, 2014-07-09 01:43:09]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
On 2014/07/08 15:19:41, danakj wrote:
> I don't see this method using lostResource still? How come?

Yes, lostResource is not needed here in my opinion. It depends on contextLost
instead.

1. if (lostResource == false && contextLost == false), it is normal case;
2. if (lostResource == false && contextLost == true), gpu process crash, reset
the mailbox status to be available, and return.
3. if (lostResource == true && contextLost == false), it implies the child
compositor doesn't receive the ACK from parent, mark this mailbox as
to-be-released mailbox, and call freeReleasedMailbox if needed.
4. if (lostResource == false && contextLost == true), gpu process crash, reset
the mailbox status to be available, and return. same as 3.

So in fact here it has nothing depending on lostResource, right?

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
On 2014/07/08 15:07:05, dshwang wrote:
> It's not important part but I want to ask why?
> On removing it from the list, the destructor of DrawingBuffer is called, so
> m_parentDrawingBuffer is cleared automatically.

Since each element in the list is ref-counted object, it is possible that the
mailbox is also referenced by others. Here I just want to ensure the
DrawingBuffer is definitely dereferenced before removing the mailbox.

[dshwang, 2014-07-09 07:14:37]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
So freeReleasedMailbox() must be called when lostResource.

Why not same style to WebGL as follow?
            if (isHidden() || contextLost || lostResource) {
                freeReleasedMailbox();
            } else {
                ASSERT(!m_destructionInProgress);
               
Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
            }

[Hongbo Min, 2014-07-09 07:36:56]

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
mailbox, bool lostResource)
On 2014/07/09 07:14:36, dshwang wrote:
> So freeReleasedMailbox() must be called when lostResource.
> 
> Why not same style to WebGL as follow?
>             if (isHidden() || contextLost || lostResource) {
>                 freeReleasedMailbox();
>             } else {
>                 ASSERT(!m_destructionInProgress);
>                
> Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
>             }

freeReleasedMailbox would return early if context is lost, as a result, the
mailbox status is not updated to the right status.

If lostResource==true && contextLost == false, it is considered as a normal case
indeed, and the GL resource should be reclaimed by freeReleasedMailbox.

Will update as following:
if (isHidden() || lostResource) {
  freeReleasedMailbox();
}

[Hongbo Min, 2014-07-09 07:39:26]

On 2014/07/08 15:19:41, danakj wrote:
>
https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> 
>
https://codereview.chromium.org/369043003/diff/160001/Source/platform/graphic...
> Source/platform/graphics/Canvas2DLayerBridge.cpp:492: void
> Canvas2DLayerBridge::mailboxReleased(const blink::WebExternalTextureMailbox&
> mailbox, bool lostResource)
> I don't see this method using lostResource still? How come?

lostResource is used in Patch Set 9.

[dshwang, 2014-07-09 08:14:50]

Thank you for updating. Overall code looks good to me.
However, Canvas code change needs to be checked by junov@
I don't fully understand Canvas2DLayerBridge, so I ask some questions in below
comments.

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:326: if (isHidden() ||
releasedMailboxHasExpired())
Is "|| lostResource" needed?

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
Do we need to remove the given mailboxInfo from m_mailboxes?

[Hongbo Min, 2014-07-11 14:43:37]

ping junov@...thanks.

[Justin Novosad, 2014-07-11 17:22:38]

lgtm

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:326: if (isHidden() ||
releasedMailboxHasExpired())
On 2014/07/09 08:14:50, dshwang wrote:
> Is "|| lostResource" needed?

Not necessary. If we are in a state where resources are being dropped, we won't
be reclaiming from this context later.  So we're safe.

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
On 2014/07/09 08:14:50, dshwang wrote:
> Do we need to remove the given mailboxInfo from m_mailboxes?

That is a good question. A generated mailbox is nothing more than a
cryptographically strong name. This name is not strictly associated with the
context that created it or any context that uses it. My understanding is that
the mailbox will be unbound from the resource it carries if that resource is
lost, but you can still continue to use the mailbox safely. Even if it were not
the case that mailboxes survive a context loss, we would still be OK, because
the context lost/restore mechanism will tear down the current layer bridge and
spawn a new one.  The Canvas2DLayerBridge associated with the lost context will
not be preparing any new mailboxes from that point on.

[Justin Novosad, 2014-07-11 17:24:34]

That is, lgtm for the 2D canvas portion.
Adding kbr to look at changes to DrawingBuffer

[danakj, 2014-07-11 17:31:07]

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
On 2014/07/11 17:22:38, junov wrote:
> On 2014/07/09 08:14:50, dshwang wrote:
> > Do we need to remove the given mailboxInfo from m_mailboxes?
> 
> That is a good question. A generated mailbox is nothing more than a
> cryptographically strong name. This name is not strictly associated with the
> context that created it or any context that uses it. My understanding is that
> the mailbox will be unbound from the resource it carries if that resource is
> lost, but you can still continue to use the mailbox safely. Even if it were
not
> the case that mailboxes survive a context loss, we would still be OK, because
> the context lost/restore mechanism will tear down the current layer bridge and
> spawn a new one.  The Canvas2DLayerBridge associated with the lost context
will
> not be preparing any new mailboxes from that point on.

To be sure we're on the same page the |lostResource| does not imply the texture
is invalid or the context3d is lost. It just says that there is no known valid
sync point at which you can start using the texture again, so the only safe
thing you can do is drop your ref to it and stop using it (delete your local
texture id bound to the mailbox). We should not be reusing the mailbox string at
all after that point. Also, when a context is really lost, all textures and
mailbox strings are no longer valid. New textures will have to be made and
produced into mailboxes to make use of again.

[Justin Novosad, 2014-07-11 17:37:53]

On 2014/07/11 17:31:07, danakj wrote:
>
https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> 
>
https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
> Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
> Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
> On 2014/07/11 17:22:38, junov wrote:
> > On 2014/07/09 08:14:50, dshwang wrote:
> > > Do we need to remove the given mailboxInfo from m_mailboxes?
> > 
> > That is a good question. A generated mailbox is nothing more than a
> > cryptographically strong name. This name is not strictly associated with the
> > context that created it or any context that uses it. My understanding is
that
> > the mailbox will be unbound from the resource it carries if that resource is
> > lost, but you can still continue to use the mailbox safely. Even if it were
> not
> > the case that mailboxes survive a context loss, we would still be OK,
because
> > the context lost/restore mechanism will tear down the current layer bridge
and
> > spawn a new one.  The Canvas2DLayerBridge associated with the lost context
> will
> > not be preparing any new mailboxes from that point on.
> 
> To be sure we're on the same page the |lostResource| does not imply the
texture
> is invalid or the context3d is lost. It just says that there is no known valid
> sync point at which you can start using the texture again, so the only safe
> thing you can do is drop your ref to it and stop using it (delete your local
> texture id bound to the mailbox). We should not be reusing the mailbox string
at
> all after that point. Also, when a context is really lost, all textures and
> mailbox strings are no longer valid. New textures will have to be made and
> produced into mailboxes to make use of again.

A couple releases ago, Canvas2DLayerBridge was recycling mailboxes after a
context loss, and there were no issues. Was that unsafe?  It seemed stable, but
maybe that was just a coincidence with the currently implementation.
Also the mailbox spec states that the mailbox is simply unbound if the texture
it was bound to is destroyed in all contexts. So it should be fine to bind a new
texture to that mailbox in prepareMailbox, right?

[danakj, 2014-07-11 17:51:20]

On Fri, Jul 11, 2014 at 1:37 PM, <junov@chromium.org> wrote:

> On 2014/07/11 17:31:07, danakj wrote:
>
> https://codereview.chromium.org/369043003/diff/180001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp
>
>> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
>>
>
>
> https://codereview.chromium.org/369043003/diff/180001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode512
>
>> Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
>> Canvas2DLayerManager::get().layerTransientResourceAllocati
>> onChanged(this);
>> On 2014/07/11 17:22:38, junov wrote:
>> > On 2014/07/09 08:14:50, dshwang wrote:
>> > > Do we need to remove the given mailboxInfo from m_mailboxes?
>> >
>> > That is a good question. A generated mailbox is nothing more than a
>> > cryptographically strong name. This name is not strictly associated
>> with the
>> > context that created it or any context that uses it. My understanding is
>>
> that
>
>> > the mailbox will be unbound from the resource it carries if that
>> resource is
>> > lost, but you can still continue to use the mailbox safely. Even if it
>> were
>> not
>> > the case that mailboxes survive a context loss, we would still be OK,
>>
> because
>
>> > the context lost/restore mechanism will tear down the current layer
>> bridge
>>
> and
>
>> > spawn a new one.  The Canvas2DLayerBridge associated with the lost
>> context
>> will
>> > not be preparing any new mailboxes from that point on.
>>
>
>  To be sure we're on the same page the |lostResource| does not imply the
>>
> texture
>
>> is invalid or the context3d is lost. It just says that there is no known
>> valid
>> sync point at which you can start using the texture again, so the only
>> safe
>> thing you can do is drop your ref to it and stop using it (delete your
>> local
>> texture id bound to the mailbox). We should not be reusing the mailbox
>> string
>>
> at
>
>> all after that point. Also, when a context is really lost, all textures
>> and
>> mailbox strings are no longer valid. New textures will have to be made and
>> produced into mailboxes to make use of again.
>>
>
> A couple releases ago, Canvas2DLayerBridge was recycling mailboxes after a
> context loss, and there were no issues. Was that unsafe?  It seemed
> stable, but
> maybe that was just a coincidence with the currently implementation.
> Also the mailbox spec states that the mailbox is simply unbound if the
> texture
> it was bound to is destroyed in all contexts. So it should be fine to bind
> a new
> texture to that mailbox in prepareMailbox, right?
>

Oh, like skipping the GenMailbox? I was thinking you skipped the prepare
also.  I guess that would work in the case the context was lost. But not
necessarily in the case that the |lostResource| flag is set.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Justin Novosad, 2014-07-11 18:05:05]

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:517: if (isHidden() ||
lostResource) {
According to Dana's most recent comment, we probably need an additional step
here to discard the current mailboxInfo in the lostResource case. Also going
through the MailboxReleased state and then calling freeReleasedMailbox would not
be necessary since we don't have to care about the syncPoint for a lost
resource.

[dshwang, 2014-07-14 11:55:54]

On 2014/07/11 17:37:53, junov wrote:
> On 2014/07/11 17:31:07, danakj wrote:
> >
>
https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
> > File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> > 
> >
>
https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
> > Source/platform/graphics/Canvas2DLayerBridge.cpp:512:
> > Canvas2DLayerManager::get().layerTransientResourceAllocationChanged(this);
> > On 2014/07/11 17:22:38, junov wrote:
> > > On 2014/07/09 08:14:50, dshwang wrote:
> > > > Do we need to remove the given mailboxInfo from m_mailboxes?
> > > 
> > > That is a good question. A generated mailbox is nothing more than a
> > > cryptographically strong name. This name is not strictly associated with
the
> > > context that created it or any context that uses it. My understanding is
> that
> > > the mailbox will be unbound from the resource it carries if that resource
is
> > > lost, but you can still continue to use the mailbox safely. Even if it
were
> > not
> > > the case that mailboxes survive a context loss, we would still be OK,
> because
> > > the context lost/restore mechanism will tear down the current layer bridge
> and
> > > spawn a new one.  The Canvas2DLayerBridge associated with the lost context
> > will
> > > not be preparing any new mailboxes from that point on.
> > 
> > To be sure we're on the same page the |lostResource| does not imply the
> texture
> > is invalid or the context3d is lost. It just says that there is no known
valid
> > sync point at which you can start using the texture again, so the only safe
> > thing you can do is drop your ref to it and stop using it (delete your local
> > texture id bound to the mailbox). We should not be reusing the mailbox
string
> at
> > all after that point. Also, when a context is really lost, all textures and
> > mailbox strings are no longer valid. New textures will have to be made and
> > produced into mailboxes to make use of again.
> 
> A couple releases ago, Canvas2DLayerBridge was recycling mailboxes after a
> context loss, and there were no issues. Was that unsafe?  It seemed stable,
but
> maybe that was just a coincidence with the currently implementation.
> Also the mailbox spec states that the mailbox is simply unbound if the texture
> it was bound to is destroyed in all contexts. So it should be fine to bind a
new
> texture to that mailbox in prepareMailbox, right?

It's true, but I'm not sure if it's worth to bear the complexity. GenMailbox
might be cheap. It's easy to understand that when texture is deleted mailbox is
also deleted.

[Hongbo Min, 2014-07-15 13:51:02]

On 2014/07/11 17:24:34, junov wrote:
> That is, lgtm for the 2D canvas portion.
> Adding kbr to look at changes to DrawingBuffer

kbr@, could you please take a review the changes to DrawingBuffer? Thanks in
advance.

[Hongbo Min, 2014-07-15 13:51:13]

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/180001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:517: if (isHidden() ||
lostResource) {
On 2014/07/11 18:05:05, junov wrote:
> According to Dana's most recent comment, we probably need an additional step
> here to discard the current mailboxInfo in the lostResource case. Also going
> through the MailboxReleased state and then calling freeReleasedMailbox would
not
> be necessary since we don't have to care about the syncPoint for a lost
> resource.

To make code much clean, I reset the syncPoint to 0 for a lost resource, and
reuse freeReleasedMailbox to update the mailbox status.

@junov, @danakj, does it look good to you?

[Justin Novosad, 2014-07-15 14:38:29]

https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
mailboxInfo->m_mailbox.syncPoint = 0;
I think you should be doing more than this. According to Dana's earlier comment,
we should avoid recycling a mailbox that is associated with a lost resource. To
achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
confirm that this is what you meant?

[Hongbo Min, 2014-07-15 14:49:16]

On 2014/07/15 14:38:29, junov wrote:
>
https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> 
>
https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
> Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
> mailboxInfo->m_mailbox.syncPoint = 0;
> I think you should be doing more than this. According to Dana's earlier
comment,
> we should avoid recycling a mailbox that is associated with a lost resource.
To
> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
> confirm that this is what you meant?

@Dana, does it mean the mailboxInfo can not be reused if the resource is lost
(not caused by context lost, just because the child client doesn't know how
service side handle it)? My understanding is, the mailboxInfo can still be
reused since its internal members will be reset in prepareMailbox for next
frame, so no need to remove it from the mailboxInfo vector, right?

[danakj, 2014-07-15 14:53:08]

https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
mailboxInfo->m_mailbox.syncPoint = 0;
On 2014/07/15 14:38:28, junov wrote:
> I think you should be doing more than this. According to Dana's earlier
comment,
> we should avoid recycling a mailbox that is associated with a lost resource.
To
> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
> confirm that this is what you meant?

Yes.

[danakj, 2014-07-15 14:54:11]

On Tue, Jul 15, 2014 at 10:49 AM, <hongbo.min@intel.com> wrote:

> On 2014/07/15 14:38:29, junov wrote:
>
> https://codereview.chromium.org/369043003/diff/200001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp
>
>> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
>>
>
>
> https://codereview.chromium.org/369043003/diff/200001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
>
>> Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
>> mailboxInfo->m_mailbox.syncPoint = 0;
>> I think you should be doing more than this. According to Dana's earlier
>>
> comment,
>
>> we should avoid recycling a mailbox that is associated with a lost
>> resource.
>>
> To
>
>> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
>> confirm that this is what you meant?
>>
>
> @Dana, does it mean the mailboxInfo can not be reused if the resource is
> lost
> (not caused by context lost, just because the child client doesn't know how
> service side handle it)? My understanding is, the mailboxInfo can still be
> reused since its internal members will be reset in prepareMailbox for next
> frame, so no need to remove it from the mailboxInfo vector, right?
>

It can't be reused. There is no sync point to wait on to start using it, so
reusing it could clobber it while the parent compositor's using it.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Hongbo Min, 2014-07-15 14:55:36]

On 2014/07/15 14:49:16, Hongbo Min wrote:
> On 2014/07/15 14:38:29, junov wrote:
> >
>
https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
> > File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> > 
> >
>
https://codereview.chromium.org/369043003/diff/200001/Source/platform/graphic...
> > Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
> > mailboxInfo->m_mailbox.syncPoint = 0;
> > I think you should be doing more than this. According to Dana's earlier
> comment,
> > we should avoid recycling a mailbox that is associated with a lost resource.
> To
> > achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
> > confirm that this is what you meant?
> 
> @Dana, does it mean the mailboxInfo can not be reused if the resource is lost
> (not caused by context lost, just because the child client doesn't know how
> service side handle it)? My understanding is, the mailboxInfo can still be
> reused since its internal members will be reset in prepareMailbox for next
> frame, so no need to remove it from the mailboxInfo vector, right?

Moreover, when the mailboxInfo is freed by freeReleasedMailbox, the mailboxInfo
would be reset, including its status and backing texture. Do we need to add code
snippet for lostResource like the following?

if (lostResource) {
  clear_mailbox_SkImage;    // Done in freeReleasedMailbox
  remove_mailboxInfo_from_vector; // Specific to lostResource
}

[danakj, 2014-07-15 14:57:03]

On Tue, Jul 15, 2014 at 10:55 AM, <hongbo.min@intel.com> wrote:

> On 2014/07/15 14:49:16, Hongbo Min wrote:
>
>> On 2014/07/15 14:38:29, junov wrote:
>> >
>>
>
> https://codereview.chromium.org/369043003/diff/200001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp
>
>> > File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/369043003/diff/200001/
> Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
>
>> > Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
>> > mailboxInfo->m_mailbox.syncPoint = 0;
>> > I think you should be doing more than this. According to Dana's earlier
>> comment,
>> > we should avoid recycling a mailbox that is associated with a lost
>> resource.
>> To
>> > achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can
>> you
>> > confirm that this is what you meant?
>>
>
>  @Dana, does it mean the mailboxInfo can not be reused if the resource is
>> lost
>> (not caused by context lost, just because the child client doesn't know
>> how
>> service side handle it)? My understanding is, the mailboxInfo can still be
>> reused since its internal members will be reset in prepareMailbox for next
>> frame, so no need to remove it from the mailboxInfo vector, right?
>>
>
> Moreover, when the mailboxInfo is freed by freeReleasedMailbox, the
> mailboxInfo
> would be reset, including its status and backing texture. Do we need to
> add code
> snippet for lostResource like the following?
>
> if (lostResource) {
>   clear_mailbox_SkImage;    // Done in freeReleasedMailbox
>   remove_mailboxInfo_from_vector; // Specific to lostResource
> }
>

It's backing texture should be deleted, if that's what you're asking, maybe
Junov can answer this one better.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Hongbo Min, 2014-07-15 15:01:14]

On 2014/07/15 14:57:03, danakj wrote:
> On Tue, Jul 15, 2014 at 10:55 AM, <mailto:hongbo.min@intel.com> wrote:
> 
> > On 2014/07/15 14:49:16, Hongbo Min wrote:
> >
> >> On 2014/07/15 14:38:29, junov wrote:
> >> >
> >>
> >
> > https://codereview.chromium.org/369043003/diff/200001/
> > Source/platform/graphics/Canvas2DLayerBridge.cpp
> >
> >> > File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/369043003/diff/200001/
> > Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
> >
> >> > Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
> >> > mailboxInfo->m_mailbox.syncPoint = 0;
> >> > I think you should be doing more than this. According to Dana's earlier
> >> comment,
> >> > we should avoid recycling a mailbox that is associated with a lost
> >> resource.
> >> To
> >> > achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can
> >> you
> >> > confirm that this is what you meant?
> >>
> >
> >  @Dana, does it mean the mailboxInfo can not be reused if the resource is
> >> lost
> >> (not caused by context lost, just because the child client doesn't know
> >> how
> >> service side handle it)? My understanding is, the mailboxInfo can still be
> >> reused since its internal members will be reset in prepareMailbox for next
> >> frame, so no need to remove it from the mailboxInfo vector, right?
> >>
> >
> > Moreover, when the mailboxInfo is freed by freeReleasedMailbox, the
> > mailboxInfo
> > would be reset, including its status and backing texture. Do we need to
> > add code
> > snippet for lostResource like the following?
> >
> > if (lostResource) {
> >   clear_mailbox_SkImage;    // Done in freeReleasedMailbox
> >   remove_mailboxInfo_from_vector; // Specific to lostResource
> > }
> >
> 
> It's backing texture should be deleted, if that's what you're asking, maybe
> Junov can answer this one better.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

junov@, will the following code in freeReleasedMailbox delete the backing
texture held by SkImage?

325    if (mailboxInfo->m_image) {                                              
   
326         if (isHidden() || releasedMailboxHasExpired())                      
    
327            
mailboxInfo->m_image->getTexture()->resetFlag(static_cast<GrTextureFlags>(GrTexture::kReturnToCache_FlagBit));
328         mailboxInfo->m_image->getTexture()->textureParamsModified();        
    
329         mailboxInfo->m_image.clear();                                       
    
330     }

[Hongbo Min, 2014-07-15 15:10:29]

On 2014/07/15 14:54:11, danakj wrote:
> On Tue, Jul 15, 2014 at 10:49 AM, <mailto:hongbo.min@intel.com> wrote:
> 
> > On 2014/07/15 14:38:29, junov wrote:
> >
> > https://codereview.chromium.org/369043003/diff/200001/
> > Source/platform/graphics/Canvas2DLayerBridge.cpp
> >
> >> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> >>
> >
> >
> > https://codereview.chromium.org/369043003/diff/200001/
> > Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
> >
> >> Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
> >> mailboxInfo->m_mailbox.syncPoint = 0;
> >> I think you should be doing more than this. According to Dana's earlier
> >>
> > comment,
> >
> >> we should avoid recycling a mailbox that is associated with a lost
> >> resource.
> >>
> > To
> >
> >> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can you
> >> confirm that this is what you meant?
> >>
> >
> > @Dana, does it mean the mailboxInfo can not be reused if the resource is
> > lost
> > (not caused by context lost, just because the child client doesn't know how
> > service side handle it)? My understanding is, the mailboxInfo can still be
> > reused since its internal members will be reset in prepareMailbox for next
> > frame, so no need to remove it from the mailboxInfo vector, right?
> >
> 
> It can't be reused. There is no sync point to wait on to start using it, so
> reusing it could clobber it while the parent compositor's using it.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

But from current implementation of prepareMailbox, we also reset the syncPoint
by insertSyncPoint for the mailbox returned from createMailboxInfo which firstly
try to find the available mailbox to reuse, and from now on the parent
compositor will see the new syncPoint for the next frame. Is there anything I am
missing?

[Justin Novosad, 2014-07-15 15:17:57]

On 2014/07/15 14:49:16, Hongbo Min wrote:

> @Dana, does it mean the mailboxInfo can not be reused if the resource is lost
> (not caused by context lost, just because the child client doesn't know how
> service side handle it)? My understanding is, the mailboxInfo can still be
> reused since its internal members will be reset in prepareMailbox for next
> frame, so no need to remove it from the mailboxInfo vector, right?

The problem is that prepareMailbox actually does not reset all the members of
mailboxInfo. It recycles the mailbox name, and that is the member that really
matters in this case. So when a resource is lost, you can either reset the
mailbox name (call GenMailbox) or delete the mailboxInfo entirely, and
createMailboxInfo will generate a new mailbox the next time one is needed.  I
think it is better to delete the mailboxInfo because calling GenMailbox has a
cost, so we should only call it once we know that a new mailbox will be needed.

[danakj, 2014-07-15 15:21:43]

On Tue, Jul 15, 2014 at 11:10 AM, <hongbo.min@intel.com> wrote:

> On 2014/07/15 14:54:11, danakj wrote:
>
>  On Tue, Jul 15, 2014 at 10:49 AM, <mailto:hongbo.min@intel.com> wrote:
>>
>
>  > On 2014/07/15 14:38:29, junov wrote:
>> >
>> > https://codereview.chromium.org/369043003/diff/200001/
>> > Source/platform/graphics/Canvas2DLayerBridge.cpp
>> >
>> >> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/369043003/diff/200001/
>> > Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
>> >
>> >> Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
>> >> mailboxInfo->m_mailbox.syncPoint = 0;
>> >> I think you should be doing more than this. According to Dana's earlier
>> >>
>> > comment,
>> >
>> >> we should avoid recycling a mailbox that is associated with a lost
>> >> resource.
>> >>
>> > To
>> >
>> >> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can
>> you
>> >> confirm that this is what you meant?
>> >>
>> >
>> > @Dana, does it mean the mailboxInfo can not be reused if the resource is
>> > lost
>> > (not caused by context lost, just because the child client doesn't know
>> how
>> > service side handle it)? My understanding is, the mailboxInfo can still
>> be
>> > reused since its internal members will be reset in prepareMailbox for
>> next
>> > frame, so no need to remove it from the mailboxInfo vector, right?
>> >
>>
>
>  It can't be reused. There is no sync point to wait on to start using it,
>> so
>> reusing it could clobber it while the parent compositor's using it.
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:blink-reviews+unsubscribe@chromium.org.
>>
>
> But from current implementation of prepareMailbox, we also reset the
> syncPoint
> by insertSyncPoint for the mailbox returned from createMailboxInfo which
> firstly
> try to find the available mailbox to reuse, and from now on the parent
> compositor will see the new syncPoint for the next frame. Is there
> anything I am
> missing?
>

Insert sync point says "don't use the texture before this time". It does
nothing to prevent you from using the texture while someone else is using
it. You need to wait on their sync point to achieve that.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Hongbo Min, 2014-07-15 15:30:03]

On 2014/07/15 15:21:43, danakj wrote:
> On Tue, Jul 15, 2014 at 11:10 AM, <mailto:hongbo.min@intel.com> wrote:
> 
> > On 2014/07/15 14:54:11, danakj wrote:
> >
> >  On Tue, Jul 15, 2014 at 10:49 AM, <mailto:hongbo.min@intel.com> wrote:
> >>
> >
> >  > On 2014/07/15 14:38:29, junov wrote:
> >> >
> >> > https://codereview.chromium.org/369043003/diff/200001/
> >> > Source/platform/graphics/Canvas2DLayerBridge.cpp
> >> >
> >> >> File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):
> >> >>
> >> >
> >> >
> >> > https://codereview.chromium.org/369043003/diff/200001/
> >> > Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode518
> >> >
> >> >> Source/platform/graphics/Canvas2DLayerBridge.cpp:518:
> >> >> mailboxInfo->m_mailbox.syncPoint = 0;
> >> >> I think you should be doing more than this. According to Dana's earlier
> >> >>
> >> > comment,
> >> >
> >> >> we should avoid recycling a mailbox that is associated with a lost
> >> >> resource.
> >> >>
> >> > To
> >> >
> >> >> achieve that, you must remove mailboxInfo from m_mailboxes.  Dana, can
> >> you
> >> >> confirm that this is what you meant?
> >> >>
> >> >
> >> > @Dana, does it mean the mailboxInfo can not be reused if the resource is
> >> > lost
> >> > (not caused by context lost, just because the child client doesn't know
> >> how
> >> > service side handle it)? My understanding is, the mailboxInfo can still
> >> be
> >> > reused since its internal members will be reset in prepareMailbox for
> >> next
> >> > frame, so no need to remove it from the mailboxInfo vector, right?
> >> >
> >>
> >
> >  It can't be reused. There is no sync point to wait on to start using it,
> >> so
> >> reusing it could clobber it while the parent compositor's using it.
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:blink-reviews+unsubscribe@chromium.org.
> >>
> >
> > But from current implementation of prepareMailbox, we also reset the
> > syncPoint
> > by insertSyncPoint for the mailbox returned from createMailboxInfo which
> > firstly
> > try to find the available mailbox to reuse, and from now on the parent
> > compositor will see the new syncPoint for the next frame. Is there
> > anything I am
> > missing?
> >
> 
> Insert sync point says "don't use the texture before this time". It does
> nothing to prevent you from using the texture while someone else is using
> it. You need to wait on their sync point to achieve that.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

I see, thanks for your explanations, junov@ and Dana@, I will update the CL
again.

[Hongbo Min, 2014-07-15 15:52:46]

junov@, Dana@, the CL is updated at Patch Set 11. Please have a review. Thanks a
lot!

[Justin Novosad, 2014-07-15 16:12:03]

The canvas 2D parts look good to me, but let's wait for Ken's feedback on the
WebGL part. Perhaps you could split this into two changes.  The WebGL and 2D
canvas parts don't really depend on each other.

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:528: return;
More readable if you put the code below inside an "else", instead of having a
"return" here. But that is just my personal opinion on style. Feel free to
ignore.

[Ken Russell (switch to Gerrit), 2014-07-15 22:22:33]

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if (m_destructionInProgress
|| m_context->isContextLost() || lostResource) {
Could you please add a new test to DrawingBufferTest.cpp which verifies the
behavior when mailboxRelease is called with lostResource=true?

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
Why is this necessary? The call to m_textureMailboxes.remove(i), below, should
cause the MailboxInfo to be deleted and its m_parentDrawingBuffer field cleared.

[Hongbo Min, 2014-07-16 01:29:16]

On 2014/07/15 22:22:33, Ken Russell wrote:
>
https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
> File Source/platform/graphics/gpu/DrawingBuffer.cpp (right):
> 
>
https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:281: if
(m_destructionInProgress
> || m_context->isContextLost() || lostResource) {
> Could you please add a new test to DrawingBufferTest.cpp which verifies the
> behavior when mailboxRelease is called with lostResource=true?
> 
>
https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
> Source/platform/graphics/gpu/DrawingBuffer.cpp:357:
> m_textureMailboxes[i]->m_parentDrawingBuffer.clear();
> Why is this necessary? The call to m_textureMailboxes.remove(i), below, should
> cause the MailboxInfo to be deleted and its m_parentDrawingBuffer field
cleared.

Thanks for your comments. I am going to split the webgl part into another CL.

[Hongbo Min, 2014-07-16 01:32:20]

@kbr, per junov@ suggestion, the webgl part would be split into another new CL.
In the Patch Set 12, the changes to webgl is just to lostResource, but the code
path is not changed actually since it will return early if lostResource is true.

Next step I will submit a new CL for WebGL part plus the test cases.

@kbr, does this CL changes to webgl look good to you? Thanks!

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
File Source/platform/graphics/Canvas2DLayerBridge.cpp (right):

https://codereview.chromium.org/369043003/diff/220001/Source/platform/graphic...
Source/platform/graphics/Canvas2DLayerBridge.cpp:528: return;
On 2014/07/15 16:12:03, junov wrote:
> More readable if you put the code below inside an "else", instead of having a
> "return" here. But that is just my personal opinion on style. Feel free to
> ignore.

Done.

[Ken Russell (switch to Gerrit), 2014-07-16 01:42:03]

Yes, that sounds fine. LGTM

[Hongbo Min, 2014-07-16 02:46:54]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-16 02:47:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/240001

[Hongbo Min, 2014-07-16 02:54:49]

jamesr@, I need your lgtm for the change of WebExternalTextureLayerClient.h?
Could you please take a look? Thanks.

[commit-bot: I haz the power, 2014-07-16 03:58:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17151)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-16 04:01:29]

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

[commit-bot: I haz the power, 2014-07-16 04:01:31]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[jamesr, 2014-07-16 06:08:58]

/public/ lgtm

https://codereview.chromium.org/369043003/diff/240001/public/platform/WebExte...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/369043003/diff/240001/public/platform/WebExte...
public/platform/WebExternalTextureLayerClient.h:48: virtual void
mailboxReleased(const WebExternalTextureMailbox&, bool lostResource = false) =
0;
are you planning to remove the defaultness of this param when you land the
chromium side? if so leave yourself a FIXME indicating as such.

[jamesr, 2014-07-16 06:09:00]

/public/ lgtm

[Hongbo Min, 2014-07-16 06:25:24]

https://codereview.chromium.org/369043003/diff/240001/public/platform/WebExte...
File public/platform/WebExternalTextureLayerClient.h (right):

https://codereview.chromium.org/369043003/diff/240001/public/platform/WebExte...
public/platform/WebExternalTextureLayerClient.h:48: virtual void
mailboxReleased(const WebExternalTextureMailbox&, bool lostResource = false) =
0;
On 2014/07/16 06:08:58, jamesr wrote:
> are you planning to remove the defaultness of this param when you land the
> chromium side? if so leave yourself a FIXME indicating as such.

Sure. FIXME is added in the new PS.

[Hongbo Min, 2014-07-16 06:27:35]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-16 06:28:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/260001

[commit-bot: I haz the power, 2014-07-16 07:43:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15698)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17189)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/29156)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/33566)

[commit-bot: I haz the power, 2014-07-16 08:31:46]

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

[commit-bot: I haz the power, 2014-07-16 08:31:47]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15710)

[Hongbo Min, 2014-07-16 08:44:39]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-16 08:44:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/260001

[commit-bot: I haz the power, 2014-07-16 09:30:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15726)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/17222)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-16 09:53:24]

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

[commit-bot: I haz the power, 2014-07-16 09:53:26]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[dshwang, 2014-07-16 12:44:08]

Thank you for completing CL. non-owner lgtm too

[Hongbo Min, 2014-07-16 12:49:30]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-16 12:50:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/260001

[commit-bot: I haz the power, 2014-07-16 14:02:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/15758)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-16 15:10:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-16 15:11:18]

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

[commit-bot: I haz the power, 2014-07-16 15:11:20]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Ken Russell (switch to Gerrit), 2014-07-16 18:51:08]

On 2014/07/16 15:11:20, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
>   mac_gpu_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
>   win_gpu_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

These failures definitely look real and caused by this CL. It looks like the
tab's crashing running some canvas-related WebGL rendering tests. Please let me
know if you have any difficulty reproducing these failures locally.

[Hongbo Min, 2014-07-17 01:40:21]

On 2014/07/16 18:51:08, Ken Russell wrote:
> On 2014/07/16 15:11:20, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
> >   mac_gpu_triggered_tests on tryserver.chromium.gpu
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
> >   win_gpu_triggered_tests on tryserver.chromium.gpu
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
> 
> These failures definitely look real and caused by this CL. It looks like the
> tab's crashing running some canvas-related WebGL rendering tests. Please let
me
> know if you have any difficulty reproducing these failures locally.

kbr@, it seems the failures were happened when running webgl conformance on win
and mac:

https://www.khronos.org/registry/webgl/sdk/tests/conformance/canvas/draw-stat...
https://www.khronos.org/registry/webgl/sdk/tests/conformance/canvas/to-data-u...
 
Unfortunately, I don't have a win or mac machine on hand for use to reproduce
them.

For the code change for WebGL part, this CL actually doesn't change the code
path for mailboxReleased since the CL for chromium part
https://codereview.chromium.org/369823002/ is not landed yet. I am curious why
the conformance test fails.

[Hongbo Min, 2014-07-17 04:55:29]

On 2014/07/17 01:40:21, Hongbo Min wrote:
> On 2014/07/16 18:51:08, Ken Russell wrote:
> > On 2014/07/16 15:11:20, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
> > >   mac_gpu_triggered_tests on tryserver.chromium.gpu
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
> > >   win_gpu_triggered_tests on tryserver.chromium.gpu
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
> > 
> > These failures definitely look real and caused by this CL. It looks like the
> > tab's crashing running some canvas-related WebGL rendering tests. Please let
> me
> > know if you have any difficulty reproducing these failures locally.
> 
> kbr@, it seems the failures were happened when running webgl conformance on
win
> and mac:
> 
>
https://www.khronos.org/registry/webgl/sdk/tests/conformance/canvas/draw-stat...
>
https://www.khronos.org/registry/webgl/sdk/tests/conformance/canvas/to-data-u...
>  
> Unfortunately, I don't have a win or mac machine on hand for use to reproduce
> them.
> 
> For the code change for WebGL part, this CL actually doesn't change the code
> path for mailboxReleased since the CL for chromium part
> https://codereview.chromium.org/369823002/ is not landed yet. I am curious why
> the conformance test fails.

Well...the above 2 test also depends on Canvas 2D.

[Ken Russell (switch to Gerrit), 2014-07-17 18:29:29]

On 2014/07/17 04:55:29, Hongbo Min wrote:
> 
> Well...the above 2 test also depends on Canvas 2D.

Correct. Something has probably been broken in the Canvas 2D -> WebGL upload
path by your patch.

If you don't have any machines which can reproduce this failure, you could add
more logging to your CL and send it to the win_gpu and mac_gpu try servers to
try to narrow down what is going wrong.

[Hongbo Min, 2014-07-21 14:43:18]

@kbr, thanks for your information.

@junov, @kbr, right now the trybots for gpu tests on win and mac are green.

The change I did in the latest Patch Set 16 is, mailboxReleased would return
early in case of context is really lost. It seems that we don't need to touch
the mailboxInfo if the context is lost.

Could you please review it again for the new code? Thanks!

[Justin Novosad, 2014-07-21 16:46:13]

On 2014/07/21 14:43:18, Hongbo Min wrote:
> @kbr, thanks for your information.
> 
> @junov, @kbr, right now the trybots for gpu tests on win and mac are green.
> 
> The change I did in the latest Patch Set 16 is, mailboxReleased would return
> early in case of context is really lost. It seems that we don't need to touch
> the mailboxInfo if the context is lost.
> 
> Could you please review it again for the new code? Thanks!

That makes perfect sense to me. If the context was lost, the Canvas2DLayerBridge
will be torn down anyways.

lgtm

[Ken Russell (switch to Gerrit), 2014-07-21 19:18:23]

On 2014/07/21 14:43:18, Hongbo Min wrote:
> @kbr, thanks for your information.
> 
> @junov, @kbr, right now the trybots for gpu tests on win and mac are green.
> 
> The change I did in the latest Patch Set 16 is, mailboxReleased would return
> early in case of context is really lost. It seems that we don't need to touch
> the mailboxInfo if the context is lost.

What was causing the tab to crash in the earlier version of the CL? Do you have
a stack trace for the crash from the debugger?

I'm concerned that if the Canvas2DLayerBridge has already been released by the
time mailboxReleased is called, that the dereference of
this->m_contextProvider->context3d()->isContextLost() might accidentally
succeed, but is actually a bug.

[Hongbo Min, 2014-07-22 14:09:21]

On 2014/07/21 19:18:23, Ken Russell wrote:
> On 2014/07/21 14:43:18, Hongbo Min wrote:
> > @kbr, thanks for your information.
> > 
> > @junov, @kbr, right now the trybots for gpu tests on win and mac are green.
> > 
> > The change I did in the latest Patch Set 16 is, mailboxReleased would return
> > early in case of context is really lost. It seems that we don't need to
touch
> > the mailboxInfo if the context is lost.
> 
> What was causing the tab to crash in the earlier version of the CL? Do you
have
> a stack trace for the crash from the debugger?
> 
> I'm concerned that if the Canvas2DLayerBridge has already been released by the
> time mailboxReleased is called, that the dereference of
> this->m_contextProvider->context3d()->isContextLost() might accidentally
> succeed, but is actually a bug.

kbr@, I think the root cause for tab crash was identified though I didn't
capture the stack trace. It is mainly caused by an incorrect order of clearing
Canvas2DLayerBridge object.

The statement "mailboxInfo->m_parentLayerBridge.clear();" may trigger
Canvas2DLayerBridge self-destruction if its ref count is zero. If it is put
before mailbox cleanup, see Patch Set 13, the crash happens since the
Canvas2DLayerBridge object will be used after deleted.

Patch Set 17 show the fix to the crash, it puts the above statement right before
"return" statement from mailboxReleased method, and the trybots of GPU tests are
all green now.

So if the client code (mailboxReleased caller) doesn't reference
Canvas2DLayerBridge object again after mailboxReleased is called in case of the
context is lost, it is ok. Otherwise, the crash would still happen for context
lost case.

Therefore, for safety, it'd better not to touch mailboxInfo and return early if
the context is lost, as shown in Patch Set 15.

kbr@, does it look good to you now?

[Ken Russell (switch to Gerrit), 2014-07-22 18:36:37]

On 2014/07/22 14:09:21, Hongbo Min wrote:
> On 2014/07/21 19:18:23, Ken Russell wrote:
> > On 2014/07/21 14:43:18, Hongbo Min wrote:
> > > @kbr, thanks for your information.
> > > 
> > > @junov, @kbr, right now the trybots for gpu tests on win and mac are
green.
> > > 
> > > The change I did in the latest Patch Set 16 is, mailboxReleased would
return
> > > early in case of context is really lost. It seems that we don't need to
> touch
> > > the mailboxInfo if the context is lost.
> > 
> > What was causing the tab to crash in the earlier version of the CL? Do you
> have
> > a stack trace for the crash from the debugger?
> > 
> > I'm concerned that if the Canvas2DLayerBridge has already been released by
the
> > time mailboxReleased is called, that the dereference of
> > this->m_contextProvider->context3d()->isContextLost() might accidentally
> > succeed, but is actually a bug.
> 
> kbr@, I think the root cause for tab crash was identified though I didn't
> capture the stack trace. It is mainly caused by an incorrect order of clearing
> Canvas2DLayerBridge object.
> 
> The statement "mailboxInfo->m_parentLayerBridge.clear();" may trigger
> Canvas2DLayerBridge self-destruction if its ref count is zero. If it is put
> before mailbox cleanup, see Patch Set 13, the crash happens since the
> Canvas2DLayerBridge object will be used after deleted.
> 
> Patch Set 17 show the fix to the crash, it puts the above statement right
before
> "return" statement from mailboxReleased method, and the trybots of GPU tests
are
> all green now.
> 
> So if the client code (mailboxReleased caller) doesn't reference
> Canvas2DLayerBridge object again after mailboxReleased is called in case of
the
> context is lost, it is ok. Otherwise, the crash would still happen for context
> lost case.
> 
> Therefore, for safety, it'd better not to touch mailboxInfo and return early
if
> the context is lost, as shown in Patch Set 15.
> 
> kbr@, does it look good to you now?

I defer review of Canvas2DLayerBridge to junov@. Please just ensure that the
layout tests and tests on the GPU bots are all green with your change.

[Justin Novosad, 2014-07-22 20:33:37]

On 2014/07/22 18:36:37, Ken Russell wrote:

> 
> I defer review of Canvas2DLayerBridge to junov@. Please just ensure that the
> layout tests and tests on the GPU bots are all green with your change.

re lgtm for Canvas2DLayerBridge.
Clearing the self reference at the very end is definitely the right thing to do.

[Hongbo Min, 2014-07-23 01:09:06]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-23 01:10:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/460001

[Hongbo Min, 2014-07-23 01:10:16]

The CQ bit was unchecked by hongbo.min@intel.com

[Hongbo Min, 2014-07-23 01:15:53]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-23 01:17:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/480001

[commit-bot: I haz the power, 2014-07-23 03:11:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18409)

[commit-bot: I haz the power, 2014-07-23 04:13:19]

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

[commit-bot: I haz the power, 2014-07-23 04:13:22]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/18432)

[Hongbo Min, 2014-07-23 05:17:34]

The CQ bit was checked by hongbo.min@intel.com

[commit-bot: I haz the power, 2014-07-23 05:17:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hongbo.min@intel.com/369043003/480001

[commit-bot: I haz the power, 2014-07-23 05:48:05]

Change committed as 178736