Last set of fixes to make the src/mojo/edk pass the page cycler.

Last set of fixes to make the src/mojo/edk pass the page cycler.

-remove the different code paths in RawChannelWin for using IOCP or manual reset. We know we definitely can't use the former. The latter is too slow and we know for sure now that auto-reset works.
-after calling CancelIO(Ex) synchronously wait for the result. Otherwise we would sometimes miss notification that a page was read, leading to gaps in in data read from the pipes.
-call RegisterWaitForSingleObject directly instead of using ObjectWatcher, to make the above possible. It would have made the API for ObjectWatcher too ugly to add this support there.
-ensure that if there's a pending post task to set the error state, it is serialized as well. otherwise the consumer of the mojo primitive will never see a peer-closed signal, leading to hangs.
-ensure we don't call the delegate's OnError more than once. this could lead to UAFs.
-ensure that the read/write lock are acquired before modifying pending_read_/pending_write_, otherwise there are races where the IO thread set it to false before calling into RawChannel::OnReadCompleted/OnWriteCompleted, and ReleaseHandle didn't see that there was pending IO

BUG=478251
Committed: https://crrev.com/447c7923b1b92c978f5dc594939b407d6c4686c2
Cr-Commit-Position: refs/heads/master@{#354191}

[jam, 2015-10-14 00:54:32]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2015-10-14 18:23:46]

a few nits. I will wait for your update to do a second rounds of review. Thanks!

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.cc (right):

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.cc:339: pending_error_ = true;
pending_error_ is never reset to false, is that intended?

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.h (right):

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:226: void OnReadCompleted(IOResult io_result,
size_t bytes_read);
Does it make sense tor remove On(Read|Write)Completed? (I understand they are
now still called on posix, but seems easy to change the callsites.)

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:227: // Like above, except that the caller needs
to acquire read_lock_ first.
style nit: please use |some_variable_name_| (and line 234).

[jam, 2015-10-14 20:01:44]

ptal

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.cc (right):

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.cc:339: pending_error_ = true;
On 2015/10/14 18:23:45, yzshen1 wrote:
> pending_error_ is never reset to false, is that intended?

yep

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.h (right):

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:226: void OnReadCompleted(IOResult io_result,
size_t bytes_read);
On 2015/10/14 18:23:46, yzshen1 wrote:
> Does it make sense tor remove On(Read|Write)Completed? (I understand they are
> now still called on posix, but seems easy to change the callsites.)

good point. done.

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:227: // Like above, except that the caller needs
to acquire read_lock_ first.
On 2015/10/14 18:23:46, yzshen1 wrote:
> style nit: please use |some_variable_name_| (and line 234).

I stopped this since it's not in the style guide; just some preferences that
some people use. I find it obvious without :)

[yzshen1, 2015-10-14 21:37:01]

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.h (right):

https://codereview.chromium.org/1403033003/diff/40001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:227: // Like above, except that the caller needs
to acquire read_lock_ first.
On 2015/10/14 20:01:44, jam wrote:
> On 2015/10/14 18:23:46, yzshen1 wrote:
> > style nit: please use |some_variable_name_| (and line 234).
> 
> I stopped this since it's not in the style guide; just some preferences that
> some people use. I find it obvious without :)

I see, although I think consistency within the file prevails.
Please consider this as an optional comment. :)

https://codereview.chromium.org/1403033003/diff/60001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.h (right):

https://codereview.chromium.org/1403033003/diff/60001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:382: // Acquires read_lock_ and calls
OnReadCompleted.
nit: OnReadCompleted -> OnReadCompletedNoLock

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel_win.cc (right):

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:283: handle_.reset();
Just to make sure I understand: does this part need to be changed in later CLs
according to our discussion about "write error doesn't stop reads"?

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:334: } else if (error == ERROR_BROKEN_PIPE 
|| error == STATUS_PIPE_BROKEN) {
I vaguely remember that you mentioned that at the end we only need one error
code here, right?

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:430: that->read_event_signalled_ = true;
Is it safe to set the value outside of lock? ReleaseHandle could check the value
on any thread, right?

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:442: that->write_event_signalled_ = true;
ditto.

[jam, 2015-10-14 22:35:36]

https://codereview.chromium.org/1403033003/diff/60001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel.h (right):

https://codereview.chromium.org/1403033003/diff/60001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel.h:382: // Acquires read_lock_ and calls
OnReadCompleted.
On 2015/10/14 21:37:00, yzshen1 wrote:
> nit: OnReadCompleted -> OnReadCompletedNoLock

Done.

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
File mojo/edk/system/raw_channel_win.cc (right):

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:283: handle_.reset();
On 2015/10/14 21:37:00, yzshen1 wrote:
> Just to make sure I understand: does this part need to be changed in later CLs
> according to our discussion about "write error doesn't stop reads"?

yep

https://codereview.chromium.org/1403033003/diff/80001/mojo/edk/system/raw_cha...
mojo/edk/system/raw_channel_win.cc:430: that->read_event_signalled_ = true;
On 2015/10/14 21:37:00, yzshen1 wrote:
> Is it safe to set the value outside of lock? ReleaseHandle could check the
value
> on any thread, right?

ReleaseHandle only checks it after calling UnregisterWaitEx, which guarantees
that if this callback would have run, it already has completed.

[yzshen1, 2015-10-14 22:46:37]

lgtm

[jam, 2015-10-14 22:58:58]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-10-14 23:00:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403033003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403033003/100001

[commit-bot: I haz the power, 2015-10-15 00:04:11]

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

[commit-bot: I haz the power, 2015-10-15 00:04:12]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jam, 2015-10-15 00:11:27]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-10-15 00:12:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403033003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403033003/100001

[commit-bot: I haz the power, 2015-10-15 01:24:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-15 01:25:27]

Patchset 6 (id:??) landed as
https://crrev.com/447c7923b1b92c978f5dc594939b407d6c4686c2
Cr-Commit-Position: refs/heads/master@{#354191}