Cancel the request when URLLoader is gone

Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

BUG=603396
Committed: https://crrev.com/5939370208c57d841e0b2cb3e12ae28d0d6cad1b
Cr-Commit-Position: refs/heads/master@{#432375}

[yhirano, 2016-11-01 05:59:59]

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

[commit-bot: I haz the power, 2016-11-01 06:00:16]

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

[yhirano, 2016-11-01 06:28:27]

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

[commit-bot: I haz the power, 2016-11-01 06:28:48]

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-11-01 07:08:38]

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

[commit-bot: I haz the power, 2016-11-01 07:08:39]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-01 07:12:02]

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

[commit-bot: I haz the power, 2016-11-01 07:12:14]

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

[yhirano, 2016-11-01 07:12:49]

Description was changed from

==========
Implement MojoAsyncResourceHandler::Cancel

This CL CL implements MojoAsyncResourceHandler::Cancel which overrides
content::mojom::URLLoader::Cancel.

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

to

==========
Implement MojoAsyncResourceHandler::Cancel

This CL implements MojoAsyncResourceHandler::Cancel which overrides
content::mojom::URLLoader::Cancel.

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

[yhirano, 2016-11-01 07:13:28]

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

[commit-bot: I haz the power, 2016-11-01 08:47:44]

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

[commit-bot: I haz the power, 2016-11-01 08:47:45]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-11-01 13:52:31]

mmenke@chromium.org changed reviewers:
+ csharrison@chromium.org, rdsmith@chromium.org

[mmenke, 2016-11-01 13:52:32]

[+rdsmith, +csharrison]:  Any thoughts on the cancellation question?  Callbacks
on cancellation have gotten us into a lot of issues, and the question seems a
bit related to our current work cleaning up the borked-by-design ResourceLoader
cancellation flow.

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2439: // CancelRequest
methods in this file, which also tear down the loader.
Existing bug, but class-level comments should go with method declaration, not
the definition.

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2440: void
ResourceDispatcherHostImpl::CancelRequestFromRenderer(
Why the new method?

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
Hrm...I wonder...Do we want to promise an OnComplete call on cancellation?  We
do currently send messages in this case, but ResourceDispatcher has already
cleaned up the request by then, anyways.

[yhirano, 2016-11-01 14:24:25]

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2440: void
ResourceDispatcherHostImpl::CancelRequestFromRenderer(
On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> Why the new method?

OnCancelRequest expects that |filter_| is set, but we can't do that from outside
of this class.

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> Hrm...I wonder...Do we want to promise an OnComplete call on cancellation?  We
> do currently send messages in this case, but ResourceDispatcher has already
> cleaned up the request by then, anyways.

Yeah, I'm not particularly interested in receiving a OnComplete method, but it's
the simplest way to detect that the loading is aborted as far as I know.

[mmenke, 2016-11-01 14:27:54]

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
On 2016/11/01 14:24:25, yhirano wrote:
> On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> > Hrm...I wonder...Do we want to promise an OnComplete call on cancellation? 
We
> > do currently send messages in this case, but ResourceDispatcher has already
> > cleaned up the request by then, anyways.
> 
> Yeah, I'm not particularly interested in receiving a OnComplete method, but
it's
> the simplest way to detect that the loading is aborted as far as I know.

I'm thinking it may be simplest to tear down the loader in this case, excluding
the usual download/detachable cases.

One other question:  Is there some way to have mojo automatically trigger a
cancel if the URLLoader is torn town?

[mmenke, 2016-11-01 14:30:05]

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
On 2016/11/01 14:27:54, mmenke (Busy Nov 2-3) wrote:
> On 2016/11/01 14:24:25, yhirano wrote:
> > On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> > > Hrm...I wonder...Do we want to promise an OnComplete call on cancellation?

> We
> > > do currently send messages in this case, but ResourceDispatcher has
already
> > > cleaned up the request by then, anyways.
> > 
> > Yeah, I'm not particularly interested in receiving a OnComplete method, but
> it's
> > the simplest way to detect that the loading is aborted as far as I know.
> 
> I'm thinking it may be simplest to tear down the loader in this case,
excluding
> the usual download/detachable cases.
> 
> One other question:  Is there some way to have mojo automatically trigger a
> cancel if the URLLoader is torn town?

Also, if we're not going to promise an OnComplete call (And I'd rather not),
then I'd like to ensure we don't call it from the start.

[yhirano, 2016-11-02 04:58:09]

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

[commit-bot: I haz the power, 2016-11-02 04:58:22]

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-11-02 05:46:43]

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

