[mojo-edk] Broadcast surprise port disruptions

[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864, 618206
R=amistry@chromium.org

Committed: https://crrev.com/3a9157501ed6f0d4de687908919be8111408b53c
Cr-Commit-Position: refs/heads/master@{#401698}

[Ken Rockot(use gerrit already), 2016-05-13 05:30:34]

OK last one. Fixing this bug does require introducing a new broadcast mechanism,
which means changing the transport protocol. I'll coordinate with arc folks
before landing this.

[Ken Rockot(use gerrit already), 2016-05-15 20:08:05]

BTW, I think if we're going to coordinate with arc folks to land a transport
protocol change, we might as well also remove the exceptional Android/Chrome OS
code to reduce complexity a bit.

I could opportunistically include that change in this CL - WDYT?

[Anand Mistry (off Chromium), 2016-05-16 04:03:58]

I would support getting ARC un-forked as part of this CL.

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/node_co...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/node_co...
mojo/edk/system/node_controller.cc:901: for (auto iter : peers_) {
auto&

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
File mojo/edk/system/ports/node.cc (right):

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1214: scoped_refptr<Port>& port = iter->second;
very subjective nit: Looks like using Port* is a fairly common pattern, even
though it's only used with PortRef. Also, using a reference to a scoped_refptr
feel wrong. Feel free to ignore if you think otherwise.

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1251: iter = ports_.erase(iter);
um... according to
http://en.cppreference.com/w/cpp/container/unordered_map/erase, this pattern is
only valid in C++14 and later, even though it probably works in our current
compilers. That makes me a little bit queezy (but not enough to be offended
since C++ before C++11 established this as a pattern with set/map and C++11 is
stupid for not specifying it). What do you think about it?

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1259: for (size_t i = 0; i <
ports_to_notify.size(); ++i)
for (const auto& port : ports_to_notify)

[Ken Rockot(use gerrit already), 2016-05-16 04:42:37]

OK - Android/ChromeOS unforked and other stuff addressed!

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/node_co...
File mojo/edk/system/node_controller.cc (right):

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/node_co...
mojo/edk/system/node_controller.cc:901: for (auto iter : peers_) {
On 2016/05/16 at 04:03:57, Anand Mistry wrote:
> auto&

Done

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
File mojo/edk/system/ports/node.cc (right):

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1214: scoped_refptr<Port>& port = iter->second;
On 2016/05/16 at 04:03:57, Anand Mistry wrote:
> very subjective nit: Looks like using Port* is a fairly common pattern, even
though it's only used with PortRef. Also, using a reference to a scoped_refptr
feel wrong. Feel free to ignore if you think otherwise.

Done

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1251: iter = ports_.erase(iter);
On 2016/05/16 at 04:03:57, Anand Mistry wrote:
> um... according to
http://en.cppreference.com/w/cpp/container/unordered_map/erase, this pattern is
only valid in C++14 and later, even though it probably works in our current
compilers. That makes me a little bit queezy (but not enough to be offended
since C++ before C++11 established this as a pattern with set/map and C++11 is
stupid for not specifying it). What do you think about it?

Yeah, that makes me uncomfortable. I changed it to remove all of them in
sequence after the iteration is done. I considered accumulating a separate list
of iterators for slightly faster removal, but it just seemed like uglier code to
me.

https://codereview.chromium.org/1975073002/diff/60001/mojo/edk/system/ports/n...
mojo/edk/system/ports/node.cc:1259: for (size_t i = 0; i <
ports_to_notify.size(); ++i)
On 2016/05/16 at 04:03:57, Anand Mistry wrote:
> for (const auto& port : ports_to_notify)

Done

[Anand Mistry (off Chromium), 2016-05-16 04:58:00]

LGTM

Please add to the CL description that this removes the ChromeOS/Android binary
compatibility code.

[Ken Rockot(use gerrit already), 2016-05-16 04:59:56]

Description was changed from

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864
R=amistry@chromium.org
==========

to

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

Because this must change the transport protocol across all
platforms, this also removes legacy protocol support for
Chrome OS and Android.

BUG=589864
R=amistry@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-05-16 21:52:09]

BTW spoke with folks - Sounds like we'll be good to land this Friday.

[Ken Rockot(use gerrit already), 2016-05-17 14:46:05]

Patchset #9 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-17 14:46:10]

Patchset #8 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2016-05-17 14:47:11]

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

[Ken Rockot(use gerrit already), 2016-05-17 14:47:11]

ochang@ could you please take a look for security?

[Oliver Chang, 2016-05-17 16:59:51]

security lgtm

[Ken Rockot(use gerrit already), 2016-05-23 22:41:09]

Patchset #10 (id:220001) has been deleted

[Ken Rockot(use gerrit already), 2016-06-09 03:05:41]

Description was changed from

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

Because this must change the transport protocol across all
platforms, this also removes legacy protocol support for
Chrome OS and Android.

BUG=589864
R=amistry@chromium.org
==========

to

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864
R=amistry@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-06-09 03:06:12]

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

[Ken Rockot(use gerrit already), 2016-06-09 03:06:13]

+Luis - any chance it's OK to land this yet?

[Anand Mistry (off Chromium), 2016-06-14 05:33:17]

On 2016/06/09 03:06:13, Ken Rockot wrote:
> +Luis - any chance it's OK to land this yet?

FYI, not back-propagating port closures is the root cause of a bug I've been
tracking down: https://bugs.chromium.org/p/chromium/issues/detail?id=618206

[Ken Rockot(use gerrit already), 2016-06-14 05:46:26]

On 2016/06/14 at 05:33:17, amistry wrote:
> On 2016/06/09 03:06:13, Ken Rockot wrote:
> > +Luis - any chance it's OK to land this yet?
> 
> FYI, not back-propagating port closures is the root cause of a bug I've been
tracking down: https://bugs.chromium.org/p/chromium/issues/detail?id=618206

There are 3 CLs blocking our ability to land this. It looks like they've all
made some progress in the last 24 hours:

https://chromium-review.googlesource.com/#/c/349101/
https://chromium-review.googlesource.com/#/c/346457/
https://chromium-review.googlesource.com/#/c/346763/

[Ken Rockot(use gerrit already), 2016-06-20 20:35:25]

Description was changed from

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864
R=amistry@chromium.org
==========

to

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864,618206
R=amistry@chromium.org
==========

[Luis Héctor Chávez, 2016-06-23 17:51:18]

On 2016/06/14 05:46:26, Ken Rockot wrote:
> On 2016/06/14 at 05:33:17, amistry wrote:
> > On 2016/06/09 03:06:13, Ken Rockot wrote:
> > > +Luis - any chance it's OK to land this yet?
> > 
> > FYI, not back-propagating port closures is the root cause of a bug I've been
> tracking down: https://bugs.chromium.org/p/chromium/issues/detail?id=618206
> 
> There are 3 CLs blocking our ability to land this. It looks like they've all
> made some progress in the last 24 hours:
> 
> https://chromium-review.googlesource.com/#/c/349101/
> https://chromium-review.googlesource.com/#/c/346457/
> https://chromium-review.googlesource.com/#/c/346763/

FYI just now Android and Chrome OS are synced to r395517

[Ken Rockot(use gerrit already), 2016-06-23 17:57:10]

Woohoo! Thank you :)

[Ken Rockot(use gerrit already), 2016-06-23 19:22:05]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-23 19:22:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
amistry@chromium.org, ochang@chromium.org
Link to the patchset: https://codereview.chromium.org/1975073002/#ps320001
(title: ".")

[commit-bot: I haz the power, 2016-06-23 19:22:39]

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

[commit-bot: I haz the power, 2016-06-23 20:14:47]

Committed patchset #14 (id:320001)

[commit-bot: I haz the power, 2016-06-23 20:17:35]

Description was changed from

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864,618206
R=amistry@chromium.org
==========

to

==========
[mojo-edk]  Broadcast surprise port disruptions

If a node connection is lost, all ports which have peers
on that node become useless and we destroy them. This can
in turn lead to other ports (ports which have the now-destroyed
ports as peers) becoming useless themselves.

Previously we had no way to back-propagate awareness of this
condition. This CL introduces a broadcast mechanism to facilitate
said back-propagation.

BUG=589864,618206
R=amistry@chromium.org

Committed: https://crrev.com/3a9157501ed6f0d4de687908919be8111408b53c
Cr-Commit-Position: refs/heads/master@{#401698}
==========

[commit-bot: I haz the power, 2016-06-23 20:17:36]

Patchset 14 (id:??) landed as
https://crrev.com/3a9157501ed6f0d4de687908919be8111408b53c
Cr-Commit-Position: refs/heads/master@{#401698}