[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture layer.

[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now processed in the context of its own message:
WaitSyncToken.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, sky@chromium.org

Committed: https://crrev.com/4734ed1b6740b772201b8accb3e2bd88cdea5c99
Cr-Commit-Position: refs/heads/master@{#391686}

[erikchen, 2016-05-02 19:29:14]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
TakeFrontBuffer gives ownership of the front buffer to the client. When the
client returns it with ReturnFrontBuffer, the command buffer may choose to reuse
it.

This means that pepper no longer needs to use
SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/1912833002
Cr-Commit-Position: refs/heads/master@{#390570}
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
TakeFrontBuffer gives ownership of the front buffer to the client. When the
client returns it with ReturnFrontBuffer, the command buffer may choose to reuse
it.

This means that pepper no longer needs to use
SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-05-02 22:04:41]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2016-05-02 22:04:42]

lgtm

[erikchen, 2016-05-02 22:39:26]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-02 22:39:44]

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

[erikchen, 2016-05-02 22:41:41]

piman: I added a test in patch set 3 which catches the bug.

[commit-bot: I haz the power, 2016-05-02 22:42:40]

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

[commit-bot: I haz the power, 2016-05-02 22:42:41]

Dry run: Try jobs failed on following builders:
  win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)

[erikchen, 2016-05-02 22:45:47]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
TakeFrontBuffer gives ownership of the front buffer to the client. When the
client returns it with ReturnFrontBuffer, the command buffer may choose to reuse
it.

This means that pepper no longer needs to use
SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[erikchen, 2016-05-02 22:49:44]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=ccameron@chromium.org, bbudge@chromium.org, tsepez@chromium.org,
sky@chromium.org
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

[erikchen, 2016-05-02 22:50:50]

erikchen@chromium.org changed reviewers:
+ bbudge@chromium.org, ccameron@chromium.org, sky@chromium.org,
tsepez@chromium.org

[erikchen, 2016-05-02 22:50:51]

TBRing ccameron@chromium.org, bbudge@chromium.org, tsepez@chromium.org,
sky@chromium.org since all changes were made in files owned by piman@.

[piman, 2016-05-02 22:59:13]

lgtm

[erikchen, 2016-05-03 00:06:04]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-05-03 00:06:40]

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

[erikchen, 2016-05-03 00:07:30]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2016-05-03 01:27:33]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=ccameron@chromium.org, bbudge@chromium.org, tsepez@chromium.org,
sky@chromium.org
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

[erikchen, 2016-05-03 01:28:40]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 01:29:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943513002/60001

[erikchen, 2016-05-03 01:50:11]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

to

==========
[Not ready for review] [Reland 1] Pepper takes ownership of a mailbox before
passing it to the texture layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

[erikchen, 2016-05-03 02:24:12]

Description was changed from

==========
[Not ready for review] [Reland 1] Pepper takes ownership of a mailbox before
passing it to the texture layer.

This CL makes two changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now sent and processed asynchronously by
PPB_Graphics3D_Impl. Calling OnWaitFenceSync() from GpuCommandBufferStub in the
context of another message was incorrect.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, tsepez@chromium.org,
sky@chromium.org
==========

[erikchen, 2016-05-03 02:24:24]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 02:25:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943513002/80001

[erikchen, 2016-05-03 02:26:17]

piman: Please review, again [I fixed a SyncToken processing bug].

This time, I manually tested against videos from cnn.com, ted.com, msn.com, and
dailymail.co.uk and everything seems to be working correctly.

[piman, 2016-05-03 02:52:04]

https://codereview.chromium.org/1943513002/diff/40001/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1943513002/diff/40001/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:732: sync_token.release_count());
Oh, duh, sorry I missed this. OnWaitFenceSync can't work, we need to go back to
the message loop.

Doing the round trip with SignalSyncToken works, but the extra latency means
that we're going to use more memory, because we'd probably squeeze an extra
SwapBuffers (or 2) before the actual ReturnFrontBuffer happens.

Really what we need is something akin to GpuCommandBufferStub::OnWaitFenceSync,
where we'd do ReturnFrontBuffer from the callback.

Another way is to introduce a WaitSyncToken IPC that behaves just like the
WaitSyncToken command (but could be done from the renderer side) - i.e. just
call OnWaitFenceSync. From the renderer you'd send WaitSyncToken then
ReturnFrontBuffer, that would ensure the ReturnFrontBuffer isn't processed until
the fence has passed.

