Set error on WebDataConsumerHandle while loading body.

Set error on WebDataConsumerHandle while loading body.

This CL makes the associated WebDataConsumerHandle errored when
the loading fails or it is canceled.

This CL also:
 - makes closing notification asynchronous.
 - implements some more error checks, especially for two-phase read
   sessions.
 - removes the deprecated implementation for the old API.

BUG=480746
Committed: https://crrev.com/9fb5ecf62351a235b723c15c65343c895e8e7640
Cr-Commit-Position: refs/heads/master@{#335477}

[yhirano, 2015-06-11 11:44:40]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-06-11 11:44:42]

Patchset #2 (id:40001) has been deleted

[yhirano, 2015-06-11 11:44:55]

Patchset #2 (id:60001) has been deleted

[yhirano, 2015-06-11 11:48:56]

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

[yhirano, 2015-06-11 11:51:47]

This CL depends on https://codereview.chromium.org/1164493008/.

[hiroshige, 2015-06-18 07:01:21]

As discussed offline, Notify() might call didGetReadable() synchronously and
this might cause reentrant to didGetReadable().

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:279:
context_->set_result(UnexpectedError);
Is this change for setting errors on read()/beginRead() during two-phase read
(which was previously unchecked) and thus independent from error notification to
WebDataConsumerHandle?
If so, please mention this in the CL description.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:309:
context_->set_result(UnexpectedError);
ditto.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:329: return UnexpectedError;
Can/should we call ClearQueue() in this then clause? (not sure)

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:676:
reader->endRead(size);
Should we add EXPECT_EQ(kUnexpectedError) for the return value?

[hiroshige, 2015-06-18 07:01:21]

As discussed offline, Notify() might call didGetReadable() synchronously and
this might cause reentrant to didGetReadable().

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:242: // Otherwise, clear the
data.
Please mention in the comment when/how ClearQueue() is called when
is_two_phase_read_in_progress() is true.
(In my understandings, if two-phase read is ongoing then ClearQueue() is called
in endRead() because we set_result(UnexpectedError) here.)

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:279:
context_->set_result(UnexpectedError);
Is this change for setting errors on read()/beginRead() during two-phase read
(which was previously unchecked) and thus independent from error notification to
WebDataConsumerHandle?
If so, please mention this in the CL description.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:309:
context_->set_result(UnexpectedError);
ditto.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:329: return UnexpectedError;
Can/should we call ClearQueue() in this then clause? (not sure)

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:676:
reader->endRead(size);
Should we add EXPECT_EQ(kUnexpectedError) for the return value?

[yhirano, 2015-06-18 08:08:00]

Patchset #5 (id:140001) has been deleted

[yhirano, 2015-06-18 08:12:38]

Patchset #4 (id:120001) has been deleted

[yhirano, 2015-06-18 08:13:33]

PS4 removes deprecated implementations.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:242: // Otherwise, clear the
data.
On 2015/06/18 07:01:20, hiroshige wrote:
> Please mention in the comment when/how ClearQueue() is called when
> is_two_phase_read_in_progress() is true.
> (In my understandings, if two-phase read is ongoing then ClearQueue() is
called
> in endRead() because we set_result(UnexpectedError) here.)

Done.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:279:
context_->set_result(UnexpectedError);
On 2015/06/18 07:01:21, hiroshige wrote:
> Is this change for setting errors on read()/beginRead() during two-phase read
> (which was previously unchecked) and thus independent from error notification
to
> WebDataConsumerHandle?
> If so, please mention this in the CL description.

Done. By the way, I added a condition to stabilize the result.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:309:
context_->set_result(UnexpectedError);
On 2015/06/18 07:01:21, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:329: return UnexpectedError;
On 2015/06/18 07:01:20, hiroshige wrote:
> Can/should we call ClearQueue() in this then clause? (not sure)

I do not want to do so, because it may complicate the code. Clearing the queue
is not mandatory in general: it should be cleared in the destruction process. I
added early-clear when closed / errored for performance. In L328 case, a user
does something wrong. I don't want to implement such optimization for bad users.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:676:
reader->endRead(size);
On 2015/06/18 07:01:21, hiroshige wrote:
> Should we add EXPECT_EQ(kUnexpectedError) for the return value?

