IPC: Add attachment brokering support to the message header.

IPC: Add attachment brokering support to the message header.

Message dispatch happens before message translation, and message dispatch
requires that all brokered attachments have been received. This means that
attachment brokering needs to function without message translation. This is
accomplished by modifying the message header to include a new field
num_brokered_attachments, and writing the attachment ids into the IPC Channel
immediately following the pickled message itself.

AttachmentBrokerPrivilegedWinUnittest was expanded to test ChannelReader in the
receiving process. It is now a fully functional end-to-end test of attachment
brokering.

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

[erikchen, 2015-08-13 01:30:55]

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

[commit-bot: I haz the power, 2015-08-13 01:31:09]

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

[commit-bot: I haz the power, 2015-08-13 01:46:24]

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

[commit-bot: I haz the power, 2015-08-13 01:46:25]

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

[erikchen, 2015-08-13 08:05:08]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-08-13 08:05:13]

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

[commit-bot: I haz the power, 2015-08-13 08:05:14]

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

[commit-bot: I haz the power, 2015-08-13 08:05:15]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[erikchen, 2015-08-13 17:59:58]

Patchset #7 (id:120001) has been deleted

[erikchen, 2015-08-13 18:34:43]

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

[commit-bot: I haz the power, 2015-08-13 18:35:09]

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

[commit-bot: I haz the power, 2015-08-13 18:51:51]

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

[commit-bot: I haz the power, 2015-08-13 18:51:52]

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, 2015-08-13 20:58:08]

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

[commit-bot: I haz the power, 2015-08-13 20:58:51]

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

[commit-bot: I haz the power, 2015-08-13 21:26:14]

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

[commit-bot: I haz the power, 2015-08-13 21:26:14]

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, 2015-08-13 21:46:41]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-08-13 21:47:09]

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

[commit-bot: I haz the power, 2015-08-13 21:47:10]

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

[commit-bot: I haz the power, 2015-08-13 21:47:10]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[erikchen, 2015-08-13 21:50:41]

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

[commit-bot: I haz the power, 2015-08-13 21:51:21]

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

[erikchen, 2015-08-13 22:01:01]

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

[commit-bot: I haz the power, 2015-08-13 22:01:34]

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

[commit-bot: I haz the power, 2015-08-13 22:31:30]

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

[commit-bot: I haz the power, 2015-08-13 22:31:31]

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

[erikchen, 2015-08-13 22:51:09]

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

[commit-bot: I haz the power, 2015-08-13 22:51:27]

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

[commit-bot: I haz the power, 2015-08-13 23:30:41]

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

[commit-bot: I haz the power, 2015-08-13 23:30:42]

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, 2015-08-14 01:06:58]

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

[commit-bot: I haz the power, 2015-08-14 01:07:14]

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

[commit-bot: I haz the power, 2015-08-14 02:07:42]

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

[commit-bot: I haz the power, 2015-08-14 02:07:43]

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

[erikchen, 2015-08-14 18:25:09]

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

[commit-bot: I haz the power, 2015-08-14 18:25:32]

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

[erikchen, 2015-08-14 20:20:20]

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

[erikchen, 2015-08-14 20:20:21]

tsepez: Please review.

The only try-bot failure is unrelated:
"""
Failure: Could not find image Pixel_Canvas2DRedBox in cloud storage
"""

[commit-bot: I haz the power, 2015-08-14 21:14:29]

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

[commit-bot: I haz the power, 2015-08-14 21:14:30]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2015-08-17 18:29:39]

