Pepper takes ownership of a mailbox before passing it to the texture layer.

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

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

[erikchen, 2016-04-21 20:56:05]

Description was changed from

==========
Pepper takes ownership of a mailbox before passing it to the texture layer.

This

BUG=
==========

to

==========
Pepper takes ownership of a mailbox before passing it to the texture layer.

This

BUG=
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-04-21 20:59:35]

Description was changed from

==========
Pepper takes ownership of a mailbox before passing it to the texture layer.

This

BUG=
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

==========
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
==========

[erikchen, 2016-04-22 01:29:03]

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

[commit-bot: I haz the power, 2016-04-22 01:29:18]

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

[commit-bot: I haz the power, 2016-04-22 01:30:58]

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

[commit-bot: I haz the power, 2016-04-22 01:30:59]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_optional_gpu_tests_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[erikchen, 2016-04-22 01:36:35]

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

[commit-bot: I haz the power, 2016-04-22 01:36:55]

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

[commit-bot: I haz the power, 2016-04-22 01:39:18]

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

[commit-bot: I haz the power, 2016-04-22 01:39:18]

Dry run: Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-04-22 01:46:18]

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

[commit-bot: I haz the power, 2016-04-22 01:46:43]

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

[commit-bot: I haz the power, 2016-04-22 01:56:57]

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

[commit-bot: I haz the power, 2016-04-22 01:56:58]

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

[erikchen, 2016-04-22 02:02:20]

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

[commit-bot: I haz the power, 2016-04-22 02:02:43]

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

[commit-bot: I haz the power, 2016-04-22 02:35:12]

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

[commit-bot: I haz the power, 2016-04-22 02:35:13]

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

[erikchen, 2016-04-22 04:45:06]

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

[commit-bot: I haz the power, 2016-04-22 04:45:13]

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

[erikchen, 2016-04-22 04:45:23]

erikchen@chromium.org changed reviewers:
+ ccameron@chromium.org

[erikchen, 2016-04-22 04:45:24]

ccameron: Please review.

[commit-bot: I haz the power, 2016-04-22 06:30:19]

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

[commit-bot: I haz the power, 2016-04-22 06:30:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2016-04-22 18:25:55]

This lg overall.

That said, there may be subtleties (particularly to the command buffer stuff)
that I may have missed.

https://codereview.chromium.org/1912833002/diff/160001/cc/layers/texture_laye...
File cc/layers/texture_layer.cc (left):

https://codereview.chromium.org/1912833002/diff/160001/cc/layers/texture_laye...
cc/layers/texture_layer.cc:157: static void IgnoreReleaseCallback(const
gpu::SyncToken& sync_token, bool lost) {
:D

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2033: //
offscreen_target_stencil_render_buffer_ is unused.
<idle complaining>
GLES2CmdDecoder is far too large a struct, and all of these backbuffer
management things would be good candidates for moving to a separate file.
</idle complaining>

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4331: NOTREACHED();
Should this be "TODO: delete me" as well?

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4381: Texture* texture =
mailbox_manager()->ConsumeTexture(mailbox);
Pull the ConsumeTexture above the loop?

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4391: it->in_use = false;
Are there circumstances where we would be concerned about the growth of
non-in_use saved_back_textures_?

[erikchen, 2016-04-25 20:58:24]

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4381: Texture* texture =
mailbox_manager()->ConsumeTexture(mailbox);
On 2016/04/22 18:25:55, ccameron wrote:
> Pull the ConsumeTexture above the loop?

Done.

https://codereview.chromium.org/1912833002/diff/160001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4391: it->in_use = false;
On 2016/04/22 18:25:55, ccameron wrote:
> Are there circumstances where we would be concerned about the growth of
> non-in_use saved_back_textures_?

If this becomes a problem, we can deal with it. I'd like to avoid premature
optimization.

[This is the same thing we do for WebGL].

[erikchen, 2016-04-25 20:58:32]

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

[commit-bot: I haz the power, 2016-04-25 20:59:10]

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

[ccameron, 2016-04-25 20:59:41]

Sounds fair, lgtm.

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

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

[commit-bot: I haz the power, 2016-04-25 21:18:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-25 21:40:23]

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

[commit-bot: I haz the power, 2016-04-25 21:41:11]

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

[commit-bot: I haz the power, 2016-04-25 23:09:08]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-26 00:04:59]

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

