[mojo-edk] Post a task to process local messages.

[mojo-edk] Post a task to process local messages.

This ensures the local message queue is _always_ processed, instead of
relying on the side effect of certain functions such as SendMessage.

BUG=612500
Committed: https://crrev.com/a18ca6b383554d7806787028c0961858e90bb086
Cr-Commit-Position: refs/heads/master@{#404281}

[Anand Mistry (off Chromium), 2016-07-06 05:14:51]

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

[Anand Mistry (off Chromium), 2016-07-06 05:14:51]

Highly speculative, wildly accusational and completely unsubstantiated fix for
sync call renderer hangs. Approve at your peril.

[Ken Rockot(use gerrit already), 2016-07-06 20:03:08]

On 2016/07/06 at 05:14:51, amistry wrote:
> Highly speculative, wildly accusational and completely unsubstantiated fix for
sync call renderer hangs. Approve at your peril.

Hmm. I'm a bit worried that this will spam the task queue with mostly
unnecessary tasks.

I see we're missing AcceptIncomingMessages() specifically from
OnRequestPortMerge, which is the only incoming channel message that directly
calls into node_ and thus needs to pump messages. Several message handlers may
also call DropPeer() directly, and this should be pumping messages as well.

Those are the only missing calls I see. They are significant though and I like
your speculation that this may well be the cause of our hangs. I'd rather just
fix these paths for now and avoid posting tasks.

[Anand Mistry (off Chromium), 2016-07-07 00:01:41]

On 2016/07/06 20:03:08, Ken Rockot wrote:
> On 2016/07/06 at 05:14:51, amistry wrote:
> > Highly speculative, wildly accusational and completely unsubstantiated fix
for
> sync call renderer hangs. Approve at your peril.
> 
> Hmm. I'm a bit worried that this will spam the task queue with mostly
> unnecessary tasks.
> 
> I see we're missing AcceptIncomingMessages() specifically from
> OnRequestPortMerge, which is the only incoming channel message that directly
> calls into node_ and thus needs to pump messages. Several message handlers may
> also call DropPeer() directly, and this should be pumping messages as well.
> 
> Those are the only missing calls I see. They are significant though and I like
> your speculation that this may well be the cause of our hangs. I'd rather just
> fix these paths for now and avoid posting tasks.

Two things:
1. Spam is minimized by only posting on an empty message queue. And there's an
upper bound on tasks which is one per local message. Since local messages are
relatively rare (i.e. collapsed ports), I don't consider this to be a
significant issue.
2. The goal of this CL is to capture any forgotten cases and _guarantee_ the
messages will be processed. I've always found the current approach to be very
brittle since it requires you to scatter AcceptIncomingMessages() wherever you
expect these messages to occur. I think that should be treated as an
optimisation, and my opinion is that optimisations should always have a safe
fall-back path.

Oh, and #3. I added tracking and ran browser_tests. It didn't find anything.
Then again, browser_tests don't exercise all code paths, so that's not really a
result.

[Ken Rockot(use gerrit already), 2016-07-07 04:57:45]

On 2016/07/07 at 00:01:41, amistry wrote:
> On 2016/07/06 20:03:08, Ken Rockot wrote:
> > On 2016/07/06 at 05:14:51, amistry wrote:
> > > Highly speculative, wildly accusational and completely unsubstantiated fix
for
> > sync call renderer hangs. Approve at your peril.
> > 
> > Hmm. I'm a bit worried that this will spam the task queue with mostly
> > unnecessary tasks.
> > 
> > I see we're missing AcceptIncomingMessages() specifically from
> > OnRequestPortMerge, which is the only incoming channel message that directly
> > calls into node_ and thus needs to pump messages. Several message handlers
may
> > also call DropPeer() directly, and this should be pumping messages as well.
> > 
> > Those are the only missing calls I see. They are significant though and I
like
> > your speculation that this may well be the cause of our hangs. I'd rather
just
> > fix these paths for now and avoid posting tasks.
> 
> Two things:
> 1. Spam is minimized by only posting on an empty message queue. And there's an
upper bound on tasks which is one per local message. Since local messages are
relatively rare (i.e. collapsed ports), I don't consider this to be a
significant issue.
> 2. The goal of this CL is to capture any forgotten cases and _guarantee_ the
messages will be processed. I've always found the current approach to be very
brittle since it requires you to scatter AcceptIncomingMessages() wherever you
expect these messages to occur. I think that should be treated as an
optimisation, and my opinion is that optimisations should always have a safe
fall-back path.
> 
> Oh, and #3. I added tracking and ran browser_tests. It didn't find anything.
Then again, browser_tests don't exercise all code paths, so that's not really a
result.

OK, I'm convinced. LGTM!

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

The CQ bit was checked by amistry@chromium.org

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

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, 2016-07-07 07:03:01]

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

[commit-bot: I haz the power, 2016-07-07 07:03:02]

Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Anand Mistry (off Chromium), 2016-07-08 00:31:50]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-07-08 00:33:36]

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, 2016-07-08 00:47:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-08 00:50:05]

Description was changed from

==========
[mojo-edk] Post a task to process local messages.

This ensures the local message queue is _always_ processed, instead of
relying on the side effect of certain functions such as SendMessage.

BUG=612500
==========

to

==========
[mojo-edk] Post a task to process local messages.

This ensures the local message queue is _always_ processed, instead of
relying on the side effect of certain functions such as SendMessage.

BUG=612500

Committed: https://crrev.com/a18ca6b383554d7806787028c0961858e90bb086
Cr-Commit-Position: refs/heads/master@{#404281}
==========

[commit-bot: I haz the power, 2016-07-08 00:50:06]

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