New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687240 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Make ResourceHandler::OnWillRead able to complete asynchronously

Project Member Reported by mmenke@chromium.org, Jan 31 2017

Issue description

This simplifies the Mojo code a bit, and it makes it consistent with all other ResourceHandler methods, which always use ResourceController for resuming and cancellation, rather than returning a bool.

The conversion isn't too difficult, but will require tests for everything that calls the method and can see async results (ResourceLoader, InterceptingRH, DetachableRH, MimeSniffingRH, and Mojo test, too, if I switch that over to being able to allocate the buffer asychronously in the same CL - on the fence about that, may be better to split it off in another CL).
 

Comment 1 by mmenke@chromium.org, Jan 31 2017

Components: Internals>Network
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59420d8f9e1521355d98b03cbf2748f7ad0dbb07

commit 59420d8f9e1521355d98b03cbf2748f7ad0dbb07
Author: mmenke <mmenke@chromium.org>
Date: Thu Feb 02 00:41:04 2017

Fix DetachableResourceHandler when OnResponseCompleted is deferred.

https://codereview.chromium.org/2526983002/ made it so that when the
next ResourceHandler defers OnResponseCompleted, and then the
ResourceHandler is detached, DetachableResourceHandler wouldn't tear
down the request.

This probably never happpens in practice, but it was supported before
that CL.

BUG= 687240 

Review-Url: https://codereview.chromium.org/2673453002
Cr-Commit-Position: refs/heads/master@{#447660}

[modify] https://crrev.com/59420d8f9e1521355d98b03cbf2748f7ad0dbb07/content/browser/loader/detachable_resource_handler.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3c1d10caa48428f7a185458ae7e9f70211d7c9fd

commit 3c1d10caa48428f7a185458ae7e9f70211d7c9fd
Author: mmenke <mmenke@chromium.org>
Date: Thu Mar 09 16:25:45 2017

Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let us simplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG= 687240 

Review-Url: https://codereview.chromium.org/2668603003
Cr-Commit-Position: refs/heads/master@{#455759}

[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/download/download_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/download/download_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/download/save_file_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/download/save_file_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/async_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/async_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/detachable_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/detachable_resource_handler.h
[add] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/detachable_resource_handler_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/intercepting_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/intercepting_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/intercepting_resource_handler_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/layered_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/layered_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mime_sniffing_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mime_sniffing_resource_handler_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mock_resource_loader.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/navigation_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/navigation_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/redirect_to_file_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/redirect_to_file_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/resource_loader.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/stream_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/stream_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/sync_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/sync_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/test_resource_handler.cc
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/browser/loader/test_resource_handler.h
[modify] https://crrev.com/3c1d10caa48428f7a185458ae7e9f70211d7c9fd/content/test/BUILD.gn

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ba831790eccdf8fa9178eef72b8479a12a2f8aa

commit 2ba831790eccdf8fa9178eef72b8479a12a2f8aa
Author: mmenke <mmenke@chromium.org>
Date: Mon Mar 20 16:48:08 2017

when a Mojo buffer can't be allocated synchronously.

The class still needs to create its own intermediary buffer
when the Mojo buffer is too small to meet the
MimeSniffingResourceHandler's needs, but once that's fixed,
the class can be made significantly simpler.

BUG= 687240 

Review-Url: https://codereview.chromium.org/2738973002
Cr-Commit-Position: refs/heads/master@{#458093}

[modify] https://crrev.com/2ba831790eccdf8fa9178eef72b8479a12a2f8aa/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/2ba831790eccdf8fa9178eef72b8479a12a2f8aa/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/2ba831790eccdf8fa9178eef72b8479a12a2f8aa/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/2ba831790eccdf8fa9178eef72b8479a12a2f8aa/content/browser/loader/mojo_async_resource_handler_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4fede0f13bb7bd62971e98b966548953b56059aa

commit 4fede0f13bb7bd62971e98b966548953b56059aa
Author: yhirano <yhirano@chromium.org>
Date: Tue Mar 21 03:37:45 2017

Revert of Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously (patchset #5 id:80001 of https://codereview.chromium.org/2738973002/ )

Reason for revert:
This CL leads to a layout test crash.
virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html

Original issue's description:
> when a Mojo buffer can't be allocated synchronously.
>
> The class still needs to create its own intermediary buffer
> when the Mojo buffer is too small to meet the
> MimeSniffingResourceHandler's needs, but once that's fixed,
> the class can be made significantly simpler.
>
> BUG= 687240 
>
> Review-Url: https://codereview.chromium.org/2738973002
> Cr-Commit-Position: refs/heads/master@{#458093}
> Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8479a12a2f8aa

TBR=rdsmith@chromium.org,mmenke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 687240 ,  703471 

Review-Url: https://codereview.chromium.org/2760223002
Cr-Commit-Position: refs/heads/master@{#458297}

[modify] https://crrev.com/4fede0f13bb7bd62971e98b966548953b56059aa/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/4fede0f13bb7bd62971e98b966548953b56059aa/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/4fede0f13bb7bd62971e98b966548953b56059aa/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/4fede0f13bb7bd62971e98b966548953b56059aa/content/browser/loader/mojo_async_resource_handler_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6add17253a0318bb3360c33de735f1f0ab3b8cd2

commit 6add17253a0318bb3360c33de735f1f0ab3b8cd2
Author: mmenke <mmenke@chromium.org>
Date: Tue Mar 21 21:31:17 2017

Reland of https://codereview.chromium.org/2738973002.

It failed to mark a request as unblocked before resuming it.
Unittests didn't catch this because they don't actually drive
a URLRequest, which is where the failing DCHECK was.

Reason for revert:
This CL leads to a layout test crash.
virtual/mojo-loading/http/tests/serviceworker/chromium/request-body-blob-crash.html

Original issue's description:
> Allow MojoAsyncResourceHandler::OnWillRead to complete asyncronously
> when a Mojo buffer can't be allocated synchronously.
>
> The class still needs to create its own intermediary buffer
> when the Mojo buffer is too small to meet the
> MimeSniffingResourceHandler's needs, but once that's fixed,
> the class can be made significantly simpler.
>
> BUG= 687240 
>
> Review-Url: https://codereview.chromium.org/2738973002
> Cr-Commit-Position: refs/heads/master@{#458093}
> Committed: https://chromium.googlesource.com/chromium/src/+/2ba831790eccdf8fa9178eef72b8479a12a2f8aa

TBR=rdsmith@chromium.org,mmenke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 687240 ,  703471 
Review-Url: https://codereview.chromium.org/2760223002
Cr-Commit-Position: refs/heads/master@{#458297}
Committed: https://chromium.googlesource.com/chromium/src/+/4fede0f13bb7bd62971e98b966548953b56059aa

Review-Url: https://codereview.chromium.org/2766733002
Cr-Commit-Position: refs/heads/master@{#458554}

[modify] https://crrev.com/6add17253a0318bb3360c33de735f1f0ab3b8cd2/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/6add17253a0318bb3360c33de735f1f0ab3b8cd2/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/6add17253a0318bb3360c33de735f1f0ab3b8cd2/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/6add17253a0318bb3360c33de735f1f0ab3b8cd2/content/browser/loader/mojo_async_resource_handler_unittest.cc

Comment 7 by mmenke@chromium.org, Mar 21 2017

Gah!  I didn't notice I copied the retry in the revert's description before it was too late.  :(

Comment 8 by mmenke@chromium.org, Mar 22 2017

Status: Fixed (was: Started)

Sign in to add a comment