[commit-bot: I haz the power, 2016-11-02 05:46:44]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-02 06:02:56]

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2439: // CancelRequest
methods in this file, which also tear down the loader.
On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> Existing bug, but class-level comments should go with method declaration, not
> the definition.

Done.

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
On 2016/11/01 14:30:05, mmenke (Busy Nov 2-3) wrote:
> On 2016/11/01 14:27:54, mmenke (Busy Nov 2-3) wrote:
> > On 2016/11/01 14:24:25, yhirano wrote:
> > > On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> > > > Hrm...I wonder...Do we want to promise an OnComplete call on
cancellation?
> 
> > We
> > > > do currently send messages in this case, but ResourceDispatcher has
> already
> > > > cleaned up the request by then, anyways.
> > > 
> > > Yeah, I'm not particularly interested in receiving a OnComplete method,
but
> > it's
> > > the simplest way to detect that the loading is aborted as far as I know.
> > 
> > I'm thinking it may be simplest to tear down the loader in this case,
> excluding
> > the usual download/detachable cases.
> > 
> > One other question:  Is there some way to have mojo automatically trigger a
> > cancel if the URLLoader is torn town?
> 
> Also, if we're not going to promise an OnComplete call (And I'd rather not),
> then I'd like to ensure we don't call it from the start.

Done.

https://codereview.chromium.org/2466843002/diff/40001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:287:
client.RunUntilComplete();
On 2016/11/01 14:27:54, mmenke (Busy Nov 2-3) wrote:
> On 2016/11/01 14:24:25, yhirano wrote:
> > On 2016/11/01 13:52:32, mmenke (Busy Nov 2-3) wrote:
> > > Hrm...I wonder...Do we want to promise an OnComplete call on cancellation?

> We
> > > do currently send messages in this case, but ResourceDispatcher has
already
> > > cleaned up the request by then, anyways.
> > 
> > Yeah, I'm not particularly interested in receiving a OnComplete method, but
> it's
> > the simplest way to detect that the loading is aborted as far as I know.
> 
> I'm thinking it may be simplest to tear down the loader in this case,
excluding
> the usual download/detachable cases.
> 
> One other question:  Is there some way to have mojo automatically trigger a
> cancel if the URLLoader is torn town?

Hm, I didn't think about that. PS4 removes Cancel from the mojom file. What do
you think?

[mmenke, 2016-11-02 14:21:06]

I like this approach.  May not have time to review this today.

[mmenke, 2016-11-03 18:17:41]

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

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:116: // after |this|
destruction.
nit:  "after |this| is destroyed."

Could optionally mention that this is because it owned binding_, but think we're
fine without it.

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2446:
GetLoader(request_id.child_id, request_id.request_id);
nit:  GetLoader(request_id) (There's an overload that takes the GlobalRequestID,
instead of the separate IDs)

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:284:
ASSERT_FALSE(client.has_received_response());
What guarantee is there that we don't receive the response during the
RunUntilIdle call?  Should we instead use URLRequestFailedJob::AddUrlHandler()
and then URLRequestFailedJob::GetMockHttpUrl(ERR_IO_PENDING)?  That will create
a request that hangs once started.

https://codereview.chromium.org/2466843002/diff/60001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2466843002/diff/60001/content/common/url_load...
content/common/url_loader.mojom:16: interface URLLoader {
Should we document that deleting the URLLoader cancels the request?

[yhirano, 2016-11-04 07:31:18]

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

[commit-bot: I haz the power, 2016-11-04 07:31:33]

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-11-04 08:45:10]

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

[commit-bot: I haz the power, 2016-11-04 08:45:11]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-11-04 19:59:39]