WDYT?

[commit-bot: I haz the power, 2016-05-03 03:49:55]

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

[commit-bot: I haz the power, 2016-05-03 03:49:56]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-05-03 17:22:09]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now sent and processed asynchronously by
PPB_Graphics3D_Impl. Calling OnWaitFenceSync() from GpuCommandBufferStub in the
context of another message was incorrect.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, tsepez@chromium.org,
sky@chromium.org
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now processed in the context of its own message:
WaitSyncToken.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, sky@chromium.org
==========

[erikchen, 2016-05-03 17:24:14]

piman: PTAL

https://codereview.chromium.org/1943513002/diff/40001/gpu/ipc/service/gpu_com...
File gpu/ipc/service/gpu_command_buffer_stub.cc (right):

https://codereview.chromium.org/1943513002/diff/40001/gpu/ipc/service/gpu_com...
gpu/ipc/service/gpu_command_buffer_stub.cc:732: sync_token.release_count());
On 2016/05/03 02:52:04, piman wrote:
> Oh, duh, sorry I missed this. OnWaitFenceSync can't work, we need to go back
to
> the message loop.
> 
> Doing the round trip with SignalSyncToken works, but the extra latency means
> that we're going to use more memory, because we'd probably squeeze an extra
> SwapBuffers (or 2) before the actual ReturnFrontBuffer happens.
> 
> Really what we need is something akin to
GpuCommandBufferStub::OnWaitFenceSync,
> where we'd do ReturnFrontBuffer from the callback.
> 
> Another way is to introduce a WaitSyncToken IPC that behaves just like the
> WaitSyncToken command (but could be done from the renderer side) - i.e. just
> call OnWaitFenceSync. From the renderer you'd send WaitSyncToken then
> ReturnFrontBuffer, that would ensure the ReturnFrontBuffer isn't processed
until
> the fence has passed.
> 
> WDYT?

Ah, good point. I went with your second approach and defined a new IPC message
WaitSyncToken.

https://codereview.chromium.org/1943513002/diff/100001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (left):

https://codereview.chromium.org/1943513002/diff/100001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:243: DCHECK(scheduled_);
This assertion is no longer valid, since OnWaitSyncToken can leave the channel
descheduled.

[erikchen, 2016-05-03 17:53:02]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 17:53:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943513002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943513002/100001

[Tom Sepez, 2016-05-03 18:01:10]

RS LGTM

[commit-bot: I haz the power, 2016-05-03 19:55:29]

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

[commit-bot: I haz the power, 2016-05-03 19:55:31]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-05-03 20:03:37]

On 2016/05/03 19:55:31, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

tsepez: Could I get a real review? I added a new IPC message
GpuCommandBufferMsg_WaitSyncToken

[piman, 2016-05-03 20:57:50]

piman@chromium.org changed reviewers:
+ sunnyps@chromium.org

[piman, 2016-05-03 20:57:52]