I'm a little leery of modifying the header; got other ideas?

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:25: return id;
nit: feels wrong to allow this to return a known key, even if not
USE_ATTACHMENT_BROKER.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.h (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:20: static const size_t kNonceSize;
Why did we lose the initianlizer? should be just const size_t, losing the
static.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:26: // Default constructor doesn't initialize nonce.
nit: Seems like a mistake to leave it uninitizlized.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:35: bool operator==(const AttachmentId& rhs) const;
probably want to put the code in here to help the compiler inline.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.cc
File ipc/ipc_channel.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.cc#new...
ipc/ipc_channel.cc:51: if (message_)
no need to test before delete.  just delete them.
You might consider using a scoped_ptr here instead to indicate ownership.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:254: size_t size();
nit: size_t size() const;

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:255: const void* data();
nit: const void* data() const;

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:257: 
nit: this is const, too.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:260: const char* buffer_;
nit: is it a void*, a char*, or a uint8_t*?  Can we make this consistent witht 
the data() method, etc?

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:92: for (BrokerableAttachment::AttachmentId id :
info.attachment_ids)
nit: maybe auto makes this simpler, also do you want & to avoid a copy?

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:135: OutputElement* element2 = new
OutputElement(ids.buffer, ids.size);
nit: maybe call this new_element rather than element2

[erikchen, 2015-08-18 00:11:35]

Patchset #16 (id:320001) has been deleted

[erikchen, 2015-08-18 00:33:21]

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

[commit-bot: I haz the power, 2015-08-18 00:33:40]

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

[commit-bot: I haz the power, 2015-08-18 01:08:19]

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

[commit-bot: I haz the power, 2015-08-18 01:08:20]

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

[erikchen, 2015-08-18 01:32:03]

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

[commit-bot: I haz the power, 2015-08-18 01:34:08]

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

[commit-bot: I haz the power, 2015-08-18 02:19:48]

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

[commit-bot: I haz the power, 2015-08-18 02:19:49]

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

[erikchen, 2015-08-18 05:59:46]

tsepez: PTAL

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.cc:25: return id;
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: feels wrong to allow this to return a known key, even if not
> USE_ATTACHMENT_BROKER.

I agree. I've changed the no-parameter constructor to return a random nonce when
USE_ATTACHMENT_BROKER is true, and assert with CHECK(false) when
USE_ATTACHMENT_BROKER is false.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
File ipc/brokerable_attachment.h (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:20: static const size_t kNonceSize;
On 2015/08/17 18:29:39, Tom Sepez wrote:
> Why did we lose the initianlizer? should be just const size_t, losing the
> static.

I looked into this further...the actual problem lies in the implementation of
DCHECK_EQ, and switching DCHECK_EQ(something, kNonceSize) into DCHECK (something
== kNonceSize) fixes the problem.

http://stackoverflow.com/questions/185844/initializing-private-static-members

In C++, inlining the initializer makes this a declaration, but not a definition.
This is fine, as long as nothing attempts to take the address of kNonceSize.
I've reverted back to the inline initializer.

I'm not sure what you mean about removing "static", in this context, "static"
means that the member is a class member, rather than an instance member.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:26: // Default constructor doesn't initialize nonce.
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: Seems like a mistake to leave it uninitizlized.

You're right, I've changed the behavior and updated the comment.

https://codereview.chromium.org/1286253002/diff/300001/ipc/brokerable_attachm...
ipc/brokerable_attachment.h:35: bool operator==(const AttachmentId& rhs) const;
On 2015/08/17 18:29:39, Tom Sepez wrote:
> probably want to put the code in here to help the compiler inline.

Done.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.cc
File ipc/ipc_channel.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.cc#new...
ipc/ipc_channel.cc:51: if (message_)
On 2015/08/17 18:29:39, Tom Sepez wrote:
> no need to test before delete.  just delete them.
> You might consider using a scoped_ptr here instead to indicate ownership.

Done on both counts.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:254: size_t size();
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: size_t size() const;

Done.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:255: const void* data();
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: const void* data() const;

Done.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:257: 
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: this is const, too.

Done.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:260: const char* buffer_;
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: is it a void*, a char*, or a uint8_t*?  Can we make this consistent witht

> the data() method, etc?

It is a void*. I've made the pointers consistent.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_reader.cc
File ipc/ipc_channel_reader.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_reader...
ipc/ipc_channel_reader.cc:92: for (BrokerableAttachment::AttachmentId id :
info.attachment_ids)
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: maybe auto makes this simpler, also do you want & to avoid a copy?

Done.

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_win.cc
File ipc/ipc_channel_win.cc (right):

https://codereview.chromium.org/1286253002/diff/300001/ipc/ipc_channel_win.cc...
ipc/ipc_channel_win.cc:135: OutputElement* element2 = new
OutputElement(ids.buffer, ids.size);
On 2015/08/17 18:29:39, Tom Sepez wrote:
> nit: maybe call this new_element rather than element2

Done.

[Tom Sepez, 2015-08-18 15:18:56]

lgtm

Ok, I think this works.

https://codereview.chromium.org/1286253002/diff/380001/ipc/ipc_test_messages.h
File ipc/ipc_test_messages.h (right):

https://codereview.chromium.org/1286253002/diff/380001/ipc/ipc_test_messages....
ipc/ipc_test_messages.h:7: #if defined(OS_WIN)
nit: maybe whole file guarded by single OS_WIN

[erikchen, 2015-08-19 17:47:55]

https://codereview.chromium.org/1286253002/diff/380001/ipc/ipc_test_messages.h
File ipc/ipc_test_messages.h (right):

https://codereview.chromium.org/1286253002/diff/380001/ipc/ipc_test_messages....
ipc/ipc_test_messages.h:7: #if defined(OS_WIN)
On 2015/08/18 15:18:56, Tom Sepez (Out Aug 20-29) wrote:
> nit: maybe whole file guarded by single OS_WIN

Done.

[erikchen, 2015-08-19 17:48:08]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-08-19 17:48:08]

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

[commit-bot: I haz the power, 2015-08-19 17:48:20]

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

[commit-bot: I haz the power, 2015-08-19 18:38:55]

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

[commit-bot: I haz the power, 2015-08-19 18:38:56]

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, 2015-08-19 22:22:04]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2015-08-19 22:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1286253002/#ps420001
(title: "Compile error.")