On 2016/11/01 13:52:32, mmenke wrote:
> [+rdsmith, +csharrison]:  Any thoughts on the cancellation question? 
Callbacks
> on cancellation have gotten us into a lot of issues, and the question seems a
> bit related to our current work cleaning up the borked-by-design
ResourceLoader
> cancellation flow.

Matt: Sorry to take so long to get back to you on this, and sorrier to be in a
position where I'm asking questions instead of making useful suggestions, but:
Why does cancellation flow need to go through a new method on RDHI rather than
through the ResourceController of the ResourceHandler?  Since that's where we're
aiming to have cancellation happen in the future.

The cancellation callback (as used in the latest PS, at least) seems to me to be
mojo's problem, since it's just how cancellation is getting wired from the Mojo
loader to the resource handler.  But I don't yet understand the mojo RH/loader
architecture.

[mmenke, 2016-11-04 20:06:12]

On 2016/11/04 19:59:39, Randy Smith - Not in Mondays wrote:
> On 2016/11/01 13:52:32, mmenke wrote:
> > [+rdsmith, +csharrison]:  Any thoughts on the cancellation question? 
> Callbacks
> > on cancellation have gotten us into a lot of issues, and the question seems
a
> > bit related to our current work cleaning up the borked-by-design
> ResourceLoader
> > cancellation flow.
> 
> Matt: Sorry to take so long to get back to you on this, and sorrier to be in a
> position where I'm asking questions instead of making useful suggestions, but:
> Why does cancellation flow need to go through a new method on RDHI rather than
> through the ResourceController of the ResourceHandler?  Since that's where
we're
> aiming to have cancellation happen in the future.
> 
> The cancellation callback (as used in the latest PS, at least) seems to me to
be
> mojo's problem, since it's just how cancellation is getting wired from the
Mojo
> loader to the resource handler.  But I don't yet understand the mojo RH/loader
> architecture.

In this case, there's some extra magic behavior:  We ignore cancels from the
renderer for downloads, transferrable navigations, and detachable requests.  I
don't think we want to expose that cancellation knob to chrome/.

The cancellation callback is a question of the mojo interface we want to expose,
this is not a general mojo thing.  The question was if we wanted to invoke the
normal cancel/failure/complete callback if we were cancelled through the mojo
interface or not.  URLRequest does this, and it has proven to be a problem (Even
if some consumers really want it), so we've long talked about remove it.  Seems
like we should use that lesson here.

The current patch set just invokes cancel when the mojo interface is destroyed,
which works nicely, and makes it not matter if we try and invoke the mojo
callback with ERR_ABORTED or not, in that case.

[Charlie Harrison, 2016-11-04 20:35:37]

On 2016/11/04 20:06:12, mmenke wrote:
> On 2016/11/04 19:59:39, Randy Smith - Not in Mondays wrote:
> > On 2016/11/01 13:52:32, mmenke wrote:
> > > [+rdsmith, +csharrison]:  Any thoughts on the cancellation question? 
> > Callbacks
> > > on cancellation have gotten us into a lot of issues, and the question
seems
> a
> > > bit related to our current work cleaning up the borked-by-design
> > ResourceLoader
> > > cancellation flow.
> > 
> > Matt: Sorry to take so long to get back to you on this, and sorrier to be in
a
> > position where I'm asking questions instead of making useful suggestions,
but:
> > Why does cancellation flow need to go through a new method on RDHI rather
than
> > through the ResourceController of the ResourceHandler?  Since that's where
> we're
> > aiming to have cancellation happen in the future.
> > 
> > The cancellation callback (as used in the latest PS, at least) seems to me
to
> be
> > mojo's problem, since it's just how cancellation is getting wired from the
> Mojo
> > loader to the resource handler.  But I don't yet understand the mojo
RH/loader
> > architecture.
> 
> In this case, there's some extra magic behavior:  We ignore cancels from the
> renderer for downloads, transferrable navigations, and detachable requests.  I
> don't think we want to expose that cancellation knob to chrome/.
> 
> The cancellation callback is a question of the mojo interface we want to
expose,
> this is not a general mojo thing.  The question was if we wanted to invoke the
> normal cancel/failure/complete callback if we were cancelled through the mojo
> interface or not.  URLRequest does this, and it has proven to be a problem
(Even
> if some consumers really want it), so we've long talked about remove it. 
Seems
> like we should use that lesson here.
> 
> The current patch set just invokes cancel when the mojo interface is
destroyed,
> which works nicely, and makes it not matter if we try and invoke the mojo
> callback with ERR_ABORTED or not, in that case.

