Mojo C++ bindings: introduce an optional array to store transferred interface IDs in messages.

Mojo C++ bindings: introduce an optional array to store transferred interface IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
TBR=rockot@chromium.org
(Ken has LGed, but maybe this could bypass the PRESUBMIT bug?)

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

[yzshen1, 2017-01-30 19:31:30]

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

[yzshen1, 2017-01-30 19:32:16]

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

[yzshen1, 2017-01-30 19:32:17]

Hi, Ken.

Would you please take a look? Thanks!

[commit-bot: I haz the power, 2017-01-30 19:32:33]

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

[yzshen1, 2017-01-30 19:33:49]

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

[commit-bot: I haz the power, 2017-01-30 19:33:59]

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-01-30 20:26:15]

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

[commit-bot: I haz the power, 2017-01-30 20:26:17]

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

[yzshen1, 2017-01-30 20:34:53]

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

[commit-bot: I haz the power, 2017-01-30 20:35:22]

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-01-30 21:13:11]

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

[commit-bot: I haz the power, 2017-01-30 21:13:12]

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

[yzshen1, 2017-01-30 21:13:25]

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

[commit-bot: I haz the power, 2017-01-30 21:14:02]

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

[yzshen1, 2017-01-30 21:20:32]

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

[commit-bot: I haz the power, 2017-01-30 21:21:10]

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

[yzshen1, 2017-01-30 22:08:06]

Description was changed from

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
==========

to

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
==========

[commit-bot: I haz the power, 2017-01-30 22:46:37]

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

[commit-bot: I haz the power, 2017-01-30 22:46:38]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-01-31 18:05:57]

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

[commit-bot: I haz the power, 2017-01-31 18:06:17]

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

[Ken Rockot(use gerrit already), 2017-01-31 18:33:43]

The Message format changes themselves look fine, and LGTM overall if you still
want to go with this approach since the implementation looks correct to me; but
the approach in general scares me quite a bit.

These new Serialize/DeserializeAssociatedEndpointHandles methods on Message are
confusing, and it seems like it's very easy to get their usage wrong. In that
regard I'm not sure it's any less subtle or error-prone than simply requiring
~Message to run outside of the controller lock, and it's almost certainly more
code. Did you have any luck exploring that as an alternative solution?

https://codereview.chromium.org/2660733002/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/2660733002/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:87: DCHECK(data_num_bytes() >=
header()->num_bytes);
nit: Can we DCHECK_GE?

https://codereview.chromium.org/2660733002/diff/140001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2660733002/diff/140001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:253: // payload interface IDs desreialized into
ScopedInterfaceEndpointHandles.
nit: deserialized

https://codereview.chromium.org/2660733002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/2660733002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message_builder.cc:37: // Set it to right after the
message header.
nit: Maybe clarify this comment -- something like "The payload immediately
follows the header"

[yzshen1, 2017-01-31 19:26:05]

> The Message format changes themselves look fine, and LGTM overall if you still
> want to go with this approach since the implementation looks correct to me;
but
> the approach in general scares me quite a bit.
> 
> These new Serialize/DeserializeAssociatedEndpointHandles methods on Message
are
> confusing, and it seems like it's very easy to get their usage wrong. In that
> regard I'm not sure it's any less subtle or error-prone than simply requiring
> ~Message to run outside of the controller lock, and it's almost certainly more
> code. Did you have any luck exploring that as an alternative solution?

(Ken and I had an offline chat, but record the answer to make the thread
complete.)

The Serialize/DeserializeAssociatedEndpointHandles() methods are mostly
unrelated to whether we require ~Message to run outside of the controller lock.
The ScopedInterfaceEndpointHandles in a message need to be converted to
interface IDs before sent by the Connector; and the reverse conversion needs to
happen before dispatching an incoming message to the upper layer.

Because they deal with the conversion between two fields of Messages, it seems
reasonable to me to have these methods on Message. An alternative would be
defining them on AssociatedGroupController.

https://codereview.chromium.org/2660733002/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message.cc (right):

https://codereview.chromium.org/2660733002/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message.cc:87: DCHECK(data_num_bytes() >=
header()->num_bytes);
On 2017/01/31 18:33:42, Ken Rockot wrote:
> nit: Can we DCHECK_GE?

Done.

https://codereview.chromium.org/2660733002/diff/140001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2660733002/diff/140001/ipc/ipc_mojo_bootstrap...
ipc/ipc_mojo_bootstrap.cc:253: // payload interface IDs desreialized into
ScopedInterfaceEndpointHandles.
On 2017/01/31 18:33:42, Ken Rockot wrote:
> nit: deserialized

Done.

https://codereview.chromium.org/2660733002/diff/140001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/message_builder.cc (right):

https://codereview.chromium.org/2660733002/diff/140001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/message_builder.cc:37: // Set it to right after the
message header.
On 2017/01/31 18:33:42, Ken Rockot wrote:
> nit: Maybe clarify this comment -- something like "The payload immediately
> follows the header"

Done.

[yzshen1, 2017-01-31 19:27:15]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2017-01-31 19:27:16]

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/2660733002/#ps160001
(title: ".")

[commit-bot: I haz the power, 2017-01-31 19:28:08]

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-01-31 19:39:15]

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

[commit-bot: I haz the power, 2017-01-31 19:39:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yzshen1, 2017-01-31 20:14:24]

On 2017/01/31 19:39:16, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Presubmit complains "
Missing LGTM from an OWNER for these files:
    ipc/ipc_mojo_bootstrap.cc
"

But Ken is an owner of this file. This is probably a bug in the presubmit logic.

[yzshen1, 2017-01-31 20:14:34]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-01-31 20:15:05]

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-01-31 20:26:07]

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

[commit-bot: I haz the power, 2017-01-31 20:26:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yzshen1, 2017-01-31 20:37:03]

Description was changed from

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
==========

to

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
TBR=rockot@chromium.org
(Ken has LGed, but maybe this could bypass the PRESUBMIT bug?)
==========

[yzshen1, 2017-01-31 20:38:06]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2017-01-31 20:38:42]

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-01-31 21:20:25]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485895086637290,
"parent_rev": "272e53115aa8395317d15f7a4c976122c1651a58", "commit_rev":
"ea784ea651063e3add479242fc5c43c4bef2fe4b"}

[commit-bot: I haz the power, 2017-01-31 21:21:05]

Description was changed from

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
TBR=rockot@chromium.org
(Ken has LGed, but maybe this could bypass the PRESUBMIT bug?)
==========

to

==========
Mojo C++ bindings: introduce an optional array to store transferred interface
IDs in messages.

This is part of the work to simplify the associated interface API.

BUG=682334
TBR=rockot@chromium.org
(Ken has LGed, but maybe this could bypass the PRESUBMIT bug?)

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

[commit-bot: I haz the power, 2017-01-31 21:21:07]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ea784ea651063e3add479242fc5c...