Refactor ResourceHandler API.

Refactor ResourceHandler API.

ResourceHandlers used to have one way to cancel requests synchronously,
and another to do so asynchronously.  Now there's only one way to do it:
Call into the ResourceController.  The downside to this is that when
cancelling the request, ResourceHandlers also need to tell the upstream
object that the request is "deferred".

This is part of a refactoring to make cancellation synchronous.

BUG=too lazy to look it up right now

[mmenke, 2016-11-07 22:51:36]

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

[commit-bot: I haz the power, 2016-11-07 22:52:07]

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-07 23:10:57]

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

[commit-bot: I haz the power, 2016-11-07 23:10:58]

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

[mmenke, 2016-11-08 15:31:03]

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

[commit-bot: I haz the power, 2016-11-08 15:31:24]

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 15:58:12]

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

[commit-bot: I haz the power, 2016-11-08 15:58:13]

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

[mmenke, 2016-11-15 21:02:01]

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

[commit-bot: I haz the power, 2016-11-15 21:02:18]

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-15 21:21:08]

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

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

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

[mmenke, 2016-11-16 18:32:44]

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

[commit-bot: I haz the power, 2016-11-16 18:33:27]

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-16 18:46:23]

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

[commit-bot: I haz the power, 2016-11-16 18:46:24]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-11-16 19:19:48]

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

[commit-bot: I haz the power, 2016-11-16 19:20:40]

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

[mmenke, 2016-11-16 21:19:10]

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

[commit-bot: I haz the power, 2016-11-16 21:19:42]

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-16 21:46:02]

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

[commit-bot: I haz the power, 2016-11-16 21:46:02]

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

[mmenke, 2016-11-16 21:50:55]

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

[commit-bot: I haz the power, 2016-11-16 21:51:55]

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-16 22:39:50]

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

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

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

[mmenke, 2016-11-17 19:54:23]

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

[commit-bot: I haz the power, 2016-11-17 19:55:31]

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

[mmenke, 2016-11-17 19:59:03]

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

[mmenke, 2016-11-17 19:59:04]

[rdsmith]:  WDYT?  This is kinda a huge CL, and I should probably beef up tests
a bit for DetachableResourceHandler, at least, since cancellation is a bit more
magical there.

I think we should also switch OnWillRead to use the same form of the other
methods (Mojo will benefit from it, also makes the code more consistent), but
not in this CL, due to how big this already is.  Not sure if OnRequestComplete
should gain the ability to cancel - I guess maybe?  External callers may want to
be able to cancel at that point.

Another issue is that making cancellation async breaks at least
NavigationResourceHandler, which expects never to get a callback after it
cancels the request.  I'm a bit concerned about this - we could just add a
method to abort the request, and since things currently mostly work, they'd
still mostly work, but there's nothing guaranteeing no other class is off doing
async work that it will resume before handling the cancel.  We can fix
NavigationResourceHandler pretty easily instead/as well, just wonder if there
are other cases of classes that have those expectations.

[commit-bot: I haz the power, 2016-11-18 00:06:25]

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

[commit-bot: I haz the power, 2016-11-18 00:06:26]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2016-11-18 21:38:42]

Questions/thoughts in no particular order:
* I just skimmed this since I think we're still at a high-level/architectural
review stage--eventually I'll want to dig into each RH and understand its
special gotchas, but I haven't done that yet.

* If you wanted to do this CL in pieces (not requesting; I bias a bit towards
keeping related changes together, which this CL does) would it work to change
each RH in turn to call RC::Cancel() and defer the request, but leave the
function signatures alone?

* I find myself wincing at defer & cancel sharing a boolean signal.  The
documentation for that bool is convoluted (set if deferred for a later
Resume/Cancel, or if RC::Cancel called synchronously) which both feels like it's
harder for consumers to use properly, and that we don't have an ideal separation
of concepts.  Having said that, I don't see what to do about it; we really do
need the request to be deferred (though maybe there's a doc rephrase; I don't
know).  

Random thought: Would it be possible to change the contract for traveling down
the ResourceHandler chain s.t. the base class always handled the next_handler_
and calls to that handler went through the base class?  And (key point) we
cancelled through that base class as well?  Then that base class could
auto-defer and the RH subclass wouldn't have to worry about it.  As I say,
random thought--there may well be things wrong with it.

* I want to understand the intended usage in the async (defer) cases a bit
better.  I (naively?) imagine that only one ResourceHandler can have a request
deferred at any given time.  That RH is doing something async, and is promising
to get back to the RC with a Resume() (or Cancel()) at a later time, and all
processing in the RH stack is on hold until that occurs.  What parts of this
naive view are incorrect?  If I've got it right, under what circumstances would
we expect the a RH to get a callback after it's set the defer bit but before
*it's* called Resume() (the concern you raise in your last paragraph, IIUC).