Sorry I've been a bit MIA on this one too. I think cancel on mojo interface
destruction is definitely the preferred API. If the interface sticks around,
we'll have to service weird IPCs on already-canceled requests.

[yhirano, 2016-11-08 05:54:18]

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

[commit-bot: I haz the power, 2016-11-08 05:54:35]

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

[yhirano, 2016-11-08 06:00:07]

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

[commit-bot: I haz the power, 2016-11-08 06:00:22]

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-11-08 06:30:19]

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

[commit-bot: I haz the power, 2016-11-08 06:30:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[yhirano, 2016-11-08 10:24:37]

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

[commit-bot: I haz the power, 2016-11-08 10:24:51]

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-11-08 10:27:36]

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

[commit-bot: I haz the power, 2016-11-08 10:27:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-11-08 10:29:21]

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

[commit-bot: I haz the power, 2016-11-08 10:29:36]

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-11-08 10:31:54]

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

[commit-bot: I haz the power, 2016-11-08 10:31:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yhirano, 2016-11-08 10:32:13]

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

[commit-bot: I haz the power, 2016-11-08 10:32:33]

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-11-08 11:33:00]

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

[commit-bot: I haz the power, 2016-11-08 11:33:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-11-08 11:50:36]

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

[commit-bot: I haz the power, 2016-11-08 11:50:55]

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

[yhirano, 2016-11-08 11:52:23]

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

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:116: // after |this|
destruction.
On 2016/11/03 18:17:41, mmenke wrote:
> nit:  "after |this| is destroyed."
> 
> Could optionally mention that this is because it owned binding_, but think
we're
> fine without it.

Done.

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:2446:
GetLoader(request_id.child_id, request_id.request_id);
On 2016/11/03 18:17:41, mmenke wrote:
> nit:  GetLoader(request_id) (There's an overload that takes the
GlobalRequestID,
> instead of the separate IDs)

Done.

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
File content/browser/loader/url_loader_factory_impl_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/60001/content/browser/loader/...
content/browser/loader/url_loader_factory_impl_unittest.cc:284:
ASSERT_FALSE(client.has_received_response());
On 2016/11/03 18:17:41, mmenke wrote:
> What guarantee is there that we don't receive the response during the
> RunUntilIdle call?  Should we instead use URLRequestFailedJob::AddUrlHandler()
> and then URLRequestFailedJob::GetMockHttpUrl(ERR_IO_PENDING)?  That will
create
> a request that hangs once started.

Done.

