Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
TBR=brettw@chromium.org

Review-Url: https://codereview.chromium.org/2845113005
Cr-Commit-Position: refs/heads/master@{#468731}
Committed: https://chromium.googlesource.com/chromium/src/+/c87903ecca1345c4363da94e0120adedd83963b0

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

Description was changed from

==========
Remove share readonly path.

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

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

[erikchen, 2017-04-28 00:35:53]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

[erikchen, 2017-04-28 01:45:21]

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

[commit-bot: I haz the power, 2017-04-28 01:46:25]

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, 2017-04-28 01:58:32]

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

[commit-bot: I haz the power, 2017-04-28 01:58:33]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-04-28 18:12:52]

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

[erikchen, 2017-04-28 18:13:25]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-04-28 18:15:50]

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

[erikchen, 2017-04-28 18:16:18]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

[erikchen, 2017-04-28 18:16:39]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is poor
accounting. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

[erikchen, 2017-04-28 18:16:45]

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

[erikchen, 2017-04-28 18:16:46]

thakis: Please review

[commit-bot: I haz the power, 2017-04-28 18:16:55]

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, 2017-04-28 19:11:00]

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

[commit-bot: I haz the power, 2017-04-28 19:11:01]

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

[erikchen, 2017-04-28 21:06:11]

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

[commit-bot: I haz the power, 2017-04-28 21:06:57]

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 21:07:38]

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

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

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, 2017-04-28 23:03:43]

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

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

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_...)

[erikchen, 2017-04-29 16:57:37]

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

[commit-bot: I haz the power, 2017-04-29 16:57:49]

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-29 17:22:44]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. The new method GetReadOnlyHandle returns a read-only handle in the
current process which the IPC subsystem will transport into the destination
process.

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle

returns a read-only handle in the current process which the IPC subsystem will 
transport into the destination process. 

BUG=716206
==========

[erikchen, 2017-04-29 17:23:09]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle

returns a read-only handle in the current process which the IPC subsystem will 
transport into the destination process. 

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
==========

[commit-bot: I haz the power, 2017-04-29 18:42:48]

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

[commit-bot: I haz the power, 2017-04-29 18:42:49]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-04-29 21:46:40]

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

[erikchen, 2017-04-29 21:46:41]

brettw: Please stamp everything not in base/. I plan on TBR-ing you haven't
gotten to this when I have a base OWNER l-g-t-m, since this CL is just a
refactor.

[Nico, 2017-04-30 22:33:53]

thakis@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Nico, 2017-04-30 22:33:54]

jyasskin, since this modifies an API you added a while ago, could you give this
a look?

[Jeffrey Yasskin, 2017-05-01 22:16:34]

This would have been easier to read if the changes to migrate to the new
SharedMemoryHandle methods were separate from the changes to extract
GetReadOnlyHandle from the *ToProcess functions.

lgtm: I think I only used the ToProcess form because that's what already
existed. If all the ToProcess functions are going away, doing the readonly part
differently makes sense.

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (left):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:292: // We could imagine re-opening the file
from /dev/fd, but that can't make
This comment should probably survive the refactoring.

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (right):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:282: return true;
return new_handle->IsValid()?

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:350: handle.SetOwnershipPassesToIPC(true);
Does this need to show up in the interface of GetReadOnlyHandle()? It looks like
it's only valid to use the resulting handle once?

[erikchen, 2017-05-02 01:53:08]

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (left):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:292: // We could imagine re-opening the file
from /dev/fd, but that can't make
On 2017/05/01 22:16:34, Jeffrey Yasskin wrote:
> This comment should probably survive the refactoring.

Done.

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_mac.cc (right):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_mac.cc:282: return true;
On 2017/05/01 22:16:34, Jeffrey Yasskin wrote:
> return new_handle->IsValid()?

Done.

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2845113005/diff/200001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:350: handle.SetOwnershipPassesToIPC(true);
On 2017/05/01 22:16:34, Jeffrey Yasskin wrote:
> Does this need to show up in the interface of GetReadOnlyHandle()? It looks
like
> it's only valid to use the resulting handle once?

Done.

[erikchen, 2017-05-02 01:53:13]

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

[commit-bot: I haz the power, 2017-05-02 01:53:52]

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, 2017-05-02 03:23:38]

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

[commit-bot: I haz the power, 2017-05-02 03:23:39]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-05-02 15:48:39]

lgtm

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
File base/memory/shared_memory.h (right):

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory.h:210: // ownership of the handle. CHECK-fails if the
region wasn't Created or
I didn't see this promised CHECK on all platforms.

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory.h:213: // pass the sample handle to the IPC subsystem
twice.
Say that this will return an invalid handle on failure.

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:343: ProcessHandle process =
GetCurrentProcess();
(nit: why does this have a variable?)

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:364: if (close_self) {
We don't need self-closing readonly shared memory, yes?

https://codereview.chromium.org/2845113005/diff/220001/mojo/edk/embedder/plat...
File mojo/edk/embedder/platform_shared_buffer.cc (left):

https://codereview.chromium.org/2845113005/diff/220001/mojo/edk/embedder/plat...
mojo/edk/embedder/platform_shared_buffer.cc:207: bool success;
nice

[Nico, 2017-05-02 15:49:45]

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:364: if (close_self) {
On 2017/05/02 15:48:39, Nico wrote:
> We don't need self-closing readonly shared memory, yes?

Ignore this.

[Nico, 2017-05-02 15:51:34]

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
File base/memory/shared_memory_win.cc (right):

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory_win.cc:343: ProcessHandle process =
GetCurrentProcess();
On 2017/05/02 15:48:39, Nico wrote:
> (nit: why does this have a variable?)

(this one too)

[erikchen, 2017-05-02 17:52:19]

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
File base/memory/shared_memory.h (right):

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory.h:210: // ownership of the handle. CHECK-fails if the
region wasn't Created or
On 2017/05/02 15:48:39, Nico wrote:
> I didn't see this promised CHECK on all platforms.

It's only relevant for POSIX handles. I've updated the comment.

https://codereview.chromium.org/2845113005/diff/220001/base/memory/shared_mem...
base/memory/shared_memory.h:213: // pass the sample handle to the IPC subsystem
twice.
On 2017/05/02 15:48:39, Nico wrote:
> Say that this will return an invalid handle on failure.

Done.

[erikchen, 2017-05-02 17:52:33]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
TBR=brettw@chromium.org
==========

[erikchen, 2017-05-02 17:52:36]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-02 17:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2845113005/#ps240001
(title: "Comments from thakis.")

[commit-bot: I haz the power, 2017-05-02 17:53:02]

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-05-02 19:05:07]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1493747556230440,
"parent_rev": "befe02860a5bdbf38349b76a56b55ae9f03714b7", "commit_rev":
"c87903ecca1345c4363da94e0120adedd83963b0"}

[commit-bot: I haz the power, 2017-05-02 19:05:15]

Description was changed from

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
TBR=brettw@chromium.org
==========

to

==========
Replace base::SharedMemory read-only methods with GetReadOnlyHandle.

This CL is a refactor with no intended behavior change.

The previous methods ShareReadOnlyToProcess and GiveReadOnlyToProcess could be
used to directly add a handle to the destination process, which is a bad
practice. All callers already avoid doing this. The new method GetReadOnlyHandle
returns a read-only handle in the current process which the IPC subsystem will
transport into the destination process.

BUG=716206
TBR=brettw@chromium.org

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

[commit-bot: I haz the power, 2017-05-02 19:05:18]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/c87903ecca1345c4363da94e0120...