Make WebDataConsumerHandleImpl return OK while zero-length reading of closed pipe.

Make WebDataConsumerHandleImpl return OK while zero-length reading of closed pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558
Committed: https://crrev.com/21cb200fb6ef1ea455ac5a2e820a1ed4fcdea5e5
Cr-Commit-Position: refs/heads/master@{#421706}

[horo, 2016-09-23 03:45:33]

Description was changed from

==========
Make ReaderImpl::read() return Ok when the size is 0 and the producer handle was
closed.

BUG=649558
==========

to

==========
Make WebDataConsumerHandleImpl return OK while zero-length reading of closed
pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558
==========

[horo, 2016-09-23 03:45:39]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 03:45:55]

Dry run: 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-09-23 04:32:18]

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

[commit-bot: I haz the power, 2016-09-23 04:32:19]

Dry run: This issue passed the CQ dry run.

[horo, 2016-09-23 05:53:57]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2016-09-23 05:53:57]

yhirano@
Could you please review this?

[yhirano, 2016-09-23 06:55:55]

lgtm

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
content/child/web_data_consumer_handle_impl_unittest.cc:203: static int
kDataPipeCapacity;
+ constexpr

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
content/child/web_data_consumer_handle_impl_unittest.cc:285: const size_t
data_size = kDataPipeCapacity - 1;
constexpr

[horo, 2016-09-23 07:38:26]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 07:38:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-09-23 07:38:37]

Thank you.

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
File content/child/web_data_consumer_handle_impl_unittest.cc (right):

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
content/child/web_data_consumer_handle_impl_unittest.cc:203: static int
kDataPipeCapacity;
On 2016/09/23 06:55:55, yhirano wrote:
> + constexpr

Done.

https://codereview.chromium.org/2363963002/diff/1/content/child/web_data_cons...
content/child/web_data_consumer_handle_impl_unittest.cc:285: const size_t
data_size = kDataPipeCapacity - 1;
On 2016/09/23 06:55:55, yhirano wrote:
> constexpr

Done.

[horo, 2016-09-23 07:40:53]

horo@chromium.org changed reviewers:
+ esprehn@chromium.org

[horo, 2016-09-23 07:40:54]

esprehn@

Could you please review this?

[commit-bot: I haz the power, 2016-09-23 08:17:58]

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

[commit-bot: I haz the power, 2016-09-23 08:17:59]

Dry run: This issue passed the CQ dry run.

[horo, 2016-09-28 00:04:35]

On 2016/09/23 07:40:54, horo wrote:
> esprehn@
> 
> Could you please review this?

Ping?

[esprehn, 2016-09-28 04:04:14]

esprehn@chromium.org changed reviewers:
+ rockot@chromium.org
- esprehn@chromium.org

[esprehn, 2016-09-28 04:04:14]

rockot@ might be a better reviewer? I'm happy to stamp after.

[Ken Rockot(use gerrit already), 2016-09-28 05:34:12]

If you want to test for peer closure without it feeling like a hack, you can
call:

MojoWait(handle, MOJO_HANDLE_SIGNAL_PEER_CLOSED,
         0, nullptr);

This will never actually "wait" on anything because the deadline is 0, so it's
always safe to call.

If the peer is known to be closed, this will return MOJO_RESULT_OK. Otherwise
it'll return MOJO_RESULT_SHOULD_WAIT.

[horo, 2016-09-28 08:16:53]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[horo, 2016-09-28 08:16:53]

On 2016/09/28 05:34:12, Ken Rockot wrote:
> If you want to test for peer closure without it feeling like a hack, you can
> call:
> 
> MojoWait(handle, MOJO_HANDLE_SIGNAL_PEER_CLOSED,
>          0, nullptr);
> 
> This will never actually "wait" on anything because the deadline is 0, so it's
> always safe to call.
> 
> If the peer is known to be closed, this will return MOJO_RESULT_OK. Otherwise
> it'll return MOJO_RESULT_SHOULD_WAIT.

Uploaded Patch Set 3.
We want to test whether the handle is readable or not even if the peer was
closed.
So Patch Set 3 is using mojo::Wait(handle, MOJO_HANDLE_SIGNAL_READABLE, 0,
nullptr)

[Ken Rockot(use gerrit already), 2016-09-28 13:18:29]

lgtm

[horo, 2016-09-28 14:01:39]

On 2016/09/28 13:18:29, Ken Rockot wrote:
> lgtm

Thank you.

[horo, 2016-09-28 14:02:44]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 14:02:58]

Dry run: 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-09-28 14:09:31]

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

[commit-bot: I haz the power, 2016-09-28 14:09:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-09-29 00:16:35]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 00:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-09-29 00:21:39]

The CQ bit was unchecked by horo@chromium.org

[horo, 2016-09-29 00:21:50]

The CQ bit was checked by horo@chromium.org

[horo, 2016-09-29 00:21:50]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2363963002/#ps40001
(title: "use mojo::Wait() with 0 deadline to check whether readable or not")

[commit-bot: I haz the power, 2016-09-29 00:22:21]

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-09-29 00:30:50]

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

[commit-bot: I haz the power, 2016-09-29 00:30:51]

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

[horo, 2016-09-29 01:06:27]

horo@chromium.org changed reviewers:
+ esprehn@chromium.org

[horo, 2016-09-29 01:06:28]

+esprehn@

Please :)

[esprehn, 2016-09-29 01:17:30]

lgtm

[horo, 2016-09-29 01:23:48]

On 2016/09/29 01:17:30, esprehn wrote:
> lgtm

thank you!

[horo, 2016-09-29 01:23:56]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-09-29 01:24:28]

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-09-29 01:38:52]

Description was changed from

==========
Make WebDataConsumerHandleImpl return OK while zero-length reading of closed
pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558
==========

to

==========
Make WebDataConsumerHandleImpl return OK while zero-length reading of closed
pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558
==========

[commit-bot: I haz the power, 2016-09-29 01:38:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-29 01:42:20]

Description was changed from

==========
Make WebDataConsumerHandleImpl return OK while zero-length reading of closed
pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558
==========

to

==========
Make WebDataConsumerHandleImpl return OK while zero-length reading of closed
pipe.

Even if there is unread data available, mojo::ReadDataRaw() returns
FAILED_PRECONDITION when |size| is 0 and the producer handle was closed. But in
this case, WebDataConsumerHandle::Reader::read() must return OK.

BytesConsumerForDataConsumerHandle::didGetReadable() calls zero-length read to
check the handle's status. If ReaderImpl::read() returns Done, the consumer
handle will be closed even if there is unread data available.

BUG=649558

Committed: https://crrev.com/21cb200fb6ef1ea455ac5a2e820a1ed4fcdea5e5
Cr-Commit-Position: refs/heads/master@{#421706}
==========

[commit-bot: I haz the power, 2016-09-29 01:42:22]

Patchset 3 (id:??) landed as
https://crrev.com/21cb200fb6ef1ea455ac5a2e820a1ed4fcdea5e5
Cr-Commit-Position: refs/heads/master@{#421706}