https://codereview.chromium.org/2466843002/diff/60001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2466843002/diff/60001/content/common/url_load...
content/common/url_loader.mojom:16: interface URLLoader {
On 2016/11/03 18:17:41, mmenke wrote:
> Should we document that deleting the URLLoader cancels the request?

Done.

[mmenke, 2016-11-08 15:14:21]

https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:267: class
TestURLLoaderFactory final : public mojom::URLLoaderFactory {
Could you do this in a separate CL?  I'm not familiar with this code, and this
change doesn't seem to belond with the rest.  Changes here also don't seem to be
trivial.

[mmenke, 2016-11-08 15:15:37]

On 2016/11/08 15:14:21, mmenke wrote:
>
https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
> File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):
> 
>
https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
> content/browser/loader/mojo_async_resource_handler_unittest.cc:267: class
> TestURLLoaderFactory final : public mojom::URLLoaderFactory {
> Could you do this in a separate CL?  I'm not familiar with this code, and this
> change doesn't seem to belond with the rest.  Changes here also don't seem to
be
> trivial.

And this LGTM, assuming the TestURLLoaderFactory change/move is reverted.

[commit-bot: I haz the power, 2016-11-08 15:42:09]

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

[commit-bot: I haz the power, 2016-11-08 15:42:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yhirano, 2016-11-09 01:49:27]

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

[commit-bot: I haz the power, 2016-11-09 01:50:05]

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

[yhirano, 2016-11-09 03:38:07]

https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2466843002/diff/200001/content/browser/loader...
content/browser/loader/mojo_async_resource_handler_unittest.cc:267: class
TestURLLoaderFactory final : public mojom::URLLoaderFactory {
On 2016/11/08 15:14:21, mmenke wrote:
> Could you do this in a separate CL?  I'm not familiar with this code, and this
> change doesn't seem to belond with the rest.  Changes here also don't seem to
be
> trivial.

Done: https://codereview.chromium.org/2489793002/

[yhirano, 2016-11-09 03:42:23]

Description was changed from

==========
Implement MojoAsyncResourceHandler::Cancel

This CL implements MojoAsyncResourceHandler::Cancel which overrides
content::mojom::URLLoader::Cancel.

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

to

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

[yhirano, 2016-11-09 03:42:36]

Description was changed from

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

to

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

[yhirano, 2016-11-09 03:45:39]

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

[yhirano, 2016-11-09 03:45:40]

+horo@ for content/browser/service_worker.
+dcheng@ for content/common/url_loader.mojom.

[commit-bot: I haz the power, 2016-11-09 04:59:44]

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

[commit-bot: I haz the power, 2016-11-09 04:59:45]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-09 05:29:21]

On 2016/11/09 03:45:40, yhirano wrote:
> +horo@ for content/browser/service_worker.
> +dcheng@ for content/common/url_loader.mojom.

lgtm.

This change may make ServiceWorkerNavigationPreloadTest.NetworkFallback browser
test flaky.
So I created a cl to fix the test.
https://codereview.chromium.org/2482263004/

[yhirano, 2016-11-09 05:31:05]

On 2016/11/09 05:29:21, horo wrote:
> On 2016/11/09 03:45:40, yhirano wrote:
> > +horo@ for content/browser/service_worker.
> > +dcheng@ for content/common/url_loader.mojom.
> 
> lgtm.
> 
> This change may make ServiceWorkerNavigationPreloadTest.NetworkFallback
browser
> test flaky.
> So I created a cl to fix the test.
> https://codereview.chromium.org/2482263004/

Thank you!

[dcheng, 2016-11-09 06:34:25]

mojo lgtm

[yhirano, 2016-11-09 06:40:42]

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

[commit-bot: I haz the power, 2016-11-09 06:40:52]

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

[yhirano, 2016-11-09 06:41:24]

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

[commit-bot: I haz the power, 2016-11-09 06:41:30]

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

[yhirano, 2016-11-09 06:57:57]

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

[commit-bot: I haz the power, 2016-11-09 06:58:05]

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

[yhirano, 2016-11-09 07:48:29]

Horo-san: Sorry for bothering you again, it turns out we need one more error
handler to make SW-browsertest pass. Are you OK with PS15[1]?

https://codereview.chromium.org/2466843002/diff2/220001:220002/content/browse...

[commit-bot: I haz the power, 2016-11-09 08:20:37]

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

[commit-bot: I haz the power, 2016-11-09 08:20:38]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-09 10:02:08]

https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:60: void
Cancel() { loader_ = nullptr; }
Please add comments.

  // Called when the mojom::URLLoaderPtr in the service worker is deleted.
  void Cancel() {
    // Clears |loader_| to call MojoAsyncResourceHandler::Cancel() and cancel
    // the request if it is still ongoing.
    loader_ = nullptr;
  }

[yhirano, 2016-11-11 02:27:05]

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

[commit-bot: I haz the power, 2016-11-11 02:27:31]

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-11-11 04:08:39]

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

[commit-bot: I haz the power, 2016-11-11 04:08:40]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-11 04:22:22]

https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:60: void
Cancel() { loader_ = nullptr; }
On 2016/11/09 10:02:08, horo wrote:
> Please add comments.
> 
>   // Called when the mojom::URLLoaderPtr in the service worker is deleted.
>   void Cancel() {
>     // Clears |loader_| to call MojoAsyncResourceHandler::Cancel() and cancel
>     // the request if it is still ongoing.
>     loader_ = nullptr;
>   }

