Make PlatformFileForTransit a class on Windows.

Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414
Committed: https://crrev.com/19e5f6905203ecd6adbbc4bfe57e086eb61028c6
Cr-Commit-Position: refs/heads/master@{#383849}

[erikchen, 2016-03-24 01:18:22]

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

[commit-bot: I haz the power, 2016-03-24 01:18:37]

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

[commit-bot: I haz the power, 2016-03-24 02:53:35]

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

[commit-bot: I haz the power, 2016-03-24 02:53:36]

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-03-24 18:23:11]

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

[commit-bot: I haz the power, 2016-03-24 18:23:33]

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

[erikchen, 2016-03-24 18:26:46]

Description was changed from

==========
Create typedef for PlatformFileForTransit.

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

to

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

[erikchen, 2016-03-24 18:27:33]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

[erikchen, 2016-03-24 18:27:57]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This CL does not introduce any behavior changes.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

[erikchen, 2016-03-24 18:28:33]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This CL does not introduce any behavior changes.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This CL does not change any functionality.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

[commit-bot: I haz the power, 2016-03-24 21:28:47]

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

[commit-bot: I haz the power, 2016-03-24 21:28:48]

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

[erikchen, 2016-03-24 23:20:49]

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

[commit-bot: I haz the power, 2016-03-24 23:21:06]

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

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

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

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

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-03-25 22:08:45]

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

[commit-bot: I haz the power, 2016-03-25 22:09:10]

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

[erikchen, 2016-03-25 22:14:16]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This CL does not change any functionality.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414
==========

[commit-bot: I haz the power, 2016-03-26 00:40:54]

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

[commit-bot: I haz the power, 2016-03-26 00:40:55]

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-03-28 20:57:54]

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

[commit-bot: I haz the power, 2016-03-28 20:58:09]

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

[commit-bot: I haz the power, 2016-03-28 22:22:10]

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

[commit-bot: I haz the power, 2016-03-28 22:22:11]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-03-28 23:15:37]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414
==========

[erikchen, 2016-03-28 23:17:23]

erikchen@chromium.org changed reviewers:
+ avi@chromium.org, mseaborn@chromium.org, sergeyu@chromium.org,
tsepez@chromium.org

[erikchen, 2016-03-28 23:17:24]

mseaborn: Please review components/nacl
avi: Please review content/
tsepez: Please review ipc/
sergeyu: Please review remoting/

[Tom Sepez, 2016-03-28 23:42:04]

lgtm

[Avi (use Gerrit), 2016-03-29 01:30:59]

lgtm

[Sergey Ulanov, 2016-03-29 18:19:41]

lgtm

[Mark Seaborn, 2016-03-29 19:24:10]

LGTM

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:577: )));
Nit: should line up with iter->descriptor() args above

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:93: bool ok =
content::BrokerDuplicateHandle(
So this case would get converted by a later change?

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:475: // Duplicate the handle
on the browser side instead of the renderer.
This comment doesn't apply any more, since you're removing the DuplicateHandle()
call in components/nacl/browser/nacl_host_message_filter.cc, so can you remove
it?

I assume the new code path handles renderer -> browser transfers securely?

[erikchen, 2016-03-29 20:57:42]

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/loader/...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/loader/...
components/nacl/loader/nacl_ipc_adapter.cc:577: )));
On 2016/03/29 19:24:09, Mark Seaborn wrote:
> Nit: should line up with iter->descriptor() args above

Done.

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
File components/nacl/renderer/manifest_service_channel.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
components/nacl/renderer/manifest_service_channel.cc:93: bool ok =
content::BrokerDuplicateHandle(
On 2016/03/29 19:24:09, Mark Seaborn wrote:
> So this case would get converted by a later change?

yes. Trying to keep this change as small as possible.

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1830853002/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:475: // Duplicate the handle
on the browser side instead of the renderer.
On 2016/03/29 19:24:09, Mark Seaborn wrote:
> This comment doesn't apply any more, since you're removing the
DuplicateHandle()
> call in components/nacl/browser/nacl_host_message_filter.cc, so can you remove
> it?
> 
> I assume the new code path handles renderer -> browser transfers securely?

Removed the comment. Yes, the new path is secure.

[erikchen, 2016-03-29 20:57:48]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-03-29 20:57:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, tsepez@chromium.org, avi@chromium.org,
sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1830853002/#ps100001
(title: "Comments from mseaborn.")

[commit-bot: I haz the power, 2016-03-29 20:58:47]

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

[commit-bot: I haz the power, 2016-03-29 22:26:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-29 22:28:25]

Description was changed from

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414
==========

to

==========
Make PlatformFileForTransit a class on Windows.

This is required to transition consumers of PlatformFileForTransit into using
attachment brokering. Alias PlatformFileForTransit to base::SharedMemoryHandle,
since that class has the exact semantics required for brokering a HANDLE.

I tried to avoid unnecessary functionality changes, but couldn't avoid changing
some logic in NaclHostMessageFilter and DesktopSessionProxy. Both classes
receive an IPC message that contain a HANDLE from the sender process, and try to
copy the HANDLE into the current process. By switching to an
attachment-brokerable class, PlatformFileForTransit is automatically brokered,
so the duplication logic is no longer required.

Expect future CLs to convert the remaining consumers of PlatformFileForTransit
into using attachment brokering.

BUG=493414

Committed: https://crrev.com/19e5f6905203ecd6adbbc4bfe57e086eb61028c6
Cr-Commit-Position: refs/heads/master@{#383849}
==========

[commit-bot: I haz the power, 2016-03-29 22:28:26]

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

[Nico, 2016-03-30 12:01:00]

I believe this broke builds with GYP_DEFINES=enable_ipc_fuzzer=1:

e.g.
https://build.chromium.org/p/chromium.fyi/builders/CrWinAsan/builds/3207/step...
:

..\..\tools\ipc_fuzzer\fuzzer\fuzzer.cc(1356,8):  error: redefinition of
'ipc_fuzzer::FuzzTraits<base::SharedMemoryHandle>'
struct FuzzTraits<IPC::PlatformFileForTransit> {
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
..\..\tools\ipc_fuzzer\fuzzer\fuzzer.cc(458,8):  note: previous definition is
here
struct FuzzTraits<base::SharedMemoryHandle> {
       ^
1 error generated.

Can you take a look?