[commit-bot: I haz the power, 2015-08-19 22:22:28]

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

[commit-bot: I haz the power, 2015-08-20 00:59:23]

Committed patchset #20 (id:420001)

[commit-bot: I haz the power, 2015-08-20 01:00:10]

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

[Nico, 2015-08-20 01:55:31]

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

[Nico, 2015-08-20 01:55:32]

This broke the clang/win build, can you take a look?

FAILED: ninja -t msvc -e environment.x64 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo
/showIncludes /FC @obj\ipc\ipc_win64.attachment_broker_privileged_win.obj.rsp /c
..\..\ipc\attachment_broker_privileged_win.cc
/Foobj\ipc\ipc_win64.attachment_broker_privileged_win.obj
/Fdobj\ipc\ipc_win64.cc.pdb 
..\..\ipc\attachment_broker_privileged_win.cc(24,11) : error: enumeration value
'PLACEHOLDER' not handled in switch [-Werror,-Wswitch]
  switch (attachment->GetBrokerableType()) {
          ^
1 error generated.
FAILED: ninja -t msvc -e environment.x64 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo
/showIncludes /FC @obj\ipc\ipc_win64.attachment_broker_unprivileged_win.obj.rsp
/c ..\..\ipc\attachment_broker_unprivileged_win.cc
/Foobj\ipc\ipc_win64.attachment_broker_unprivileged_win.obj
/Fdobj\ipc\ipc_win64.cc.pdb 
..\..\ipc\attachment_broker_unprivileged_win.cc(22,11) : error: enumeration
value 'PLACEHOLDER' not handled in switch [-Werror,-Wswitch]
  switch (attachment->GetBrokerableType()) {
          ^

[Adam Rice, 2015-08-20 09:21:16]

A revert of this CL (patchset #20 id:420001) has been created in
https://codereview.chromium.org/1286883003/ by ricea@chromium.org.

The reason for reverting is: Suspected of breaking XP Tests. See, for example
https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/build...
. .