gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/73794caeafd8fb67b345eae7cea31763274beaec
Cr-Commit-Position: refs/heads/master@{#434884}

[sadrul, 2016-11-27 01:08:34]

Description was changed from

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=none
==========

to

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sadrul, 2016-11-27 01:08:44]

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

[commit-bot: I haz the power, 2016-11-27 01:08:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-27 01:12:07]

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

[commit-bot: I haz the power, 2016-11-27 01:12:08]

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

[sadrul, 2016-11-27 01:24:10]

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

[commit-bot: I haz the power, 2016-11-27 01:24:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-27 01:39:18]

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

[commit-bot: I haz the power, 2016-11-27 01:39:19]

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

[sadrul, 2016-11-27 01:52:38]

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

[commit-bot: I haz the power, 2016-11-27 01:52:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-27 02:26:56]

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

[commit-bot: I haz the power, 2016-11-27 02:26:56]

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

[sadrul, 2016-11-27 02:39:41]

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

[commit-bot: I haz the power, 2016-11-27 02:39:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-27 02:50:19]

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

[commit-bot: I haz the power, 2016-11-27 02:50:20]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sadrul, 2016-11-27 02:54:22]

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

[commit-bot: I haz the power, 2016-11-27 02:54:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-11-27 02:57:16]

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

[commit-bot: I haz the power, 2016-11-27 02:57:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-11-27 03:00:43]

Description was changed from

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-27 03:10:33]

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

[commit-bot: I haz the power, 2016-11-27 03:10:36]

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

[sadrul, 2016-11-27 03:14:39]

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

[commit-bot: I haz the power, 2016-11-27 03:14:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-11-27 03:59:05]

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

[sadrul, 2016-11-27 03:59:06]

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

https://codereview.chromium.org/2531963002/diff/120001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:468: :
gfx::CloneHandleForIPC(gpu_memory_buffer->GetHandle());
This may actually change behaviour. Right now, 'IsLost()' is checked only when
the GMBHandle's type is SHARED_MEMORY_HANDLE, and only the SharedMemoryHandle is
null'ed out. The rest of the fields are copied/cloned normally. With this
change, all of the fields are left invalid. I think this ought to be OK?

[commit-bot: I haz the power, 2016-11-27 04:07:26]

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

[commit-bot: I haz the power, 2016-11-27 04:07:27]

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

[piman, 2016-11-28 23:42:37]

LGTM, but one comment below, I think we should skip the IsLost test, actually.

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

https://codereview.chromium.org/2531963002/diff/120001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:466: channel_->IsLost()
Actually this is not necessary anymore, since now handle ownership is passed to
the IPC system on all platforms (including Windows and Mac). This check is racy
anyway (IsLost can change on the IO thread)... previously the code was
duplicating handles directly into the GPU process, and there could be ways in
which they were leaked there if we couldn't send the message. But now the handle
is always passed/brokered/freed whether the message is sent or not, so I think
we should just unconditionally clone. If the channel is lost, we're most likely
going to early out on l.451, the only few remaining cases (channel is lost on
the IO thread but we haven't yet executed the task that tells us so) are not
interesting to "optimize".

[sadrul, 2016-11-29 01:52:48]

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

[commit-bot: I haz the power, 2016-11-29 01:53:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2016-11-29 01:53:51]

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

https://codereview.chromium.org/2531963002/diff/120001/gpu/ipc/client/command...
gpu/ipc/client/command_buffer_proxy_impl.cc:466: channel_->IsLost()
On 2016/11/28 23:42:36, piman wrote:
> Actually this is not necessary anymore, since now handle ownership is passed
to
> the IPC system on all platforms (including Windows and Mac). This check is
racy
> anyway (IsLost can change on the IO thread)... previously the code was
> duplicating handles directly into the GPU process, and there could be ways in
> which they were leaked there if we couldn't send the message. But now the
handle
> is always passed/brokered/freed whether the message is sent or not, so I think
> we should just unconditionally clone. If the channel is lost, we're most
likely
> going to early out on l.451, the only few remaining cases (channel is lost on
> the IO thread but we haven't yet executed the task that tells us so) are not
> interesting to "optimize".

Oh cool. Thanks! Removed the IsLost() check from here.

[sadrul, 2016-11-29 01:54:27]

Description was changed from

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[piman, 2016-11-29 02:05:38]

lgtm

[sadrul, 2016-11-29 03:10:17]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-11-29 03:10:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 04:10:11]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480389017708760,
"parent_rev": "bc5187887e6864ad805dfe067d06a9224cb32800", "commit_rev":
"5b2d8b8e15c8a21d24bfb88c4287e131147166fc"}

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

Description was changed from

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-11-29 04:11:39]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-29 04:14:01]

Description was changed from

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
gfx: Fix sending native ozone pixmaps from InProcessCommandBuffer.

The file-descriptors in NativePixmapHandle need to be dup'ed before then
can be sent over mojo IPC from InProcessCommandBuffer. Introduce a
convenience function CloneHandleForIPC() so that it can be used by both
GpuChannelHost and InProcessCommandBuffer.

BUG=643746
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/73794caeafd8fb67b345eae7cea31763274beaec
Cr-Commit-Position: refs/heads/master@{#434884}
==========

[commit-bot: I haz the power, 2016-11-29 04:14:02]

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