Make PlatformFileForTransit its own class on Windows.

Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f6905203ecd6adbbc4bfe57e086eb61028c6]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG=713763
Review-Url: https://codereview.chromium.org/2846293002
Cr-Commit-Position: refs/heads/master@{#468213}
Committed: https://chromium.googlesource.com/chromium/src/+/d804e105f1e27e1ffafb7ac454d445188d6f602c

[erikchen, 2017-04-28 17:41:56]

Description was changed from

==========
make platformfilefortransit it's own class on windows


start on ipc.

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

to

==========
Make PlatformFileForTransit its own class on Windows.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

[erikchen, 2017-04-28 17:47:10]

Description was changed from

==========
Make PlatformFileForTransit its own class on Windows.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

to

==========
Make PlatformFileForTransit its own class on Windows.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

[erikchen, 2017-04-28 17:47:42]

Description was changed from

==========
Make PlatformFileForTransit its own class on Windows.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

to

==========
Make PlatformFileForTransit its own class on Windows.

This CL is a refactor and has no intended behavior change.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

[erikchen, 2017-04-28 17:47:44]

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

[erikchen, 2017-04-28 17:47:44]

rockot: Please review.

[erikchen, 2017-04-28 17:47:45]

rockot: Please review.

[erikchen, 2017-04-28 17:47:54]

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

[commit-bot: I haz the power, 2017-04-28 17:48:31]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-28 20:39:31]

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

[commit-bot: I haz the power, 2017-04-28 20:40:08]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-28 20:40:47]

Description was changed from

==========
Make PlatformFileForTransit its own class on Windows.

This CL is a refactor and has no intended behavior change.

Previously, it had the same semantics as SharedMemoryHandle, so it was
typedeffed to SharedMemoryHandle. However, the semantics for SharedMemoryHandle
are changing, so it's no longer appropriate to do so.

BUG=713763
==========

to

==========
Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

BUG=713763
==========

[erikchen, 2017-04-28 20:42:31]

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

[erikchen, 2017-04-28 20:42:32]

mseaborn: Please review components/nacl

[Ken Rockot(use gerrit already), 2017-04-28 20:45:52]

ipc lgtm

[Mark Seaborn, 2017-04-28 22:45:21]

https://codereview.chromium.org/2846293002/diff/40001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (left):

https://codereview.chromium.org/2846293002/diff/40001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:464: nexe_for_transit =
IPC::PlatformFileForTransit(nexe_file_info->handle,
You said "This CL includes a small fix to ppb_nacl_private_impl.cc, which
previously leaked a HANDLE on Windows, but not on an fd on POSIX".

In what case was it leaking a HANDLE (on error or success or both)?  Are you
saying that this is fixed by the change?

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.cc
File ipc/ipc_platform_file.cc (right):

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.c...
ipc/ipc_platform_file.cc:15: PlatformFileForTransit::PlatformFileForTransit() :
handle_(nullptr){};
Nit: remove ';' and add space before '{}'

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.c...
ipc/ipc_platform_file.cc:18: : handle_(handle){};
Ditto

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h
File ipc/ipc_platform_file.h (right):

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h...
ipc/ipc_platform_file.h:20: // The semantics for IPC transfer of a
SharedMemoryHandle are exactly the same
I think this reference to SharedMemoryHandle needs to be updated.

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h...
ipc/ipc_platform_file.h:22: // metadata that indicates the process to which the
HANDLE belongs.
PlatformFileForTransit no longer has this metadata, right?

[erikchen, 2017-04-28 23:21:53]

Description was changed from

==========
Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

BUG=713763
==========

to

==========
Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f6905203ecd6adbbc4bfe57e086eb61028c6]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG=713763
==========

[erikchen, 2017-04-28 23:22:47]

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

[commit-bot: I haz the power, 2017-04-28 23:23:08]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-04-28 23:24:37]

mseaborn: PTAL

https://codereview.chromium.org/2846293002/diff/40001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (left):

