Fix posix IPC channel hanging problem.

Fix posix IPC channel hanging problem.

If a channel closes right before a send call, listeners might not be notified of
the problem, which can cause hangs. This CL fixes that and adds a test that makes
sure that this does not happen in the future.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248060

[hubbe, 2013-10-18 22:49:31]

Not sure if this ever happens, except when the mac kernel goes berserk. But it
occurred to me that there was two problems: 1 the mac kernel had a bug, 2 the
IPC layer got stuck instead of dying gracefully.  This CL fixes #2.

The first upload contains the test, should fail on trybots, the second upload
contains the fix, which should make the test work again.

[cpu_(ooo_6.6-7.5), 2013-10-18 23:23:25]

plus scott

[hubbe, 2013-10-21 23:41:53]

On 2013/10/18 23:23:25, cpu wrote:
> plus scott

Ping?

[Scott Hess - ex-Googler, 2013-10-22 23:52:45]

LGTM, I think.  I'm a little out of my depth, though.

https://codereview.chromium.org/30133002/diff/40001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/30133002/diff/40001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:713: return ProcessOutgoingMessages();
Could this have the same race condition if the other end closes after accept but
before ProcessOutgoingMessages()?

[hubbe, 2013-10-23 00:29:27]

https://codereview.chromium.org/30133002/diff/40001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/30133002/diff/40001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:713: return ProcessOutgoingMessages();
On 2013/10/22 23:52:46, shess wrote:
> Could this have the same race condition if the other end closes after accept
but
> before ProcessOutgoingMessages()?

Yes, I think so.
I'll see if I can trigger this in a test.

[cpu_(ooo_6.6-7.5), 2013-10-23 20:18:45]

lgtm

[commit-bot: I haz the power, 2013-10-23 20:19:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/40001

[commit-bot: I haz the power, 2013-10-23 21:58:58]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[cpu_(ooo_6.6-7.5), 2013-11-11 23:15:57]

is this CL abandoned?

[hubbe, 2013-11-11 23:41:58]

On 2013/11/11 23:15:57, cpu wrote:
> is this CL abandoned?

No, but it's been on an extended vacation since I've been quite busy with other
stuff.
I'll pick it up again soon.

[hubbe, 2013-11-12 01:10:28]

On 2013/11/11 23:41:58, hubbe wrote:
> On 2013/11/11 23:15:57, cpu wrote:
> > is this CL abandoned?
> 
> No, but it's been on an extended vacation since I've been quite busy with
other
> stuff.
> I'll pick it up again soon.

Added test & fix for the other race that shess pointed out.
PTAL.

    /Hubbe

[cpu_(ooo_6.6-7.5), 2013-11-13 21:18:26]

lgtm

[Scott Hess - ex-Googler, 2013-11-13 21:33:22]

lgtm.

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
File ipc/ipc_channel_posix_unittest.cc (right):

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
ipc/ipc_channel_posix_unittest.cc:252: in_chan.Close();  // simulate remote
process dying at an unforunate time.
sp "unfortunate".

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
ipc/ipc_channel_posix_unittest.cc:261: // If a connection closes right before an
Connect() call,
"a Connect() call".  Also paragraph formatting looks weird WRT previous test's
comment.

[hubbe, 2013-11-20 20:21:21]

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
File ipc/ipc_channel_posix_unittest.cc (right):

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
ipc/ipc_channel_posix_unittest.cc:252: in_chan.Close();  // simulate remote
process dying at an unforunate time.
On 2013/11/13 21:33:22, shess wrote:
> sp "unfortunate".

Done.

https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
ipc/ipc_channel_posix_unittest.cc:261: // If a connection closes right before an
Connect() call,
On 2013/11/13 21:33:22, shess wrote:
> "a Connect() call".  Also paragraph formatting looks weird WRT previous test's
> comment.

Done.

[hubbe, 2013-11-26 18:12:27]

On 2013/11/20 20:21:21, hubbe wrote:
>
https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
> File ipc/ipc_channel_posix_unittest.cc (right):
> 
>
https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
> ipc/ipc_channel_posix_unittest.cc:252: in_chan.Close();  // simulate remote
> process dying at an unforunate time.
> On 2013/11/13 21:33:22, shess wrote:
> > sp "unfortunate".
> 
> Done.
> 
>
https://codereview.chromium.org/30133002/diff/440001/ipc/ipc_channel_posix_un...
> ipc/ipc_channel_posix_unittest.cc:261: // If a connection closes right before
an
> Connect() call,
> On 2013/11/13 21:33:22, shess wrote:
> > "a Connect() call".  Also paragraph formatting looks weird WRT previous
test's
> > comment.
> 
> Done.

I had to do one more (minor) things to make all tests pass.
Please take a look before I submit.

[Scott Hess - ex-Googler, 2013-11-26 18:21:09]

lgtm

[commit-bot: I haz the power, 2013-11-27 20:02:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 01:14:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 01:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 02:16:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 02:24:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 02:56:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 03:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[commit-bot: I haz the power, 2013-11-28 03:57:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Scott Hess - ex-Googler, 2014-01-29 19:13:51]

Any news on this?

https://codereview.chromium.org/131513018/ wants to fix the same thing (I
think).

[hubbe1, 2014-01-29 19:47:37]

I haven't given up, but fixing things is harder than I thought, because it
is sometimes difficult to detect if the ipc channel is not yet open or have
already been closed.



On Wed, Jan 29, 2014 at 11:13 AM, <shess@chromium.org> wrote:

> Any news on this?
>
> https://codereview.chromium.org/131513018/ wants to fix the same thing (I
> think).
>
> https://codereview.chromium.org/30133002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fredrik Öhrn, 2014-01-30 09:45:25]

On 2014/01/29 19:13:51, shess wrote:
> Any news on this?
> 
> https://codereview.chromium.org/131513018/ wants to fix the same thing (I
> think).

Correct.

It triggers a crash in Opera for Android (crbug.com/338709) so we'd really like
to see it fixed.

[commit-bot: I haz the power, 2014-01-30 19:57:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/730001

[hubbe, 2014-01-30 19:57:25]

On 2014/01/30 09:45:25, Fredrik Öhrn wrote:
> On 2014/01/29 19:13:51, shess wrote:
> > Any news on this?
> > 
> > https://codereview.chromium.org/131513018/ wants to fix the same thing (I
> > think).
> 
> Correct.
> 
> It triggers a crash in Opera for Android (crbug.com/338709) so we'd really
like
> to see it fixed.

I thought there was some more work to do on this, but now that I look at it I
cannot remember
what it was, and tests seem to pass, so I'm going to try to submit again.

[commit-bot: I haz the power, 2014-01-30 20:20:49]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-30 20:20:59]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 20:59:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hubbe@chromium.org/30133002/750001

[commit-bot: I haz the power, 2014-01-30 22:31:16]

Change committed as 248060

[commit-bot: I haz the power, 2014-01-30 22:31:20]

CQ bit was unchecked on CL. Ignoring.

[hubbe, 2014-01-30 22:59:43]

A revert of this CL has been created in
https://codereview.chromium.org/150883002/ by hubbe@chromium.org.

The reason for reverting is: Seems to cause some leaks.
Reverting while I investigate.
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASAN%20Tests%20%...
.