Further investigation reveals that the ObserveProxy behavior is WAI but is leading to a cycle because an external message which would break the cycle is never processed. This occurs because AcceptIncomingMessages effectively blocks the IO thread pumping the cycle.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cdb85fdae4f9f9487d67d080b794ca633219303

commit 5cdb85fdae4f9f9487d67d080b794ca633219303
Author: rockot <rockot@chromium.org>
Date: Fri Jul 15 17:07:45 2016

[mojo-edk] Prevent AcceptIncomingMessages flushing aggressively

Changes AcceptIncomingMessages to only flush the incoming
message queue once synchronously. This prevents deadlock cycles
that can block incoming external messages by spinning the IO
thread indefinitely. It also prevents similar cycles from
chewing up CPU on other threads.

Also fixes some Watcher tests that are broken by
ProcessIncomingMessages behavior. This change made the
brokenness more apparent.

BUG= 628492 
R=amistry@chromium.org

Review-Url: https://codereview.chromium.org/2153733002
Cr-Commit-Position: refs/heads/master@{#405776}

[modify] https://crrev.com/5cdb85fdae4f9f9487d67d080b794ca633219303/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/5cdb85fdae4f9f9487d67d080b794ca633219303/mojo/edk/system/node_controller.h
[modify] https://crrev.com/5cdb85fdae4f9f9487d67d080b794ca633219303/mojo/edk/system/watch_unittest.cc

This bug can cause any thread to get stuck spinning in a loop indefinitely. It's a relatively rare condition but it's been observed in the wild. It was only noticed and fixed in M54 due to changes which happened to trigger it much more frequently.

The fix is small and may eliminate some hangs in M53 and M52. I would like to merge it back.

Your change meets the bar and is auto-approved for M53 (branch: 2785)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c27ce502f0b6912b43be47a57400c87d168e81a

commit 2c27ce502f0b6912b43be47a57400c87d168e81a
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Aug 03 16:11:47 2016

[mojo-edk] Prevent AcceptIncomingMessages flushing aggressively

Changes AcceptIncomingMessages to only flush the incoming
message queue once synchronously. This prevents deadlock cycles
that can block incoming external messages by spinning the IO
thread indefinitely. It also prevents similar cycles from
chewing up CPU on other threads.

Also fixes some Watcher tests that are broken by
ProcessIncomingMessages behavior. This change made the
brokenness more apparent.

BUG= 628492 
R=amistry@chromium.org

Review-Url: https://codereview.chromium.org/2153733002
Cr-Commit-Position: refs/heads/master@{#405776}

TBR=yzshen@chromium.org

Review URL: https://codereview.chromium.org/2209733002 .

Cr-Commit-Position: refs/branch-heads/2785@{#486}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/2c27ce502f0b6912b43be47a57400c87d168e81a/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/2c27ce502f0b6912b43be47a57400c87d168e81a/mojo/edk/system/node_controller.h
[modify] https://crrev.com/2c27ce502f0b6912b43be47a57400c87d168e81a/mojo/edk/system/watch_unittest.cc

This change causes regression: https://bugs.chromium.org/p/chromium/issues/detail?id=635508

Thanks. I'm already in the process of reverting it, but other changes need
to land with the revert or there will be other regressions. Should be fixed
within a day.

Reopening this. The merge was reverted from branch 2785, and I'm going to revert the original CL from master as well.

I have a different fix for the underlying bug that will land along with the revert. I'll merge that fix into M53. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36f85398960e7836d5325e1a58298209ced34f33

commit 36f85398960e7836d5325e1a58298209ced34f33
Author: rockot <rockot@chromium.org>
Date: Tue Aug 09 02:26:52 2016

[mojo-edk] Revert ObserveProxy retransmission behavior

Gets rid of ObserveProxy retransmission which was only required
to facilitate excessive randomization in tests. In practice this
behavior can race with normal port closure and lead to busy
loops.

Gets rid of excessive randomization in tests. The new PortsTest
environment more closely emulates reality: Nodes operate
independently but there are some basic and trivially satisfiable
ordering guarantees.

Gets rid of deferred AcceptIncomingMessages pumping in
NodeController, which was only added to facilitate
ObserveProxy retransmission.

BUG= 628492 ,635508

Review-Url: https://codereview.chromium.org/2219733005
Cr-Commit-Position: refs/heads/master@{#410535}

[modify] https://crrev.com/36f85398960e7836d5325e1a58298209ced34f33/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/36f85398960e7836d5325e1a58298209ced34f33/mojo/edk/system/node_controller.h
[modify] https://crrev.com/36f85398960e7836d5325e1a58298209ced34f33/mojo/edk/system/ports/node.cc
[modify] https://crrev.com/36f85398960e7836d5325e1a58298209ced34f33/mojo/edk/system/ports/node.h
[modify] https://crrev.com/36f85398960e7836d5325e1a58298209ced34f33/mojo/edk/system/ports/ports_unittest.cc

Requesting to merge r410535 into branch 2785.

Before we approve merge to M53, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Also is this change applicable to all OS or any specific OS?

This change applies to all OSes. It doesn't look like it made it into the last canary push so I'll wait for the next one to go out. It is however reasonably safe to merge since it's merely a revert of previously landed changes.

Thank you ken. Waiting for this to bake in Canary as per our chat. Please update the bug with Canary result. Thank you.

Your change meets the bar and is auto-approved for M53 (branch: 2785)

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This has had time to bake, seems fine. I think the merge approval may have been premature, but I'll go ahead with a merge now if there's no opposition.

Please go ahead and merge to M53 branch 2785. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/573af98298255787657627c25a78a5de1676da73

commit 573af98298255787657627c25a78a5de1676da73
Author: Ken Rockot <rockot@chromium.org>
Date: Wed Aug 10 23:16:16 2016

[mojo-edk] Revert ObserveProxy retransmission behavior

Gets rid of ObserveProxy retransmission which was only required
to facilitate excessive randomization in tests. In practice this
behavior can race with normal port closure and lead to busy
loops.

Gets rid of excessive randomization in tests. The new PortsTest
environment more closely emulates reality: Nodes operate
independently but there are some basic and trivially satisfiable
ordering guarantees.

Gets rid of deferred AcceptIncomingMessages pumping in
NodeController, which was only added to facilitate
ObserveProxy retransmission.

BUG= 628492 ,635508

Review-Url: https://codereview.chromium.org/2219733005
Cr-Commit-Position: refs/heads/master@{#410535}
(cherry picked from commit 36f85398960e7836d5325e1a58298209ced34f33)

Review URL: https://codereview.chromium.org/2236473003 .

Cr-Commit-Position: refs/branch-heads/2785@{#558}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/573af98298255787657627c25a78a5de1676da73/mojo/edk/system/node_controller.cc
[modify] https://crrev.com/573af98298255787657627c25a78a5de1676da73/mojo/edk/system/ports/node.cc
[modify] https://crrev.com/573af98298255787657627c25a78a5de1676da73/mojo/edk/system/ports/node.h
[modify] https://crrev.com/573af98298255787657627c25a78a5de1676da73/mojo/edk/system/ports/ports_unittest.cc