https://codereview.chromium.org/2846293002/diff/40001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:464: nexe_for_transit =
IPC::PlatformFileForTransit(nexe_file_info->handle,
On 2017/04/28 22:45:21, Mark Seaborn wrote:
> You said "This CL includes a small fix to ppb_nacl_private_impl.cc, which
> previously leaked a HANDLE on Windows, but not on an fd on POSIX".
> 
> In what case was it leaking a HANDLE (on error or success or both)?  Are you
> saying that this is fixed by the change?

Both and yes. I've substantially expanded the CL description with details.

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.cc
File ipc/ipc_platform_file.cc (right):

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.c...
ipc/ipc_platform_file.cc:15: PlatformFileForTransit::PlatformFileForTransit() :
handle_(nullptr){};
On 2017/04/28 22:45:21, Mark Seaborn wrote:
> Nit: remove ';' and add space before '{}'

Interesting. Removing the ";" causes clang-format to do the right thing.

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.c...
ipc/ipc_platform_file.cc:18: : handle_(handle){};
On 2017/04/28 22:45:21, Mark Seaborn wrote:
> Ditto

Interesting. Removing the ";" causes clang-format to do the right thing.

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h
File ipc/ipc_platform_file.h (right):

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h...
ipc/ipc_platform_file.h:20: // The semantics for IPC transfer of a
SharedMemoryHandle are exactly the same
On 2017/04/28 22:45:21, Mark Seaborn wrote:
> I think this reference to SharedMemoryHandle needs to be updated.

This whole comment is no longer meaningful. Removed.

https://codereview.chromium.org/2846293002/diff/40001/ipc/ipc_platform_file.h...
ipc/ipc_platform_file.h:22: // metadata that indicates the process to which the
HANDLE belongs.
On 2017/04/28 22:45:21, Mark Seaborn wrote:
> PlatformFileForTransit no longer has this metadata, right?

This whole comment is no longer meaningful. Removed.

[Mark Seaborn, 2017-04-28 23:34:23]

LGTM, thanks

[erikchen, 2017-04-28 23:46:24]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-04-28 23:46:25]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-28 23:46:25]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2846293002/#ps60001
(title: "Comments from mseaborn.")

[commit-bot: I haz the power, 2017-04-28 23:46:57]

CQ is trying da patch.

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

[erikchen, 2017-04-29 01:07:39]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-04-29 01:07:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2846293002/#ps80001
(title: "Change message serialization to write nothing if the handle isn't
valid.")

[commit-bot: I haz the power, 2017-04-29 01:08:00]

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, 2017-04-29 02:24:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493428059199660,
"parent_rev": "7988a5c627d0f19ab14ccddfa532db52076c6ce5", "commit_rev":
"d804e105f1e27e1ffafb7ac454d445188d6f602c"}

[commit-bot: I haz the power, 2017-04-29 02:24:51]

Description was changed from

==========
Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f6905203ecd6adbbc4bfe57e086eb61028c6]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG=713763
==========

to

==========
Make PlatformFileForTransit its own class on Windows.

This CL is *mostly* a refactor with no intended behavior change.

Previously, PlatformFileForTransit had the same semantics as SharedMemoryHandle,
so it was typedef-ed to SharedMemoryHandle. However, the semantics for
SharedMemoryHandle are changing, so it's no longer appropriate to do so.

This CL includes a small fix to ppb_nacl_private_impl.cc, which previously
leaked a HANDLE on Windows, but not on an fd on POSIX.

The leak was introduced by "Make PlatformFileForTransit a class on Windows."
[commit: 19e5f6905203ecd6adbbc4bfe57e086eb61028c6]. Prior to that CL:
  On Windows, NaCl would duplicate the handle into the destination process
  [closing the original handle in the process], and then pass the result as a
  raw int.

  On POSIX, NaCl would duplicate the handle using Chrome IPC, using
  base::FileDescriptor.auto_close = true, which would cause the IPC subsystem
  to close the handle.

After that CL:
  On Windows, NaCl would let Chrome IPC transport the handle, but failed to
  call PlatformFileForTransit::SetOwnershipPassesToIPC(true) [the equivalent of
  base::FileDescriptor.auto_close]. This caused a leak of the handle in the
  browser process.

In this CL:
  PlatformFileForTransit is now its own class, with no auto_close parameter
  because the IPC subsystem always closes the handle in the source process.

BUG=713763

Review-Url: https://codereview.chromium.org/2846293002
Cr-Commit-Position: refs/heads/master@{#468213}
Committed:
https://chromium.googlesource.com/chromium/src/+/d804e105f1e27e1ffafb7ac454d4...
==========

[commit-bot: I haz the power, 2017-04-29 02:24:53]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/d804e105f1e27e1ffafb7ac454d4...