* Could you say a bit more about your model WRT OnRequestComplete(); what kinds
of external callers might want to be able to cancel at that point?  (I don't
care a lot--I'm strongly in favor of API consistency, so I do want to extend the
pattern to OnRequestComplete/OnWillRead--but I do care about understanding the
use case space, even if it's currently imaginary).

[mmenke, 2016-11-18 22:27:23]

Worth noting I'm in no rush here, rather get things right, even if it takes a
number of significant alterations / a lot of back and forth.

On 2016/11/18 21:38:42, Randy Smith - Not in Mondays wrote:
> Questions/thoughts in no particular order:
> * I just skimmed this since I think we're still at a high-level/architectural
> review stage--eventually I'll want to dig into each RH and understand its
> special gotchas, but I haven't done that yet.
> 
> * If you wanted to do this CL in pieces (not requesting; I bias a bit towards
> keeping related changes together, which this CL does) would it work to change
> each RH in turn to call RC::Cancel() and defer the request, but leave the
> function signatures alone?

I think this should work, since the existing code already handles sync Cancel +
returning true + setting defer basically correctly.

> * I find myself wincing at defer & cancel sharing a boolean signal.  The
> documentation for that bool is convoluted (set if deferred for a later
> Resume/Cancel, or if RC::Cancel called synchronously) which both feels like
it's
> harder for consumers to use properly, and that we don't have an ideal
separation
> of concepts.  Having said that, I don't see what to do about it; we really do
> need the request to be deferred (though maybe there's a doc rephrase; I don't
> know).  

I agree with this.  The alternatives aren't great.  We could separate out the
signals, make the cancellation state of the ResourceController observable
(Optionally adding deferral state there, too, though that gets ugly), or make
destruction synchronous.  While I really like the idea of making teardown
synchronous, in theory, it will make for a much more complicated CL, and will
likely break a lot of things.  I'd be willing to try this approach out.

> Random thought: Would it be possible to change the contract for traveling down
> the ResourceHandler chain s.t. the base class always handled the next_handler_
> and calls to that handler went through the base class?  And (key point) we
> cancelled through that base class as well?  Then that base class could
> auto-defer and the RH subclass wouldn't have to worry about it.  As I say,
> random thought--there may well be things wrong with it.

I don't see any problems with doing things incrementally, though I could be
missing something.

> * I want to understand the intended usage in the async (defer) cases a bit
> better.  I (naively?) imagine that only one ResourceHandler can have a request
> deferred at any given time.  That RH is doing something async, and is
promising
> to get back to the RC with a Resume() (or Cancel()) at a later time, and all
> processing in the RH stack is on hold until that occurs.  What parts of this
> naive view are incorrect?  If I've got it right, under what circumstances
would
> we expect the a RH to get a callback after it's set the defer bit but before
> *it's* called Resume() (the concern you raise in your last paragraph, IIUC).

Other than cancellation, or one handler both pulling and pushing information at
the same time (Like the RedirectToFileHandler), there should only be one thing
going on at a time.  We don't defer the RedirectToFileHandler's messages,
though, so...I think this is basically the case.  So out-of-band cancellation
should be the only case here (Note that every RH can be confused by this - they
think things are deferred by an RH down the stack, and then see the
cancellation).  And of course, there's the double-defer case:  One defers
something, something else cancels, then cancellation is defered, and the
original RH tries to continue the request, which is what motivated this whole
thing.

> * Could you say a bit more about your model WRT OnRequestComplete(); what
kinds
> of external callers might want to be able to cancel at that point?  (I don't
> care a lot--I'm strongly in favor of API consistency, so I do want to extend
the
> pattern to OnRequestComplete/OnWillRead--but I do care about understanding the
> use case space, even if it's currently imaginary).

Any caller that doesn't know (Or check that) the request is actually complete. 
The AsyncResourceHandler may not know the request is complete when a new
navigation is started, cancelling existing requests (Or the stop button is
pressed, etc).  The NavigationResourceHandler has a similar same issue, I
believe.  There's also RenderFrame/RenderProcess teardown, which can happen at
any time.  To the extent of my knowledge, out of band cancellations mostly come
from the renderer, or change in renderer state, rather than from RHs/RTs.  It
looks like the SaveFileManager also may do this (At least it cancels requests
through the RDH interface, assume other things can be going on when it does
this, though this probably doesn't happen when using the only RH that delays
destruction).

[mmenke, 2016-11-18 23:08:08]

On 2016/11/18 21:38:42, Randy Smith - Not in Mondays wrote:
> Random thought: Would it be possible to change the contract for traveling down
> the ResourceHandler chain s.t. the base class always handled the next_handler_
> and calls to that handler went through the base class?  And (key point) we
> cancelled through that base class as well?  Then that base class could
> auto-defer and the RH subclass wouldn't have to worry about it.  As I say,
> random thought--there may well be things wrong with it.

Sorry, missed this.  I guess deferral would happen there, too?  I'm not a big
fan of this approach.  I'm not a fan of the whole "a.foo() calls b.bar() to do
stuff, b.bar() calls a.baz() to tell a.foo() to do stuff when b.foo() returns"
pattern.

Another option would be to just make things work like net:  Return net error
code, merge Resume and CancelWithError.

We could make out of band cancels have to go through the RDH and do sync
teardown (Regardless of what we do with the RH stack / ResourceControllers),
though not sure if that gets us anything.

Anyhow, it's 6 pm on a Friday.

[mmenke, 2016-11-22 17:53:03]

On 2016/11/18 23:08:08, mmenke wrote:
> On 2016/11/18 21:38:42, Randy Smith - Not in Mondays wrote:
> > Random thought: Would it be possible to change the contract for traveling
down
> > the ResourceHandler chain s.t. the base class always handled the
next_handler_
> > and calls to that handler went through the base class?  And (key point) we
> > cancelled through that base class as well?  Then that base class could
> > auto-defer and the RH subclass wouldn't have to worry about it.  As I say,
> > random thought--there may well be things wrong with it.
> 
> Sorry, missed this.  I guess deferral would happen there, too?  I'm not a big
> fan of this approach.  I'm not a fan of the whole "a.foo() calls b.bar() to do
> stuff, b.bar() calls a.baz() to tell a.foo() to do stuff when b.foo() returns"
> pattern.
> 
> Another option would be to just make things work like net:  Return net error
> code, merge Resume and CancelWithError.
> 
> We could make out of band cancels have to go through the RDH and do sync
> teardown (Regardless of what we do with the RH stack / ResourceControllers),
> though not sure if that gets us anything.
> 
> Anyhow, it's 6 pm on a Friday.

I've been thinking about this, and the idea's growing on me.  I've been wanting
a ResourceController per-ResourceHandler, to give us the ability to do sanity
checks, and that basically gives us this.  I'll try making a ResourceHandler
that does this, complete with wrapper for the old API, and see how things go.

[mmenke, 2016-11-22 18:55:11]

On 2016/11/22 17:53:03, mmenke wrote:
> On 2016/11/18 23:08:08, mmenke wrote:
> > On 2016/11/18 21:38:42, Randy Smith - Not in Mondays wrote:
> > > Random thought: Would it be possible to change the contract for traveling
> down
> > > the ResourceHandler chain s.t. the base class always handled the
> next_handler_
> > > and calls to that handler went through the base class?  And (key point) we
> > > cancelled through that base class as well?  Then that base class could
> > > auto-defer and the RH subclass wouldn't have to worry about it.  As I say,
> > > random thought--there may well be things wrong with it.

So I'm not sure about this...The RedirectToFileResourceHandler drives both the
URLRequest and the layers below it (Using a different message!) at the same
time.  I'm having trouble picturing a reasonable way for things like that to
work - same goes for the MimeTypeResourceHandler.  IF we just had the parent
class manage return/cancellation/resume, I think that works fine, just when we
have it managing next_handler_, too, that things get ugly.

[mmenke, 2016-12-02 15:28:02]

Description was changed from

==========
Refactor ResourceHandler API.

ResourceHandlers used to have one way to cancel requests synchronously,
and another to do so asynchronously.  Now there's only one way to do it:
Call into the ResourceController.  The downside to this is that when
cancelling the request, ResourceHandlers also need to tell the upstream
object that the request is "deferred".

This is part of a refactoring to make cancellation synchronous.

BUG=too lazy to look it up right now
==========

to

==========
Refactor ResourceHandler API.

ResourceHandlers used to have one way to cancel requests synchronously,
and another to do so asynchronously.  Now there's only one way to do it:
Call into the ResourceController.  The downside to this is that when
cancelling the request, ResourceHandlers also need to tell the upstream
object that the request is "deferred".

This is part of a refactoring to make cancellation synchronous.

BUG=too lazy to look it up right now
==========

[mmenke, 2016-12-02 15:28:02]

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