IPC::ChannelMojo: Don't supress MOJO_RESULT_FAILED_PRECONDITION

IPC::ChannelMojo: Don't supress MOJO_RESULT_FAILED_PRECONDITION

The error should be reported to IPC::Listener as it is a
signal of the dead peer. Without this, the browser cannot
detect renderer crashes.

TEST=browser_tests (with ChannelMojo enabled)
R=viettrungluu@chromium.org,darin@chromium.org
BUG=none

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

[viettrungluu, 2014-08-08 00:36:12]

LGTM w/question.

https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
ipc/mojo/ipc_message_pipe_reader.cc:70: if (wait_result != MOJO_RESULT_ABORTED)
{
This is fine, but do you ever expect to see MOJO_RESULT_ABORTED? (If so, when?)

[Hajime Morrita, 2014-08-08 01:27:18]

On 2014/08/08 00:36:12, viettrungluu wrote:
> LGTM w/question.
> 
>
https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
> File ipc/mojo/ipc_message_pipe_reader.cc (right):
> 
>
https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
> ipc/mojo/ipc_message_pipe_reader.cc:70: if (wait_result !=
MOJO_RESULT_ABORTED)
> {
> This is fine, but do you ever expect to see MOJO_RESULT_ABORTED? (If so,
when?)

I added this because it happens occasionally in the test.
Seems like this is signaled when we run the event loop after closing the
channel.
I thinks that makes sense because waiter cancellation happens in a different
thread.

[Hajime Morrita, 2014-08-08 01:27:21]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-08 01:32:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/453643003/1

[commit-bot: I haz the power, 2014-08-08 04:36:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-08 04:41:35]

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

[commit-bot: I haz the power, 2014-08-08 04:41:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[viettrungluu, 2014-08-08 04:58:13]

On 2014/08/08 01:27:18, morrita wrote:
> On 2014/08/08 00:36:12, viettrungluu wrote:
> > LGTM w/question.
> > 
> >
>
https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
> > File ipc/mojo/ipc_message_pipe_reader.cc (right):
> > 
> >
>
https://codereview.chromium.org/453643003/diff/1/ipc/mojo/ipc_message_pipe_re...
> > ipc/mojo/ipc_message_pipe_reader.cc:70: if (wait_result !=
> MOJO_RESULT_ABORTED)
> > {
> > This is fine, but do you ever expect to see MOJO_RESULT_ABORTED? (If so,
> when?)
> 
> I added this because it happens occasionally in the test.
> Seems like this is signaled when we run the event loop after closing the
> channel.
> I thinks that makes sense because waiter cancellation happens in a different
> thread.

The worry is that if you're seeing this, it means that the handle was closed on
a different thread. There's an inherent race condition in doing so: the close
might happen before you start waiting.

The likely consequence of this is that you'll see MOJO_RESULT_INVALID_PARAMETER,
which isn't necessarily disastrous. The bigger concern is that it's not
impossible that the handle (value) could be reused for some different object, in
which case you'd end up waiting on some other object. (Currently, this is
unlikely, since handle values are allocated sequentially. Sequential allocation
isn't guaranteed, and there are good arguments for, e.g., random allocation
instead, which would increase the odds of quick handle value reuse.)

[Hajime Morrita, 2014-08-08 18:05:48]

On 2014/08/08 04:58:13, viettrungluu wrote:
> The worry is that if you're seeing this, it means that the handle was closed
on
> a different thread. There's an inherent race condition in doing so: the close
> might happen before you start waiting.
> 
> The likely consequence of this is that you'll see
MOJO_RESULT_INVALID_PARAMETER,
> which isn't necessarily disastrous. The bigger concern is that it's not
> impossible that the handle (value) could be reused for some different object,
in
> which case you'd end up waiting on some other object. (Currently, this is
> unlikely, since handle values are allocated sequentially. Sequential
allocation
> isn't guaranteed, and there are good arguments for, e.g., random allocation
> instead, which would increase the odds of quick handle value reuse.)

Makes sense. Let me explain my understanding first to ensure I understand the
problem correctly:
- IPC::MessagePipeReader MojoAsyncWaiter::CancelWait() and then close the
MessagePipe immediately
  in the same (IO) thread. As the CancelWait(), and its backend WatcherBackend,
  does the real work in the different (mojo pump) thread, it can happen after
the handle is closed.
  That is problem: It can touch dead handle.

There are possible solutions:
a. Let AsyncWaiter tells its client when cancellation is finished, and let the
client wait the notification
   before closing the handle. This is simple for AsyncWaiter but a bit tricky
for the client. Or...

b. Add Close() API to AsyncWaiter which closes given handle in the mojo-pump
thread.
   This is easy for the client but feels a bit strange... The handle is close in
a different thread from
   its creation thread.

c. Make handles ref-countable. Generic, but might be overkill.

What do you think?

I file a bug to track this.
https://code.google.com/p/chromium/issues/detail?id=402040
Let me land this first then attack it separately.

[viettrungluu, 2014-08-08 18:23:03]

On 2014/08/08 18:05:48, morrita wrote:
> On 2014/08/08 04:58:13, viettrungluu wrote:
> > The worry is that if you're seeing this, it means that the handle was closed
> on
> > a different thread. There's an inherent race condition in doing so: the
close
> > might happen before you start waiting.
> > 
> > The likely consequence of this is that you'll see
> MOJO_RESULT_INVALID_PARAMETER,
> > which isn't necessarily disastrous. The bigger concern is that it's not
> > impossible that the handle (value) could be reused for some different
object,
> in
> > which case you'd end up waiting on some other object. (Currently, this is
> > unlikely, since handle values are allocated sequentially. Sequential
> allocation
> > isn't guaranteed, and there are good arguments for, e.g., random allocation
> > instead, which would increase the odds of quick handle value reuse.)
> 
> Makes sense. Let me explain my understanding first to ensure I understand the
> problem correctly:
> - IPC::MessagePipeReader MojoAsyncWaiter::CancelWait() and then close the
> MessagePipe immediately
>   in the same (IO) thread. As the CancelWait(), and its backend
WatcherBackend,
>   does the real work in the different (mojo pump) thread, it can happen after
> the handle is closed.
>   That is problem: It can touch dead handle.

Sorry, I totally missed the fact that you're using MojoAsyncWaiter. In theory,
that should do the right thing -- when you call CancelWait(), it'll stop waiting
and won't return control to you until you/your thread has full ownership of the
handle. (The Chromium implementation does this by posting a task to the handle
watcher thread and waiting.)

As far as I can tell, callbacks of MojoAsyncWaiter will only get
MOJO_RESULT_CANCELLED if you start another (async) wait on the same handle. (I
don't know if this is the correct/desired. The standalone implementation doesn't
do this.) If you see MOJO_RESULT_CANCELLED, it's then presumably because you did
an async wait on a handle with an async wait already outstanding....

> 
> There are possible solutions:
> a. Let AsyncWaiter tells its client when cancellation is finished, and let the
> client wait the notification
>    before closing the handle. This is simple for AsyncWaiter but a bit tricky
> for the client. Or...
> 
> b. Add Close() API to AsyncWaiter which closes given handle in the mojo-pump
> thread.
>    This is easy for the client but feels a bit strange... The handle is close
in
> a different thread from
>    its creation thread.
> 
> c. Make handles ref-countable. Generic, but might be overkill.
> 
> What do you think?
> 
> I file a bug to track this.
> https://code.google.com/p/chromium/issues/detail?id=402040
> Let me land this first then attack it separately.

I think you can close that bug, since I totally misunderstood your code! (This
change is still good to land.)

[Hajime Morrita, 2014-08-08 21:35:35]

> Sorry, I totally missed the fact that you're using MojoAsyncWaiter. In theory,
> that should do the right thing -- when you call CancelWait(), it'll stop
waiting
> and won't return control to you until you/your thread has full ownership of
the
> handle. (The Chromium implementation does this by posting a task to the handle
> watcher thread and waiting.)
> 
> As far as I can tell, callbacks of MojoAsyncWaiter will only get
> MOJO_RESULT_CANCELLED if you start another (async) wait on the same handle. (I
> don't know if this is the correct/desired. The standalone implementation
doesn't
> do this.) If you see MOJO_RESULT_CANCELLED, it's then presumably because you
did
> an async wait on a handle with an async wait already outstanding....
> 
Oh right. I overlooked wait() call. It should work then.

> 
> I think you can close that bug, since I totally misunderstood your code! (This
> change is still good to land.)
All right! Landing.

[Hajime Morrita, 2014-08-08 21:35:38]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-08 21:38:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/453643003/1

[commit-bot: I haz the power, 2014-08-08 21:45:21]

Change committed as 288442