[Mojo-Loading] OnStartLoadingResponseBody should be called after OnReceiveResponse

[Mojo-Loading] OnStartLoadingResponseBody should be called after OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler calls OnStartLoadingResponseBody when
OnResponseStarted is called, and that means OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
Review-Url: https://codereview.chromium.org/2633123002
Cr-Commit-Position: refs/heads/master@{#444638}
Committed: https://chromium.googlesource.com/chromium/src/+/246883ff6add126d1f6482fe5f883a2d6a2b2525

[yhirano, 2017-01-16 10:29:50]

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

[commit-bot: I haz the power, 2017-01-16 10:30:01]

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

[yhirano, 2017-01-16 10:34:01]

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

[commit-bot: I haz the power, 2017-01-16 10:34:17]

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

[yhirano, 2017-01-16 10:38:39]

Description was changed from

==========
[Mojo-Loading] Ensure OnStartLoadingResponseBody is called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler called OnStartLoadingResponseBody when
OnResponseStarted is called, and that meant OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

to

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler called OnStartLoadingResponseBody when
OnResponseStarted is called, and that meant OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

[yhirano, 2017-01-16 10:38:57]

Description was changed from

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler called OnStartLoadingResponseBody when
OnResponseStarted is called, and that meant OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

to

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler called OnStartLoadingResponseBody when
OnResponseStarted is called, and that meant OnStartLoadingResponseBody could be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

[yhirano, 2017-01-16 10:41:45]

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

[commit-bot: I haz the power, 2017-01-16 11:39:28]

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

[commit-bot: I haz the power, 2017-01-16 11:39:28]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-01-17 12:52:03]

lgtm

[yhirano, 2017-01-17 13:15:51]

yhirano@chromium.org changed reviewers:
+ dcheng@chromium.org, jam@chromium.org, mmenke@chromium.org

[yhirano, 2017-01-17 13:15:52]

+dcheng@ for content/common/url_loader.mojom
+mmenke@ for content/browser/loader
+jam@ for content/child

[yhirano, 2017-01-17 13:37:16]

Description was changed from

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler called OnStartLoadingResponseBody when
OnResponseStarted is called, and that meant OnStartLoadingResponseBody could be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

to

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler calls OnStartLoadingResponseBody when
OnResponseStarted is called, and that means OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

[mmenke, 2017-01-17 16:13:57]

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:188:
std::move(data_pipe_consumer_handle_));
std::move leaves the original unique_ptr pointer in an indeterminate state,
doesn't it?  Is data_pipe_consumer_handle_.is_valid() guaranteed to work after
calling std::move on it?

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:217:
std::move(data_pipe.consumer_handle));
Is there any significant perf penalty with just always doing this in the first
MojoAsyncResourceHandler::OnResponseStarted call?  I prefer keeping the paths as
similar as we can, so it's less likely the paths will break independently of
each other.

(On a side note, last month, rdsmith was discussing fixing the ordering here by
having MimeSniffingResourceHandler not call OnWillRead early, and instead use
its own buffer in this case, which would allow us not to do this juggling.  Also
unclear what the perf cost of that would be.  That would also let us get rid of
the min_size parameter, I believe)

[Randy Smith (Not in Mondays), 2017-01-17 16:45:10]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2017-01-17 16:45:11]

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:217:
std::move(data_pipe.consumer_handle));
On 2017/01/17 16:13:57, mmenke wrote:
> Is there any significant perf penalty with just always doing this in the first
> MojoAsyncResourceHandler::OnResponseStarted call?  I prefer keeping the paths
as
> similar as we can, so it's less likely the paths will break independently of
> each other.
> 
> (On a side note, last month, rdsmith was discussing fixing the ordering here
by
> having MimeSniffingResourceHandler not call OnWillRead early, and instead use
> its own buffer in this case, which would allow us not to do this juggling. 
Also
> unclear what the perf cost of that would be.  That would also let us get rid
of
> the min_size parameter, I believe)

If you wanted to switch course, I don't currently think that that option would
be much more complicated than this one, and it also would remove implicit
dependencies through the ResourceHandler stack on OnWillRead() returning a large
enough sized buffer for MimeSniffingResourceHandler.  If you do this, I'd
suggest putting in UMA just so we can confirm we're not killing ourselves with
too much copying.

