workaround for mac kernel bug

Alternative workaround for mac kernel bug.

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

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

[hubbe, 2013-09-30 23:56:08]

This is an alternate workaround for the mac sendmsg() bug, which uses an ACK to
close the file descriptor.

[hubbe, 2013-10-03 16:25:48]

The simpler version of this fix seems to have caused a bunch of tests to fail
and got reverted. This fix should hopefully not have that problem.

Please take another look.

[cpu_(ooo_6.6-7.5), 2013-10-04 17:58:30]

Can you repro the failures locally?

We want to understand what happened so we can code a unittest that exercises
that case.

[hubbe, 2013-10-04 21:38:50]

On 2013/10/04 17:58:30, cpu wrote:
> Can you repro the failures locally?
> 
> We want to understand what happened so we can code a unittest that exercises
> that case.

Yes, I can reproduce the failures locally, but not reliably.
One way to produce the error is to run:

out/Debug/performance_browser_tests --gtest_filter='*TabCapturePerformanceTe*'
--gtest_repeat=100  --enable-gpu --gtest_also_run_disabled_tests

Some of the tests will time out, as the renderer hangs in
ipc_sync_message_filter.cc when the gpu fd is closed upon receiving it.  The
result is that nothing is rendered in the tab and the test times out.
(What happens is that gpu_render_host.cc calls Send() on a 
GpuChannelMsg_CreateOffscreenCommandBuffer, but in ipc_sync_message_filter.cc,
OnMessageReceived is never called because the first read on the FD returns zero.
 I think there is another bug here, because logically, this should generate an
OnChannelError and/or OnChannelClosing, which should let the thread return and
deal with the error, but that does not happen.)


Alternatively, we could take my program from
http://code.google.com/p/chromium/issues/detail?id=298276 and make it into a
unit tests that tests the behavior from a kernel standpoint, but I suspect
that's not what you want.

Probably the best approach to writing a unit test is to just create three
threads and sort of mimic what my test program does:

 thread1:  while(1) { p1, p2 = socketpair();  send_to_thread2(p1);
p2->write('x'); close(p2); }
 thread2:  while(1) { send_to_thread3(read_fd_from_thread1()); }
 thread3:  while(1) { fd = read_fd_from_thread2(); assert(fd->getc() == 'x');
close(fd); }

In my test program, this usually triggers the bug within 100 iterations or so,
so running it for a a second should be more than sufficient. I'm going to take a
look at the existing unit tests and see if I can put something like this
together.

[cpu_(ooo_6.6-7.5), 2013-10-04 22:06:07]

Sounds good.

[Scott Hess - ex-Googler, 2013-10-07 17:35:58]

Sorry for the lag - been OOT.  I'll try to get to this todayish.

[hubbe1, 2013-10-07 18:25:21]

FYI: I'm working on adding a test.



On Mon, Oct 7, 2013 at 10:35 AM, <shess@chromium.org> wrote:

> Sorry for the lag - been OOT.  I'll try to get to this todayish.
>
>
https://chromiumcodereview.**appspot.com/25325002/<https://chromiumcodereview...
>

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

[Scott Hess - ex-Googler, 2013-10-09 20:18:32]

I apologize, but it's taking me some time to swap things into memory.  I haven't
been this deep in the IPC code for awhile.  I need to sit down somewhere quiet
and think about it.

It might be worthwhile to trawl around for someone else like agl to review it in
addition to me.  I don't think being an OSX person gives me any special leverage
here, and there are probably better people on the IPC side.  I'll keep grinding
on it, though.

