[mojo-edk] Bind a child token to child launches and port reservations.

[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org

Committed: https://crrev.com/6ad1e819b60a1c90f4890ae4c0872a060e09a4c4
Cr-Commit-Position: refs/heads/master@{#397978}

[Anand Mistry (off Chromium), 2016-06-01 05:55:09]

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

[Ken Rockot(use gerrit already), 2016-06-01 15:40:48]

lgtm % a few small changes/nits

https://codereview.chromium.org/2019973002/diff/100001/components/nacl/browse...
File components/nacl/browser/BUILD.gn (right):

https://codereview.chromium.org/2019973002/diff/100001/components/nacl/browse...
components/nacl/browser/BUILD.gn:42: "//mojo/edk/system",
this change (and the change to DEPS) don't seem to belong in this CL

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:179: // Ensure local port closure messages
are processes.
nit: processed*

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:624: }
Huh. It probably doesn't make much difference in practice, but I wonder if
DropAllPeers should also AcceptIncomingMessages.

[Anand Mistry (off Chromium), 2016-06-02 04:35:35]

amistry@chromium.org changed reviewers:
+ nick@chromium.org

[Anand Mistry (off Chromium), 2016-06-02 04:35:37]

nick@chromium.org: Please review changes in content/

https://codereview.chromium.org/2019973002/diff/100001/components/nacl/browse...
File components/nacl/browser/BUILD.gn (right):

https://codereview.chromium.org/2019973002/diff/100001/components/nacl/browse...
components/nacl/browser/BUILD.gn:42: "//mojo/edk/system",
On 2016/06/01 15:40:48, Ken Rockot wrote:
> this change (and the change to DEPS) don't seem to belong in this CL

Oops. Forgot to revert from a prior patchset.

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:179: // Ensure local port closure messages
are processes.
On 2016/06/01 15:40:48, Ken Rockot wrote:
> nit: processed*

Done.

https://codereview.chromium.org/2019973002/diff/100001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:624: }
On 2016/06/01 15:40:48, Ken Rockot wrote:
> Huh. It probably doesn't make much difference in practice, but I wonder if
> DropAllPeers should also AcceptIncomingMessages.

There'a problem with running AcceptIncomingMessages() here.
AcceptIncomingMessages can trigger watcher wake-ups which will call back into
user code. I don't think we want to trigger watchers since this is on message
loop shutdown.

But given that we don't close any other ports, this is probably entirely
unnecessary. This might even be completely wrong and should be removed. WDYT?

[ncarter (slow), 2016-06-02 17:43:28]

lgtm

https://codereview.chromium.org/2019973002/diff/120001/content/browser/mojo/m...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/2019973002/diff/120001/content/browser/mojo/m...
content/browser/mojo/mojo_application_host.h:33: MojoApplicationHost(const
std::string& child_token);
explicit

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:247: std::string child_token =
std::move(it->second.second);
Why is this line needed?

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.h (right):

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.h:223: // Ports reserved by token.
Seems like there are two flavors of token in this map (one in the keys and one
in the values). Could this comment clarify which is which?

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.h:224: base::hash_map<std::string,
std::pair<ports::PortRef, std::string>>
With c++11 uniform inititalization syntax, it's really easy to just use a struct
instead of std::pair, like this:

struct ReservedPort {
  ports::PortRef port;
  std::string child_token;
};

Doing that, IMO, results in more readable code when the map is used
(it->second.port vs. it->second.second).

[Anand Mistry (off Chromium), 2016-06-03 04:39:41]

https://codereview.chromium.org/2019973002/diff/120001/content/browser/mojo/m...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/2019973002/diff/120001/content/browser/mojo/m...
content/browser/mojo/mojo_application_host.h:33: MojoApplicationHost(const
std::string& child_token);
On 2016/06/02 17:43:28, ncarter wrote:
> explicit

Done.

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.cc:247: std::string child_token =
std::move(it->second.second);
On 2016/06/02 17:43:28, ncarter wrote:
> Why is this line needed?

Oops. Relic from a different day.

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
File mojo/edk/system/node_controller.h (right):

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.h:223: // Ports reserved by token.
On 2016/06/02 17:43:28, ncarter wrote:
> Seems like there are two flavors of token in this map (one in the keys and one
> in the values). Could this comment clarify which is which?

Done.

https://codereview.chromium.org/2019973002/diff/120001/mojo/edk/system/node_c...
mojo/edk/system/node_controller.h:224: base::hash_map<std::string,
std::pair<ports::PortRef, std::string>>
On 2016/06/02 17:43:28, ncarter wrote:
> With c++11 uniform inititalization syntax, it's really easy to just use a
struct
> instead of std::pair, like this:
> 
> struct ReservedPort {
>   ports::PortRef port;
>   std::string child_token;
> };
> 
> Doing that, IMO, results in more readable code when the map is used
> (it->second.port vs. it->second.second).

Done.

[Anand Mistry (off Chromium), 2016-06-06 04:57:01]

Description was changed from

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
==========

to

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org
==========

[Anand Mistry (off Chromium), 2016-06-06 04:57:54]

TBRing for mechanical changes:
phajdan.jr for chrome/test
hidehiko for arc

[Anand Mistry (off Chromium), 2016-06-06 04:58:12]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-06-06 04:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2019973002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-06 04:58:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2019973002/160001

[commit-bot: I haz the power, 2016-06-06 05:36:42]

Description was changed from

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org
==========

to

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org
==========

[commit-bot: I haz the power, 2016-06-06 05:36:44]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-06 05:38:13]

Description was changed from

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org
==========

to

==========
[mojo-edk] Bind a child token to child launches and port reservations.

This allows the EDK to close reserved ports when a child launch fails, by
being able to associate channels to reserved ports.

BUG=589859
TBR=phajdan.jr@chromium.org,hidehiko@chromium.org

Committed: https://crrev.com/6ad1e819b60a1c90f4890ae4c0872a060e09a4c4
Cr-Commit-Position: refs/heads/master@{#397978}
==========

[commit-bot: I haz the power, 2016-06-06 05:38:15]

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