(I started to rough out the approach on the plane yesterday, but it's a side
project I got into because the implicit dependencies annoyed me, and it's
unlikely I'll actually finish it before I leave on vacation in a week.)

[Randy Smith (Not in Mondays), 2017-01-17 16:45:27]

rdsmith@chromium.org changed reviewers:
- rdsmith@chromium.org

[dcheng, 2017-01-17 23:20:18]

comment-only mojo changes lgtm

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:188:
std::move(data_pipe_consumer_handle_));
On 2017/01/17 16:13:57, mmenke wrote:
> std::move leaves the original unique_ptr pointer in an indeterminate state,
> doesn't it?  Is data_pipe_consumer_handle_.is_valid() guaranteed to work after
> calling std::move on it?

Note that std::unique_ptr's moved from state is actually well-specified in
[unique.ptr]/4:

Additionally, u can, upon request, transfer ownership to another unique pointer
u2. Upon completion of
such a transfer, the following postconditions hold:
— u2.p is equal to the pre-transfer u.p,
— u.p is equal to nullptr, and
— if the pre-transfer u.d maintained state, such state has been transferred to
u2.d.

That being said, this is a good question to ask of movable types in general
(though from brief inspection, it appears that the same holds true for the mojo
scoped helpers)

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.h:123:
mojo::ScopedDataPipeConsumerHandle data_pipe_consumer_handle_;
If this data pipe is just for transferring the response body, it might be nice
to give it a less generic name.

[kinuko, 2017-01-18 01:17:11]

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

[kinuko, 2017-01-18 01:17:11]

content/child lgtm

[yhirano, 2017-01-18 06:53:12]

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

[commit-bot: I haz the power, 2017-01-18 06:53:25]

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

[yhirano, 2017-01-18 08:01:32]

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

[commit-bot: I haz the power, 2017-01-18 08:02:02]

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

[yhirano, 2017-01-18 09:01:09]

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

[commit-bot: I haz the power, 2017-01-18 09:01:20]

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

[yhirano, 2017-01-18 09:10:45]

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:188:
std::move(data_pipe_consumer_handle_));
On 2017/01/17 16:13:57, mmenke wrote:
> std::move leaves the original unique_ptr pointer in an indeterminate state,
> doesn't it?  Is data_pipe_consumer_handle_.is_valid() guaranteed to work after
> calling std::move on it?

Done (in OnReadCompleted), though I'm not sure if it's needed. According to the
ScopedHandleBase comments,

"
  It also provides (emulated) move semantics, again 
  along the lines of C++11's |unique_ptr| (and exactly
  like Chromium's |scoped_ptr|).
"

so I expect that it will be cleared when consumed by a move-constructor.

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:217:
std::move(data_pipe.consumer_handle));
On 2017/01/17 16:45:10, Randy Smith - Not in Mondays wrote:
> On 2017/01/17 16:13:57, mmenke wrote:
> > Is there any significant perf penalty with just always doing this in the
first
> > MojoAsyncResourceHandler::OnResponseStarted call?  I prefer keeping the
paths
> as
> > similar as we can, so it's less likely the paths will break independently of
> > each other.
> > 
> > (On a side note, last month, rdsmith was discussing fixing the ordering here
> by
> > having MimeSniffingResourceHandler not call OnWillRead early, and instead
use
> > its own buffer in this case, which would allow us not to do this juggling. 
> Also
> > unclear what the perf cost of that would be.  That would also let us get rid
> of
> > the min_size parameter, I believe)
> 
> If you wanted to switch course, I don't currently think that that option would
> be much more complicated than this one, and it also would remove implicit
> dependencies through the ResourceHandler stack on OnWillRead() returning a
large
> enough sized buffer for MimeSniffingResourceHandler.  If you do this, I'd
> suggest putting in UMA just so we can confirm we're not killing ourselves with
> too much copying.
> 
> (I started to rough out the approach on the plane yesterday, but it's a side
> project I got into because the implicit dependencies annoyed me, and it's
> unlikely I'll actually finish it before I leave on vacation in a week.)

I found that moving this to OnReadCompleted simplified the code (Sending the
message from OnResponseStarted unconditionally may increase memory consumption
for responses without body). Done.

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.h (right):

https://codereview.chromium.org/2633123002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.h:123:
mojo::ScopedDataPipeConsumerHandle data_pipe_consumer_handle_;
On 2017/01/17 23:20:18, dcheng wrote:
> If this data pipe is just for transferring the response body, it might be nice
> to give it a less generic name.

Done.

[commit-bot: I haz the power, 2017-01-18 09:55:30]

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

[commit-bot: I haz the power, 2017-01-18 09:55:31]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-18 16:26:12]