Also, AFAICT after this first pass, this only depends on messages being in
order, so things could also be phrased as a PING which returns a blob of data
you sent.  I'm not sure that really improves things, but something like a PING
might be more reasonable to just plumb through everywhere.  But I will admit, I
do not understand hows |hops| is used (though even that has precedent in a
PING's TTL :-).

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h#newcode90
ipc/ipc_channel.h:90: CLOSE_FD_MESSAGE_TYPE = kuint16max - 1
internall -> internally.

I think the comment is saying why this is here, but not what it does.  It seems
like if there's going to be a second one, maybe the comments should be
refactored to indicate the entire enum is about internal messages (with high
numbers to not conflict), and then have per-message comments with the
definitions.

Could you use HELLO_MESSAGE_TYPE - 1?  Presumably nobody else will define
another one, but you never know...

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:358: // descptor. For more information, see:
descriptor

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:684: if (!is_blocked_on_write_) {
I am nowhere close to understanding whether the changes in this function
replicate the original behavior.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:995: std::set<int>::iterator i =
fds_to_close_.find(fd);
You only make use of i in one case, which leads to erase().  You could probably
put this in the hops == 0 case, like:
  if (fds_to_close_.erase(fd) > 0) {
    if (HANDLE_EINTR(close(fd)) < 0)
      PLOG(ERROR) << "close";
  } else {
    NOTREACHED();
  }

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:1006: }
break;

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h#n...
ipc/ipc_channel_posix.h:193: // trying to bamboozle us.
Capitalize Keep.

[hubbe, 2013-10-09 21:33:58]

I'm not sure if ping messages are the right choice here, we want something that
can't
be mistaken for another message and is internal to the IPC implementation. I
don't see
where ping messages are defined, do they live up to these requirements?

    /Hubbe

On 2013/10/09 20:18:32, shess wrote:
> I apologize, but it's taking me some time to swap things into memory.  I
haven't
> been this deep in the IPC code for awhile.  I need to sit down somewhere quiet
> and think about it.
> 
> It might be worthwhile to trawl around for someone else like agl to review it
in
> addition to me.  I don't think being an OSX person gives me any special
leverage
> here, and there are probably better people on the IPC side.  I'll keep
grinding
> on it, though.
> 
> Also, AFAICT after this first pass, this only depends on messages being in
> order, so things could also be phrased as a PING which returns a blob of data
> you sent.  I'm not sure that really improves things, but something like a PING
> might be more reasonable to just plumb through everywhere.  But I will admit,
I
> do not understand hows |hops| is used (though even that has precedent in a
> PING's TTL :-).
> 
> https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h
> File ipc/ipc_channel.h (right):
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h#newcode90
> ipc/ipc_channel.h:90: CLOSE_FD_MESSAGE_TYPE = kuint16max - 1
> internall -> internally.
> 
> I think the comment is saying why this is here, but not what it does.  It
seems
> like if there's going to be a second one, maybe the comments should be
> refactored to indicate the entire enum is about internal messages (with high
> numbers to not conflict), and then have per-message comments with the
> definitions.
> 
> Could you use HELLO_MESSAGE_TYPE - 1?  Presumably nobody else will define
> another one, but you never know...
> 
> https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc
> File ipc/ipc_channel_posix.cc (right):
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
> ipc/ipc_channel_posix.cc:358: // descptor. For more information, see:
> descriptor
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
> ipc/ipc_channel_posix.cc:684: if (!is_blocked_on_write_) {
> I am nowhere close to understanding whether the changes in this function
> replicate the original behavior.
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
> ipc/ipc_channel_posix.cc:995: std::set<int>::iterator i =
> fds_to_close_.find(fd);
> You only make use of i in one case, which leads to erase().  You could
probably
> put this in the hops == 0 case, like:
>   if (fds_to_close_.erase(fd) > 0) {
>     if (HANDLE_EINTR(close(fd)) < 0)
>       PLOG(ERROR) << "close";
>   } else {
>     NOTREACHED();
>   }
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
> ipc/ipc_channel_posix.cc:1006: }
> break;
> 
> https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h
> File ipc/ipc_channel_posix.h (right):
> 
>
https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h#n...
> ipc/ipc_channel_posix.h:193: // trying to bamboozle us.
> Capitalize Keep.

[hubbe, 2013-10-09 21:34:11]

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel.h#newcode90
ipc/ipc_channel.h:90: CLOSE_FD_MESSAGE_TYPE = kuint16max - 1
On 2013/10/09 20:18:33, shess wrote:
> internall -> internally.
> 
> I think the comment is saying why this is here, but not what it does.  It
seems
> like if there's going to be a second one, maybe the comments should be
> refactored to indicate the entire enum is about internal messages (with high
> numbers to not conflict), and then have per-message comments with the
> definitions.
> 
> Could you use HELLO_MESSAGE_TYPE - 1?  Presumably nobody else will define
> another one, but you never know...

Done.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:358: // descptor. For more information, see:
On 2013/10/09 20:18:33, shess wrote:
> descriptor

Done.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:684: if (!is_blocked_on_write_) {
On 2013/10/09 20:18:33, shess wrote:
> I am nowhere close to understanding whether the changes in this function
> replicate the original behavior.

It's fairly easy to reason out:

When we enter OnFileCanReadWithoutBlocking, we can be in one of two states:

1. is_blocked_on_write_ is true
2. output_queue_ is empty

When we get to the end of OnFileCanReadWithoutBlocking, it is possible that some
data has been queued (a hello message or a closefd message)  if so, we should
make sure that those messages get sent at some point.  If is_blocked_on_write_
is true, then there is already a callback pending to ProcessOutgoingMessage()
and we don't need to do anything. If not, we call ProcessOutgoingMessage() which
will either empty the queue or set is_blocked_on_write_ to true, thus returning
us to a good state.

(Note that ProcessOutgoingMessage() will do nothing if the queue is empty, so we
don't really need to test if there is a message to process or not.)

There is one caveat, in the previous code send_server_hello_msg was set to false
in one case to avoid calling ProcessOutgoingMessage() if the object has already
been destroyed. That case is now taken care of with a return.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:995: std::set<int>::iterator i =
fds_to_close_.find(fd);
On 2013/10/09 20:18:33, shess wrote:
> You only make use of i in one case, which leads to erase().  You could
probably
> put this in the hops == 0 case, like:
>   if (fds_to_close_.erase(fd) > 0) {
>     if (HANDLE_EINTR(close(fd)) < 0)
>       PLOG(ERROR) << "close";
>   } else {
>     NOTREACHED();
>   }
> 

Yep, much better, done.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:1006: }
On 2013/10/09 20:18:33, shess wrote:
> break;

Done.

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/25325002/diff/25001/ipc/ipc_channel_posix.h#n...
ipc/ipc_channel_posix.h:193: // trying to bamboozle us.
On 2013/10/09 20:18:33, shess wrote:
> Capitalize Keep.

Done.

[hubbe, 2013-10-09 21:34:51]

Adding agl@ as reviewer

[agl, 2013-10-09 21:52:37]

LGTM

https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
File ipc/file_descriptor_set_posix.h (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
ipc/file_descriptor_set_posix.h:82: void GetFDsToCloseAndClear(std::vector<int>*
fds);
This looks more like a "release" operation than a "get" operation. If it were a
"get" then it could be const.

ReleaseFDsToClose()?

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h#newcode84
ipc/ipc_channel.h:84: HELLO_MESSAGE_TYPE = kuint16max,  // Maximum value of
message type (uint16),
// lines misaligned.

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:370: 
this file only has a single newline after functions.

[hubbe, 2013-10-09 21:59:39]

https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
File ipc/file_descriptor_set_posix.h (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
ipc/file_descriptor_set_posix.h:82: void GetFDsToCloseAndClear(std::vector<int>*
fds);
On 2013/10/09 21:52:38, agl wrote:
> This looks more like a "release" operation than a "get" operation. If it were
a
> "get" then it could be const.
> 
> ReleaseFDsToClose()?

Done.

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h#newcode84
ipc/ipc_channel.h:84: HELLO_MESSAGE_TYPE = kuint16max,  // Maximum value of
message type (uint16),
On 2013/10/09 21:52:38, agl wrote:
> // lines misaligned.

better?

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc
File ipc/ipc_channel_posix.cc (right):

https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc#...
ipc/ipc_channel_posix.cc:370: 
On 2013/10/09 21:52:38, agl wrote:
> this file only has a single newline after functions.

Done.

[hubbe, 2013-10-10 16:52:28]

On 2013/10/09 21:59:39, hubbe wrote:
>
https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
> File ipc/file_descriptor_set_posix.h (right):
> 
>
https://codereview.chromium.org/25325002/diff/44001/ipc/file_descriptor_set_p...
> ipc/file_descriptor_set_posix.h:82: void
GetFDsToCloseAndClear(std::vector<int>*
> fds);
> On 2013/10/09 21:52:38, agl wrote:
> > This looks more like a "release" operation than a "get" operation. If it
were
> a
> > "get" then it could be const.
> > 
> > ReleaseFDsToClose()?
> 
> Done.
> 
> https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h
> File ipc/ipc_channel.h (right):
> 
>
https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel.h#newcode84
> ipc/ipc_channel.h:84: HELLO_MESSAGE_TYPE = kuint16max,  // Maximum value of
> message type (uint16),
> On 2013/10/09 21:52:38, agl wrote:
> > // lines misaligned.
> 
> better?
> 
> https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc
> File ipc/ipc_channel_posix.cc (right):
> 
>
https://codereview.chromium.org/25325002/diff/44001/ipc/ipc_channel_posix.cc#...
> ipc/ipc_channel_posix.cc:370: 
> On 2013/10/09 21:52:38, agl wrote:
> > this file only has a single newline after functions.
> 
> Done.

Does anybody have anything to add to this review, if not I'm going to (attempt)
to check it in later today.

         /Hubbe

[agl, 2013-10-10 16:59:49]

On Thu, Oct 10, 2013 at 12:52 PM,  <hubbe@chromium.org> wrote:
>> better?

LGTM.

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

[cpu_(ooo_6.6-7.5), 2013-10-10 17:26:19]

lgtm and thanks for keep hammering at this.

[commit-bot: I haz the power, 2013-10-10 17:34:19]

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

[Scott Hess - ex-Googler, 2013-10-10 21:03:54]

lgtm.  this was a good find...

[commit-bot: I haz the power, 2013-10-10 21:12:18]

Change committed as 227999

[hubbe, 2013-10-14 19:42:50]

On 2013/10/10 21:12:18, I haz the power (commit-bot) wrote:
> Change committed as 227999

This got reverted, anybody mind if I re-use and re-submit this CL since the fix
is very small?

[commit-bot: I haz the power, 2013-10-14 21:52:59]

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

[commit-bot: I haz the power, 2013-10-15 00:07:01]

Change committed as 228569