Currently it returns Ok and I'm fine with it. I don't have a strong opinion on
this return value.

[yhirano, 2015-06-18 08:14:51]

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:329: return UnexpectedError;
> I added early-clear when closed / errored
Correction: I added early-clear when errored

[hiroshige, 2015-06-18 08:35:11]

lgtm with a optional comment.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:329: return UnexpectedError;
On 2015/06/18 08:14:51, yhirano wrote:
> > I added early-clear when closed / errored
> Correction: I added early-clear when errored
> 

OK.

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:676:
reader->endRead(size);
On 2015/06/18 08:13:33, yhirano wrote:
> On 2015/06/18 07:01:21, hiroshige wrote:
> > Should we add EXPECT_EQ(kUnexpectedError) for the return value?
> 
> Currently it returns Ok and I'm fine with it. I don't have a strong opinion on
> this return value.

OK; endRead() returns OK because we can read 'Once ' here, and next read() would
return UnexpectedError; reasonable.

optional: check that next beginRead() returns UnexpectedError.

[yhirano, 2015-06-18 08:40:55]

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/100001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:676:
reader->endRead(size);
On 2015/06/18 08:35:11, hiroshige wrote:
> On 2015/06/18 08:13:33, yhirano wrote:
> > On 2015/06/18 07:01:21, hiroshige wrote:
> > > Should we add EXPECT_EQ(kUnexpectedError) for the return value?
> > 
> > Currently it returns Ok and I'm fine with it. I don't have a strong opinion
on
> > this return value.
> 
> OK; endRead() returns OK because we can read 'Once ' here, and next read()
would
> return UnexpectedError; reasonable.
> 
> optional: check that next beginRead() returns UnexpectedError.

Done.

[yhirano, 2015-06-18 08:41:45]

Could you review https://codereview.chromium.org/1181973005/ as this CL is
blocked by it?

[yhirano, 2015-06-18 08:42:03]

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

[yhirano, 2015-06-18 08:42:04]

+kinuko for OWNER reivew.

[hiroshige, 2015-06-18 08:42:24]

https://codereview.chromium.org/1181573003/diff/180001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/180001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:682:
EXPECT_EQ(kUnexpectedError, reader->beginRead(&pointer, kNon, &size));
nit: s/kNon/kNone/?

[yhirano, 2015-06-18 09:12:37]

https://codereview.chromium.org/1181573003/diff/180001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle_unittest.cc (right):

https://codereview.chromium.org/1181573003/diff/180001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle_unittest.cc:682:
EXPECT_EQ(kUnexpectedError, reader->beginRead(&pointer, kNon, &size));
On 2015/06/18 08:42:24, hiroshige wrote:
> nit: s/kNon/kNone/?

Oops, fixed

[kinuko, 2015-06-22 06:39:28]

lgtm

(sorry for slow stamping)

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:32: // Note: We assume
|AddData| is not called in a client's didGetReadable
nit: this comment is a bit ambiguous, who's 'we' and who's expected to guarantee
the assumption?

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:37: void Error();
nit: it's subjective but 'error' sounds more like noun than verb, Fail() or
OnError() would be slightly more common

[yhirano, 2015-06-22 07:08:13]

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:32: // Note: We assume
|AddData| is not called in a client's didGetReadable
On 2015/06/22 06:39:27, kinuko wrote:
> nit: this comment is a bit ambiguous, who's 'we' and who's expected to
guarantee
> the assumption?

Fixed: Writer assumes that / A caller should ensure that.

https://codereview.chromium.org/1181573003/diff/190001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:37: void Error();
On 2015/06/22 06:39:27, kinuko wrote:
> nit: it's subjective but 'error' sounds more like noun than verb, Fail() or
> OnError() would be slightly more common

Done.

[yhirano, 2015-06-22 07:41:49]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-06-22 07:41:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1181573003/#ps250001
(title: " ")

[commit-bot: I haz the power, 2015-06-22 07:42:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181573003/250001

[commit-bot: I haz the power, 2015-06-22 08:39:07]

Committed patchset #9 (id:250001)

[commit-bot: I haz the power, 2015-06-22 08:39:52]

Patchset 9 (id:??) landed as
https://crrev.com/9fb5ecf62351a235b723c15c65343c895e8e7640
Cr-Commit-Position: refs/heads/master@{#335477}