+sunnyps in case I'm saying something dumb

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (left):

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:243: DCHECK(scheduled_);
Oh, this can't go away. Actually we need to not get here. 
The logic in GpuChannel::HandleMessage needs to go to the PauseMessageProcessing
path. Let me comment there.

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (right):

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:794: if (stub && stub->HasUnprocessedCommands())
{
I think the stub->HasUnprocessedCommands() check should be replaced by
!stub->IsScheduled(), and the DCHECK below account for both AsyncFlush as well
as WaitSyncToken.

[sunnyps, 2016-05-03 21:17:10]

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (left):

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:243: DCHECK(scheduled_);
On 2016/05/03 20:57:52, piman wrote:
> Oh, this can't go away. Actually we need to not get here. 
> The logic in GpuChannel::HandleMessage needs to go to the
PauseMessageProcessing
> path. Let me comment there.

Yes, this DCHECK is needed. We shouldn't finish a message if we get
descheduled(fence sync)/preempted.

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (right):

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:794: if (stub && stub->HasUnprocessedCommands())
{
On 2016/05/03 20:57:52, piman wrote:
> I think the stub->HasUnprocessedCommands() check should be replaced by
> !stub->IsScheduled(), and the DCHECK below account for both AsyncFlush as well
> as WaitSyncToken.

Yeah, the previous assumption was that the stub only gets descheduled while
running commands (AsyncFlush) but with the WaitFenceSync IPC this is no longer
true. This should be changed to message_queue->scheduled() - a good followup
would be to merge the PauseMessageProcessing/FinishMessageProcessing methods.
Also we do want to keep the DCHECK for AsyncFlush if
stub->HasUnprocessedCommands.

[erikchen, 2016-05-04 16:20:35]

sunnyps, piman: PTAL

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
File gpu/ipc/service/gpu_channel.cc (right):

https://codereview.chromium.org/1943513002/diff/120001/gpu/ipc/service/gpu_ch...
gpu/ipc/service/gpu_channel.cc:794: if (stub && stub->HasUnprocessedCommands())
{
On 2016/05/03 21:17:10, sunnyps wrote:
> On 2016/05/03 20:57:52, piman wrote:
> > I think the stub->HasUnprocessedCommands() check should be replaced by
> > !stub->IsScheduled(), and the DCHECK below account for both AsyncFlush as
well
> > as WaitSyncToken.
> 
> Yeah, the previous assumption was that the stub only gets descheduled while
> running commands (AsyncFlush) but with the WaitFenceSync IPC this is no longer
> true. This should be changed to message_queue->scheduled() - a good followup
> would be to merge the PauseMessageProcessing/FinishMessageProcessing methods.
> Also we do want to keep the DCHECK for AsyncFlush if
> stub->HasUnprocessedCommands.

I replaced the conditional with if (!message_queue->IsScheduled()) { and updated
the DCHECK.

[piman, 2016-05-04 21:25:40]

lgtm

[Tom Sepez, 2016-05-04 21:47:11]

On 2016/05/04 21:25:40, piman wrote:
> lgtm

LGTM, a misbehaving renderer can already block the GPU in other ways, and its a
DOS, not a security bug to do so.

[erikchen, 2016-05-04 21:48:14]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2016-05-04 21:48:37]

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

[commit-bot: I haz the power, 2016-05-04 23:23:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-04 23:25:26]

Description was changed from

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now processed in the context of its own message:
WaitSyncToken.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, sky@chromium.org
==========

to

==========
[Reland 1] Pepper takes ownership of a mailbox before passing it to the texture
layer.

This CL makes three changes from the original.

1. Replace a call to std::remove_if() with vec.erase(std::remove_if(), ...).
This was a logic error in the original CL that caused a crash any time the size
of the buffer was changed. This CL also adds a test that catches this bug.

2. Add some simple reference counting to PepperPluginInstanceImpl to track the
fact that a cc::TextureMailbox may be passed to |texture_layer_| more than once.

3. The SyncToken signal is now processed in the context of its own message:
WaitSyncToken.

> I replaced the IPC message GpuCommandBufferMsg_ProduceFrontBuffer with
> GpuCommandBufferMsg_TakeFrontBuffer and GpuCommandBufferMsg_ReturnFrontBuffer.
> TakeFrontBuffer gives ownership of the front buffer to the client. When the
> client returns it with ReturnFrontBuffer, the command buffer may choose to
reuse
> it.
>
> This means that pepper no longer needs to use
> SetTextureMailboxWithoutReleaseCallback.

BUG=350204, 602484
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
TEST=Run Chromium with the Pepper Flash plugin. Visit a site that supports Flash
video, such as http://vudu.com. Start playing a video, and then fullscreen the
video. Observe that Chromium does not crash. Please extensively test Chromium on
Flash 3D games and Flash video and make sure nothing else is working
incorrectly.
TBR=ccameron@chromium.org, bbudge@chromium.org, sky@chromium.org

Committed: https://crrev.com/4734ed1b6740b772201b8accb3e2bd88cdea5c99
Cr-Commit-Position: refs/heads/master@{#391686}
==========

[commit-bot: I haz the power, 2016-05-04 23:25:27]

Patchset 8 (id:??) landed as
https://crrev.com/4734ed1b6740b772201b8accb3e2bd88cdea5c99
Cr-Commit-Position: refs/heads/master@{#391686}

[ilja, 2016-05-10 02:17:55]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1964793002/ by ihf@chromium.org.

The reason for reverting is: Breaks WebGL on CrOS, crbug.com/610411..