Done.

[horo, 2016-11-11 04:46:12]

On 2016/11/11 04:22:22, yhirano wrote:
>
https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
> File content/browser/service_worker/service_worker_fetch_dispatcher.cc
(right):
> 
>
https://codereview.chromium.org/2466843002/diff/220002/content/browser/servic...
> content/browser/service_worker/service_worker_fetch_dispatcher.cc:60: void
> Cancel() { loader_ = nullptr; }
> On 2016/11/09 10:02:08, horo wrote:
> > Please add comments.
> > 
> >   // Called when the mojom::URLLoaderPtr in the service worker is deleted.
> >   void Cancel() {
> >     // Clears |loader_| to call MojoAsyncResourceHandler::Cancel() and
cancel
> >     // the request if it is still ongoing.
> >     loader_ = nullptr;
> >   }
> 
> Done.

lgtm

[yhirano, 2016-11-12 09:11:35]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-12 09:11:36]

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

[commit-bot: I haz the power, 2016-11-12 09:11:40]

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-11-12 09:18:30]

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

[commit-bot: I haz the power, 2016-11-12 09:18:32]

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...)

[yhirano, 2016-11-14 01:37:21]

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

[yhirano, 2016-11-14 01:37:22]

+avi@ for OWNER review.

[Avi (use Gerrit), 2016-11-14 01:52:33]

avi@chromium.org changed reviewers:
+ scottmg@chromium.org

[Avi (use Gerrit), 2016-11-14 01:52:34]

This is a DEPS question. John's out; Scott, is this DEPS change OK?

[scottmg, 2016-11-14 18:08:34]

On 2016/11/14 01:52:34, Avi wrote:
> This is a DEPS question. John's out; Scott, is this DEPS change OK?

content/browser/loader/DEPS lgtm

[yhirano, 2016-11-16 02:16:25]

avi@: friendly ping

[kinuko, 2016-11-16 03:00:33]

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

[kinuko, 2016-11-16 03:00:34]

rs/lgtm in case it helps

https://codereview.chromium.org/2466843002/diff/290001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2466843002/diff/290001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:347: // methods in this
file, which also tear down the loader.
nit: might be good to also note that this is called when mojo connection is
dropped

[yhirano, 2016-11-16 03:11:31]

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

[commit-bot: I haz the power, 2016-11-16 03:12:10]

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

[yhirano, 2016-11-16 03:13:10]

Thank you.

https://codereview.chromium.org/2466843002/diff/290001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/2466843002/diff/290001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:347: // methods in this
file, which also tear down the loader.
On 2016/11/16 03:00:34, kinuko wrote:
> nit: might be good to also note that this is called when mojo connection is
> dropped

Done.

[yhirano, 2016-11-16 03:13:18]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2016-11-16 03:13:19]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-16 03:13:19]

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

[commit-bot: I haz the power, 2016-11-16 03:13:53]

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

[Avi (use Gerrit), 2016-11-16 03:57:37]

Apologies. LGTM

[commit-bot: I haz the power, 2016-11-16 04:10:11]

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

[commit-bot: I haz the power, 2016-11-16 04:10:13]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yhirano, 2016-11-16 04:29:40]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-11-16 04:30:18]

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-11-16 06:11:20]

Description was changed from

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

to

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

[commit-bot: I haz the power, 2016-11-16 06:11:22]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-11-16 06:12:58]

Description was changed from

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

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

to

==========
Cancel the request when URLLoader is gone

With this CL, resource loading will be canceled when
mojom::URLLoaderAssociatedPtr is destroyed.

BUG=603396

Committed: https://crrev.com/5939370208c57d841e0b2cb3e12ae28d0d6cad1b
Cr-Commit-Position: refs/heads/master@{#432375}
==========

[commit-bot: I haz the power, 2016-11-16 06:13:00]

Patchset 17 (id:??) landed as
https://crrev.com/5939370208c57d841e0b2cb3e12ae28d0d6cad1b
Cr-Commit-Position: refs/heads/master@{#432375}