To elaborate (Copied from  issue 659319 ):

What I'm thinking of is making a new ResourceHandler method:  void ResourceHandler::OnCancel(net_error, defer), which for all ResourceHandlers just calls info ResourceHandler::OnResponseCompleted (Or whatever it's called).  Once we have the paths separated, then we can modify why RedirectToFileResourceHandler does on cancellation, and remove the defer parameter.

One issue here:  If we allow ResourceController::Cancel to be invoked synchronously, we either need to modify all LayeredResourceHandlers to support being synchronously deleted when calling into the next ResourceHandler in the chain, or we post a task asynchronously.  The latter approach means after something "cancels" a request, we could continue to drive the request forward.  So I think we may need to keep the "Return a value immediately for sync cancellation" logic.

Actually, how about we make cancellation always go through ResourceController::Cancel[Blah], but require it be done asynchronously, and the request be deferred in the meantime?  That does require anything that cancels to know to defer the request, but it means we only have one cancellation path.  If there's an out of band cancellation, it does mean the request will continue until cancellation occurs, unfortunately, but I think that's OK?  We could also add a URLRequest::Abort() call while the call is pending.

It does mean an extra task run in the cancellation case, but I don't think we care?  Anyhow, still playing with it.

I have a moderately strong bias (that I want to be careful about questioning) in favor of sync cancelling, just because it removes a large source of race conditions, and to the extent we can bind cancellation and deletion, simplifies potential lifetime issues (because you can always tear an object down).  So I'd like to make the cost of sticking with that is higher than the (admittedly abstract, but I'll still claim real) gain.

Is the concern the (sadly standard) one about the confusion of calling a routine that may result in deleting self?  Or is there complexity I'm missing?

I think we could probably make the async cancel path work, but I'd want to really nail down deferral semantics (more than the current API allows, AIUI).  And I'd be sorry to give up the race protection and lifetime simplification.
 