[commit-bot: I haz the power, 2016-04-26 00:05:22]

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

[erikchen, 2016-04-26 00:54:27]

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

[erikchen, 2016-04-26 00:54:27]

piman: Please review.

[commit-bot: I haz the power, 2016-04-26 01:09:19]

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

[commit-bot: I haz the power, 2016-04-26 01:09:20]

Dry run: This issue passed the CQ dry run.

[piman, 2016-04-26 01:54:01]

https://codereview.chromium.org/1912833002/diff/220001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:267: *mailbox =
gpu::Mailbox::Generate();
This is kind of costly (generate a crypto-secure number). Also, the
MailboxManager keeps a list of all mailboxes associated with a TextureRef, so
this would cause a logical leak. Could we keep a queue of these, recycling in
ReturnBackingMailbox?

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4252: LOG(ERROR) << "Called
TakeFrontBuffer on a non-offscreen context";
nit: DLOG

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4297: NOTREACHED();
nit: Not NOTREACHED(), this could be reached from a malicious client. It's ok if
it hoses its own state, but we shouldn't make the assumption that this can't
happen.

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.h:164: virtual void
ReleaseSurface() = 0;
Is this a leftover from a merge conflict? (This was added in a patch that landed
and was reverted a couple of times - https://codereview.chromium.org/1714883002/
- but it doesn't seem related to your change)

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:624: lock.reset(new
base::AutoLock(*lock_));
These should be CheckLock(). The client is responsible for taking the lock (if
needed). The lock is only acquired inside of CommandBufferProxyImpl for tasks
posted by CommandBufferProxyImpl itself, or the channel (e.g.
OnMessageReceived).

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:630: uint64_t fence =
GenerateFenceSyncRelease();
This will not do the right thing for Pepper, where the namespace for fences (and
sync tokens) is handled by PpapiCommandBufferProxy on the plugin side. This will
cause out-of-order fences, which will cause synchronization issues.

I think the way to approach the problem is to do the TakeFrontBuffer from the
plugin side: in Graphics3D::DoSwapBuffers, do the TakeFrontBuffer, pass the
mailbox and sync token to the renderer side (add the former to the
PpapiHostMsg_PPBGraphics3D_SwapBuffers message).
An argument could be made for doing the ReturnFrontBuffer there too, but that's
not strictly necessary.

[erikchen, 2016-04-27 01:31:35]

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

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

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

[erikchen, 2016-04-27 02:00:42]

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

[commit-bot: I haz the power, 2016-04-27 02:00:55]

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

[commit-bot: I haz the power, 2016-04-27 03:45:33]

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

[commit-bot: I haz the power, 2016-04-27 03:45:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-27 16:20:22]

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

[commit-bot: I haz the power, 2016-04-27 16:20:38]

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

[erikchen, 2016-04-27 16:31:24]

piman: PTAL

https://codereview.chromium.org/1912833002/diff/220001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:267: *mailbox =
gpu::Mailbox::Generate();
On 2016/04/26 01:54:01, piman wrote:
> This is kind of costly (generate a crypto-secure number). Also, the
> MailboxManager keeps a list of all mailboxes associated with a TextureRef, so
> this would cause a logical leak. Could we keep a queue of these, recycling in
> ReturnBackingMailbox?

Done.

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4252: LOG(ERROR) << "Called
TakeFrontBuffer on a non-offscreen context";
On 2016/04/26 01:54:01, piman wrote:
> nit: DLOG

Done.

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4297: NOTREACHED();
On 2016/04/26 01:54:01, piman wrote:
> nit: Not NOTREACHED(), this could be reached from a malicious client. It's ok
if
> it hoses its own state, but we shouldn't make the assumption that this can't
> happen.

This seems like the perfect place for NOTREACHED()?

If a Chrome developer misuses ReturnFrontBuffer in a DEBUG build, the assertion
will fire.
In a Release build, a malicious client will hit this logic and nothing will
happen.

If you prefer, I can switch to a DLOG but LOG messages are easily overlooked.

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.h (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.h:164: virtual void
ReleaseSurface() = 0;
On 2016/04/26 01:54:01, piman wrote:
> Is this a leftover from a merge conflict? (This was added in a patch that
landed
> and was reverted a couple of times -
https://codereview.chromium.org/1714883002/
> - but it doesn't seem related to your change)

Whoops! Removed

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:624: lock.reset(new
base::AutoLock(*lock_));
On 2016/04/26 01:54:01, piman wrote:
> These should be CheckLock(). The client is responsible for taking the lock (if
> needed). The lock is only acquired inside of CommandBufferProxyImpl for tasks
> posted by CommandBufferProxyImpl itself, or the channel (e.g.
> OnMessageReceived).

Done.

https://codereview.chromium.org/1912833002/diff/220001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:630: uint64_t fence =
GenerateFenceSyncRelease();
On 2016/04/26 01:54:01, piman wrote:
> This will not do the right thing for Pepper, where the namespace for fences
(and
> sync tokens) is handled by PpapiCommandBufferProxy on the plugin side. This
will
> cause out-of-order fences, which will cause synchronization issues.
> 
> I think the way to approach the problem is to do the TakeFrontBuffer from the
> plugin side: in Graphics3D::DoSwapBuffers, do the TakeFrontBuffer, pass the
> mailbox and sync token to the renderer side (add the former to the
> PpapiHostMsg_PPBGraphics3D_SwapBuffers message).
> An argument could be made for doing the ReturnFrontBuffer there too, but
that's
> not strictly necessary.

Got it. I now generate the sync token on the pepper side, but I still had to
plumb through TakeFrontBuffer to the renderer so that the Mailboxes can be
reused in ReturnFrontBuffer.

[commit-bot: I haz the power, 2016-04-27 17:42:19]

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

[commit-bot: I haz the power, 2016-04-27 17:42:21]

Dry run: This issue passed the CQ dry run.

[piman, 2016-04-27 23:00:58]

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4297: NOTREACHED();
On 2016/04/27 16:31:23, erikchen wrote:
> On 2016/04/26 01:54:01, piman wrote:
> > nit: Not NOTREACHED(), this could be reached from a malicious client. It's
ok
> if
> > it hoses its own state, but we shouldn't make the assumption that this can't
> > happen.
> 
> This seems like the perfect place for NOTREACHED()?
> 
> If a Chrome developer misuses ReturnFrontBuffer in a DEBUG build, the
assertion
> will fire.
> In a Release build, a malicious client will hit this logic and nothing will
> happen.
> 
> If you prefer, I can switch to a DLOG but LOG messages are easily overlooked.

I insist :) This is consistent in GPU code.
DCHECK/NOTREACHED is there for documentation purpose (as you mention, nothing
will happen with it in production). But it would be documenting the wrong thing
("this can't happen" - it's not true, it can happen, and we need to be aware
that it can). The next person refactoring this code needs to not make the wrong
assumption. E.g. just looking at this function without context one could rewrite
it as:

void GLES2DecoderImpl::ReturnFrontBuffer(const Mailbox& mailbox,
                                         bool is_lost) {	
  Texture* texture = mailbox_manager()->ConsumeTexture(mailbox);
  auto it =
      std::find_if(saved_back_textures_.begin(), saved_back_textures_.end(),
                   [texture](const SavedBackTexture& saved) {
                     return saved.texture_ref->texture() == texture;
                   });

  DCHECK(it != saved_back_textures_.end());  // Surely this is equivalent to the
previous NOTREACHED()
  if (is_lost || it->back_texture->size() != offscreen_size_) {
    it->back_texture->Invalidate();
    saved_back_textures_.erase(it);
  } else {
    it->in_use = false;
  }
}

However this would introduce a security issue (dereferencing out-of-bounds) on
invalid input.

If we want to check the client behavior, we should DCHECK/NOTREACHED it on the
client side, and/or write a test.
Replacing with a DLOG is ok (no LOG please).

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:620: void
CommandBufferProxyImpl::TakeFrontBuffer(const gpu::Mailbox& mailbox) {
nit: CheckLock();

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:629: bool is_lost) {
nit: CheckLock();

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:119:
DCHECK(taken_front_buffer_.IsZero());
This DCHECK is not valid. The plugin side may be untrusted (NaCl plugin) and
could call TakeFrontBuffer several times without a SwapBuffers.

It's ok to ignore redundant calls.

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:160:
DCHECK(!taken_front_buffer_.IsZero());
Here too, the DCHECK is invalid.

It's ok to return failure.

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:356: if
(mailboxes_to_reuse_.size()) {
nit: !mailboxes_to_reuse_.empty()

[erikchen, 2016-04-27 23:59:24]

piman: PTAL

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1912833002/diff/220001/gpu/command_buffer/ser...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4297: NOTREACHED();
On 2016/04/27 23:00:58, piman wrote:
> On 2016/04/27 16:31:23, erikchen wrote:
> > On 2016/04/26 01:54:01, piman wrote:
> > > nit: Not NOTREACHED(), this could be reached from a malicious client. It's
> ok
> > if
> > > it hoses its own state, but we shouldn't make the assumption that this
can't
> > > happen.
> > 
> > This seems like the perfect place for NOTREACHED()?
> > 
> > If a Chrome developer misuses ReturnFrontBuffer in a DEBUG build, the
> assertion
> > will fire.
> > In a Release build, a malicious client will hit this logic and nothing will
> > happen.
> > 
> > If you prefer, I can switch to a DLOG but LOG messages are easily
overlooked.
> 
> I insist :) This is consistent in GPU code.
> DCHECK/NOTREACHED is there for documentation purpose (as you mention, nothing
> will happen with it in production). But it would be documenting the wrong
thing
> ("this can't happen" - it's not true, it can happen, and we need to be aware
> that it can). The next person refactoring this code needs to not make the
wrong
> assumption. E.g. just looking at this function without context one could
rewrite
> it as:
> 
> void GLES2DecoderImpl::ReturnFrontBuffer(const Mailbox& mailbox,
>                                          bool is_lost) {	
>   Texture* texture = mailbox_manager()->ConsumeTexture(mailbox);
>   auto it =
>       std::find_if(saved_back_textures_.begin(), saved_back_textures_.end(),
>                    [texture](const SavedBackTexture& saved) {
>                      return saved.texture_ref->texture() == texture;
>                    });
> 
>   DCHECK(it != saved_back_textures_.end());  // Surely this is equivalent to
the
> previous NOTREACHED()
>   if (is_lost || it->back_texture->size() != offscreen_size_) {
>     it->back_texture->Invalidate();
>     saved_back_textures_.erase(it);
>   } else {
>     it->in_use = false;
>   }
> }
> 
> However this would introduce a security issue (dereferencing out-of-bounds) on
> invalid input.
> 
> If we want to check the client behavior, we should DCHECK/NOTREACHED it on the
> client side, and/or write a test.
> Replacing with a DLOG is ok (no LOG please).

Point taken. Thanks for the example. Switched to a DLOG.

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:620: void
CommandBufferProxyImpl::TakeFrontBuffer(const gpu::Mailbox& mailbox) {
On 2016/04/27 23:00:58, piman wrote:
> nit: CheckLock();

Done

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:629: bool is_lost) {
On 2016/04/27 23:00:58, piman wrote:
> nit: CheckLock();

Done.

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:119:
DCHECK(taken_front_buffer_.IsZero());
On 2016/04/27 23:00:58, piman wrote:
> This DCHECK is not valid. The plugin side may be untrusted (NaCl plugin) and
> could call TakeFrontBuffer several times without a SwapBuffers.
> 
> It's ok to ignore redundant calls.

Done.

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:160:
DCHECK(!taken_front_buffer_.IsZero());
On 2016/04/27 23:00:58, piman wrote:
> Here too, the DCHECK is invalid.
> 
> It's ok to return failure.

Done.

https://codereview.chromium.org/1912833002/diff/320001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:356: if
(mailboxes_to_reuse_.size()) {
On 2016/04/27 23:00:58, piman wrote:
> nit: !mailboxes_to_reuse_.empty()

Done.

[piman, 2016-04-28 02:15:05]

Missing a couple of changes, I think.
LGTM after they're added.

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:620: void
CommandBufferProxyImpl::TakeFrontBuffer(const gpu::Mailbox& mailbox) {
On 2016/04/27 23:59:24, erikchen wrote:
> On 2016/04/27 23:00:58, piman wrote:
> > nit: CheckLock();
> 
> Done

I don't see it in the latest PS (PS#18)

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:629: bool is_lost) {
On 2016/04/27 23:59:24, erikchen wrote:
> On 2016/04/27 23:00:58, piman wrote:
> > nit: CheckLock();
> 
> Done.

ditto

[erikchen, 2016-04-28 17:07:23]

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
File gpu/ipc/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/240001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:620: void
CommandBufferProxyImpl::TakeFrontBuffer(const gpu::Mailbox& mailbox) {
On 2016/04/28 02:15:05, piman wrote:
> On 2016/04/27 23:59:24, erikchen wrote:
> > On 2016/04/27 23:00:58, piman wrote:
> > > nit: CheckLock();
> > 
> > Done
> 
> I don't see it in the latest PS (PS#18)

I do see it.
https://codereview.chromium.org/1912833002/diff2/240001:330001/gpu/ipc/client...

It's been there since PS 15.

[erikchen, 2016-04-28 17:09:38]

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

[erikchen, 2016-04-28 17:09:40]

bbudge: Please review  ppapi/thunk/ppb_graphics_3d_api.h.

tsepez: Please review ppapi/proxy/ppapi_messages.h and
gpu/ipc/common/gpu_messages.h

sky: Please review components/mus

[Tom Sepez, 2016-04-28 17:48:05]

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1048: // by exactly one call to
PpapiHostMsg_PPBGraphics3D_TakeFrontBuffer. The
What happens otherwise?  Can a evil/misbehaving ppapi process crash its host if
it violates this?

[erikchen, 2016-04-28 17:50:10]

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1048: // by exactly one call to
PpapiHostMsg_PPBGraphics3D_TakeFrontBuffer. The
On 2016/04/28 17:48:04, Tom Sepez wrote:
> What happens otherwise?  Can a evil/misbehaving ppapi process crash its host
if
> it violates this?

Nope. The host will gracefully ignore the message and emit a DLOG:

https://codereview.chromium.org/1912833002/diff/330001/content/renderer/peppe...

See the first lines of:
PPB_Graphics3D_Impl::TakeFrontBuffer()
PPB_Graphics3D_Impl::DoSwapBuffers()

[bbudge, 2016-04-28 20:13:27]

LGTM w/nits

https://codereview.chromium.org/1912833002/diff/330001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/330001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:365:
mailboxes_to_reuse_.erase(mailboxes_to_reuse_.begin());
nit: Is there any reason not to take from the end for constant time complexity,
i.e.

gpu::Mailbox mailbox = mailboxes_to_reuse_.back();
mailboxes_to_reuse_.pop_back();

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1056: 
nit: stray whitespace?

[Tom Sepez, 2016-04-28 20:48:27]

lgtm

[sky, 2016-04-29 00:30:53]

mus LGTM

[erikchen, 2016-04-29 00:38:06]

https://codereview.chromium.org/1912833002/diff/330001/content/renderer/peppe...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/1912833002/diff/330001/content/renderer/peppe...
content/renderer/pepper/ppb_graphics_3d_impl.cc:365:
mailboxes_to_reuse_.erase(mailboxes_to_reuse_.begin());
On 2016/04/28 20:13:27, bbudge wrote:
> nit: Is there any reason not to take from the end for constant time
complexity,
> i.e.
> 
> gpu::Mailbox mailbox = mailboxes_to_reuse_.back();
> mailboxes_to_reuse_.pop_back();

Done.

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1912833002/diff/330001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:1056: 
On 2016/04/28 20:13:27, bbudge wrote:
> nit: stray whitespace?

removed.

[erikchen, 2016-04-29 00:38:18]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-04-29 00:38:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, tsepez@chromium.org, bbudge@chromium.org,
sky@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1912833002/#ps350001
(title: "Comments from bbudge.")

[commit-bot: I haz the power, 2016-04-29 00:38:56]

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

[commit-bot: I haz the power, 2016-04-29 01:54:52]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2016-04-30 17:23:42]

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

[commit-bot: I haz the power, 2016-04-30 17:59:00]

Description was changed from

==========
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

==========
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

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

[erikchen, 2016-05-02 16:45:19]

A revert of this CL (patchset #19 id:350001) has been created in
https://codereview.chromium.org/1937173002/ by erikchen@chromium.org.

The reason for reverting is: Top crasher on renderer and gpu processes:

https://bugs.chromium.org/p/chromium/issues/detail?id=608163.