LGTM as-is, but a couple comments/questions.

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:216:
!data_pipe.consumer_handle.is_valid()) {
Random comments on the mojo API:

C++ constructors that can fail seem a bit weird.  Something like
mojo::CreateDataPipe() that returns a null DataPipe on failure seems a bit more
natural.  (Or bool mojo::CreateDataPipes(options, &producer_handle,
&consumer_handle) WARN_UNUSED_RESULT)

Also, is it really possible for only one of these to be valid?

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:216:
!data_pipe.consumer_handle.is_valid()) {
Do we have any tests where this fails?

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:685:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission2) {
Why did you remove this test?

[yhirano, 2017-01-19 03:24:08]

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

[commit-bot: I haz the power, 2017-01-19 03:24:27]

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

[yhirano, 2017-01-19 03:25:50]

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:216:
!data_pipe.consumer_handle.is_valid()) {
On 2017/01/18 16:26:12, mmenke wrote:
> Random comments on the mojo API:
> 
> C++ constructors that can fail seem a bit weird.  Something like
> mojo::CreateDataPipe() that returns a null DataPipe on failure seems a bit
more
> natural.  (Or bool mojo::CreateDataPipes(options, &producer_handle,
> &consumer_handle) WARN_UNUSED_RESULT)
> 
> Also, is it really possible for only one of these to be valid?

It looks impossible:
https://cs.chromium.org/chromium/src/mojo/public/cpp/system/data_pipe.h?sq=pa...
has its own DCHECK. I replaced the branch with DCHECKs.

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (left):

https://codereview.chromium.org/2633123002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:685:
TEST_F(MojoAsyncResourceHandlerTest, ResponseErrorDuringBodyTransmission2) {
On 2017/01/18 16:26:12, mmenke wrote:
> Why did you remove this test?

This test doesn't have an OnReadCompleted call. With this change
OnResponseBodyArrived doesn't arrive and so the test makes no sense.

[yhirano, 2017-01-19 03:25:59]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2017-01-19 03:25:59]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-01-19 03:25:59]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
dcheng@chromium.org, tzik@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2633123002/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2017-01-19 03:26:27]

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, 2017-01-19 04:19:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484796359498410,
"parent_rev": "ac86748560893dddd38ef596512fa90e56232302", "commit_rev":
"246883ff6add126d1f6482fe5f883a2d6a2b2525"}

[commit-bot: I haz the power, 2017-01-19 04:19:50]

Description was changed from

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler calls OnStartLoadingResponseBody when
OnResponseStarted is called, and that means OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396
==========

to

==========
[Mojo-Loading] OnStartLoadingResponseBody should be called after
OnReceiveResponse

In content::ResourceHandler, OnWillRead can be called before OnResponseStarted.
MojoAsyncResourceHandler calls OnStartLoadingResponseBody when
OnResponseStarted is called, and that means OnStartLoadingResponseBody can be
called before OnReceiveResponse.

That complicates clients' code. With this CL, it is guaranteed that
URLLoaderClient::OnStartLoadingResponseBody is called after
URLLoaderClient::OnReceiveResponse.

BUG=603396

Review-Url: https://codereview.chromium.org/2633123002
Cr-Commit-Position: refs/heads/master@{#444638}
Committed:
https://chromium.googlesource.com/chromium/src/+/246883ff6add126d1f6482fe5f88...
==========

[commit-bot: I haz the power, 2017-01-19 04:19:51]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/246883ff6add126d1f6482fe5f88...