Yea, the problem with sync cancellation is we suddenly have to make the entire ResourceHandler stack capable of sync deletion.  Or we have to keep the two delete paths (Could do that and make everything return a net::Error, just like in net, I guess).  Another option would be to make ResourceController::Cancel post a task itself (I'm not concerned about perf there), but then we'd have to replace defer with defer_or_cancel everywhere.

Note that async cancel does protect against (most) races, as long as we treat it as defer.  We could do other stuff before we get the cancel message, but we'll only be processing a single cancel message, and since AsyncResourceHandler can cancel at any time, anyways, I think we're (mostly) safe?

Note that I'm not yet set on any approach here, I've gone through a number of different possibilities, thinking each was the way to go, in turn, and only one has proven basically unworkable (Making the ResourceController have a Pause() method makes things too complicated).

Capturing an offline discussion: Matt and I talked, and we decided that (given that we weren't currently aiming for the ideal world, which would be a pull based stream with AsyncResourceHandler or equivalent at the top owning everything) that the least risky change would be async cancel + defer.  Specifically, cancellation from inside the ResourceHandler/ResourceThrottle graph would defer the request and call ResourceController::Cancel* (or return synchronously, but I think our last decision was RC::C) and that RC::C would post a task to get the cancellation out of the call stack that initiated it.  

Matt, just to clarify: My understanding is that this doesn't change the aim to make the cancellation that the task from RC::C initiates synchronous, meaning we still want to separate out cancellation and error/completion paths, and we still want to make cancellation on RedirectToFileHandler synchronous, at least as far as anything outside that class knows.  That match your vision?

Correct - on cancellation through the ResourceController, cancellation would post a task (And the request must be deferred), but once that task is executed, teardown would happen synchronously, and we'd need a new callback for that.

There are unresolved issues - if we're cancelled in the middle of completion with an error code, which error do we pass down, for instance.

I'm working on making all cancellation go asynchronously through ResourceLoader::Cancel.  Once we have that wired up, next step is to split ResourceHandler completion and cancellation paths.

And, for the record, that's a needed first step because ResourceLoader::Cancel currently cancels request by going through the URLRequest, which implicitly posts a task, if the request has already started (Otherwise, it just also posts a task, IIRC, to avoid re-entrant teardown).

This plan sounds good to me. This bypasses the double cancel case because we force defer on cancellation?

It bypasses double cancellation because while two objects could cancel (One when it's not its turn), one cancellation task would be executed first, and when it is, we delete everything synchronously, so the second cancellation message would never be handled.  There is the problem that this can happen in the middle of OnResponseCompleted (Either on success, or on error), so ResourceHandlers would need to be careful about that, but that should hopefully be the only issue (Assuming classes correctly "defer" whenever they want to cancel the request)

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

commit 5159f105dbf404212d7d8e1b24539be049f67c5d
Author: mmenke <mmenke@chromium.org>
Date: Thu Jan 12 16:53:22 2017

Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG= 659317 

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

[modify] https://crrev.com/5159f105dbf404212d7d8e1b24539be049f67c5d/content/browser/loader/DEPS
[modify] https://crrev.com/5159f105dbf404212d7d8e1b24539be049f67c5d/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/5159f105dbf404212d7d8e1b24539be049f67c5d/content/browser/loader/mock_resource_loader.h
[modify] https://crrev.com/5159f105dbf404212d7d8e1b24539be049f67c5d/content/browser/loader/mojo_async_resource_handler_unittest.cc

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

commit 2522425b429d16cfdc97f11e4bbb4ac28d1a20fd
Author: mmenke <mmenke@chromium.org>
Date: Thu Jan 12 21:18:56 2017

Update MimeSniffingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests. Also add
checking of the error code the request is canceled with.

Also prevent next ResourceHandler in the chain from being called
re-entrantly

BUG= 659317 

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

[modify] https://crrev.com/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd/content/browser/loader/mime_sniffing_resource_handler_unittest.cc
[modify] https://crrev.com/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd/content/browser/loader/resource_controller.h
[modify] https://crrev.com/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd/content/browser/loader/test_resource_handler.cc

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

commit b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2
Author: mmenke <mmenke@chromium.org>
Date: Wed Jan 18 18:56:18 2017

Update InterceptingResourceHandler tests to use MockResourceLoader.

This will make it easier to land an upcoming ResourceHandler refactor,
and also gives us some more ASSERTs shared with other tests.

Also prevent next ResourceHandler in the chain from being called
re-entrantly, and do a couple other test cleanups (Removing duplicated
code, particularly), and move a lot of redundant initialization into
the test fixture.

BUG= 659317 

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

[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/intercepting_resource_handler.cc
[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/intercepting_resource_handler.h
[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/intercepting_resource_handler_unittest.cc
[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/test_resource_handler.cc
[modify] https://crrev.com/b38c6a55bbbaa4d7fa714f1ceb8cb0099c0d3ae2/content/browser/loader/test_resource_handler.h

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

commit 9874e16e6ccd443a33d4207619e87c6fc6ad1dcd
Author: mmenke <mmenke@chromium.org>
Date: Thu Jan 26 22:37:20 2017

Add unit tests for RedirectToFileResourceHandler.

It had some integration tests, but no real unit tests.  Also fix its
behavior when a write synchronously succeeds, which would cause it to
resume a request without first pausing it.  The API for writing to files
allows sync completion, though it currently never happens, so was not a
visible bug.

BUG= 679483 , 659317 

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

[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler.h
[add] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/redirect_to_file_resource_handler_unittest.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/browser/loader/test_resource_handler.cc
[modify] https://crrev.com/9874e16e6ccd443a33d4207619e87c6fc6ad1dcd/content/test/BUILD.gn

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

commit 87f5c77aaa3541bee2f8d3b6b64d3879b8e81957
Author: mmenke <mmenke@chromium.org>
Date: Tue Jan 31 16:11:26 2017

Refactor ResourceHandler API.

Previously, any ResourceHandler could resume or cancel a request at any
time, including when it was another ResourceHandler's turn to deal with
a request.  This could resume in unexpected Resumes from buggy
ResourceHandlers, or unexpected or even multiple cancels at weird times.

This CL makes it so that an owned ResourceController is passed through
the ResourceHandler chain, and only the request that owns it can use it
to resume or cancel a request.  It also introduces a new API for
cancelling requests when a ResourceHandler does not have ownership of a
ResourceHandler.

In a future API, that alternative API will result in immediate
destruction of the ResourceHandler chain, to avoid multiple messages
wending their way through the chain simultaneously.

BUG= 659317 

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

[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/BUILD.gn
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/download/download_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/download/download_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/download/save_file_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/download/save_file_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/DEPS
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/async_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/async_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/detachable_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/detachable_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/intercepting_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/intercepting_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/layered_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/layered_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mime_sniffing_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mock_resource_loader.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mock_resource_loader.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mojo_async_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mojo_async_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/mojo_async_resource_handler_unittest.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/navigation_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/navigation_resource_handler.h
[add] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/null_resource_controller.cc
[add] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/null_resource_controller.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/redirect_to_file_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/redirect_to_file_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/redirect_to_file_resource_handler_unittest.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_controller.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_dispatcher_host_unittest.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_loader.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_loader.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/resource_loader_unittest.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/stream_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/stream_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/stream_writer.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/stream_writer.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/sync_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/sync_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/test_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/test_resource_handler.h
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/throttling_resource_handler.cc
[modify] https://crrev.com/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957/content/browser/loader/throttling_resource_handler.h