Refactor ResourceHandler API.

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}
Committed: https://chromium.googlesource.com/chromium/src/+/87f5c77aaa3541bee2f8d3b6b64d3879b8e81957

[mmenke, 2016-11-29 19:34:09]

Description was changed from

==========
Rework ResourceHandler chain.

BUG=none
==========

to

==========
Rework ResourceHandler chain.

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

[mmenke, 2016-11-29 19:43:56]

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

[mmenke, 2016-11-29 19:43:56]

Randy, here's the first pass with ResourceController ownership being
transferred.  There are certainly some warts here.  I like this approach a bit
less than I did before I ran into all of them, but still think it's better than
the old approach.

Would you mind doing an initial skim, to see if you think this approach looks
sane?

I'm not sure how reasonable it is to break this up - wrapping each API with the
other one seems like it could be pretty involved.

[mmenke, 2016-11-29 19:46:50]

And just to make sure we're on the same page:

The current state of this CL is that Chrome builds and seems to work, but I
haven't updated any tests yet.

[Randy Smith (Not in Mondays), 2016-11-30 17:36:51]

On 2016/11/29 19:46:50, mmenke wrote:
> And just to make sure we're on the same page:
> 
> The current state of this CL is that Chrome builds and seems to work, but I
> haven't updated any tests yet.

Sounds good.  

I'm of the opinion that I'm going to need to writeup a doc to keep the overall
picture here from leaking out my ears between our conversations, so I'm going to
skim this CL, grok some code, write that doc and run it by you.  What this means
is you may not hear from me for a day or so, but I *am* actually paying
attention :-}.  It's just that this stuff is tricky, and I want to make sure
I've got all the different trickinesses in mind when I do the high level review.

[mmenke, 2016-12-01 17:20:46]

https://codereview.chromium.org/2526983002/diff/20001/content/browser/downloa...
File content/browser/download/save_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/20001/content/browser/downloa...
content/browser/download/save_file_resource_handler.cc:98: controller->Resume();
Ahh...This is the one without back pressure.

[mmenke, 2016-12-06 18:07:26]

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

[commit-bot: I haz the power, 2016-12-06 18:08:03]

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-12-06 18:12:45]

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

[commit-bot: I haz the power, 2016-12-06 18:12:46]

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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-12-06 20:22:34]

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

[commit-bot: I haz the power, 2016-12-06 20:23:14]

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-12-06 20:37:28]

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

[commit-bot: I haz the power, 2016-12-06 20:37:28]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mmenke, 2016-12-06 21:58:11]

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

[commit-bot: I haz the power, 2016-12-06 21:59:11]

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-12-06 22:34:54]

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

[commit-bot: I haz the power, 2016-12-06 22:34:55]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2016-12-07 22:26:02]

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

[commit-bot: I haz the power, 2016-12-07 22:26:28]

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-12-07 22:42:04]

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

[commit-bot: I haz the power, 2016-12-07 22:42:05]

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

[mmenke, 2016-12-08 16:02:36]

Random comment:  It turns out that this CL can be divided into some components:

1)  Add ResourceHandler::OutOfBandCancel, and call it where needed.  We can do
some DCHECKing that the other methods aren't called in-band, but can't really
have guaranteed failures, since we don't know if the class that invoked the
in-band methods actually was the class that last deferred the request.  We could
even add a separate path for out of band cancels, indepedently of the other
work.

2)  Update tests.  It looks like I'll need a test ResourceHandler wrapper to
handle tracking/creation of the ResourceController, which I've already started
working on.  Conveniently, the test wrapper's API can be made so that it works
both before and after the refactor (Though it will seem like overengineering a
bit with the old API).

I'm not sure if it's possible to sanely incrementally update new
ResourceHandlers to use the new API - we'd probably need to wrap the new API in
the old one, and the old API in the new one.  Going one way is reasonable, but
both seems a bit much.  Maybe we could wrap the new API and the old one, and
start with terminal ResourceHandlers, and then do the rest in one go.  "The
rest" is where most of the ugliness is, but it would make the final CL touch
fewer files.  Anyhow, still working on basic functionality, don't think I'll
have something ready this year.

[Randy Smith (Not in Mondays), 2016-12-08 17:04:01]

On 2016/12/08 16:02:36, mmenke wrote:
> Random comment:  It turns out that this CL can be divided into some
components:
> 
> 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed.  We can do
> some DCHECKing that the other methods aren't called in-band, but can't really
> have guaranteed failures, since we don't know if the class that invoked the
> in-band methods actually was the class that last deferred the request.  We
could
> even add a separate path for out of band cancels, indepedently of the other
> work.
> 
> 2)  Update tests.  It looks like I'll need a test ResourceHandler wrapper to
> handle tracking/creation of the ResourceController, which I've already started
> working on.  Conveniently, the test wrapper's API can be made so that it works
> both before and after the refactor (Though it will seem like overengineering a
> bit with the old API).
> 
> I'm not sure if it's possible to sanely incrementally update new
> ResourceHandlers to use the new API - we'd probably need to wrap the new API
in
> the old one, and the old API in the new one.  Going one way is reasonable, but
> both seems a bit much.  Maybe we could wrap the new API and the old one, and
> start with terminal ResourceHandlers, and then do the rest in one go.  "The
> rest" is where most of the ugliness is, but it would make the final CL touch
> fewer files.  Anyhow, still working on basic functionality, don't think I'll
> have something ready this year.

I think it makes sense to peel the above two items out of this CL.  My
inclination is not to try to do an incremental update on the new API--the extra
work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I don't
think there's a lot of churn in this area.  So I'm ok with keeping the meat of
it in one cL.

[mmenke, 2016-12-08 17:21:16]

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

[commit-bot: I haz the power, 2016-12-08 17:21: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-12-08 18:25:00]

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

[commit-bot: I haz the power, 2016-12-08 18:25:01]

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

[mmenke, 2016-12-08 19:41:18]

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

[commit-bot: I haz the power, 2016-12-08 19:42:06]

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-12-08 20:53:42]

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

[commit-bot: I haz the power, 2016-12-08 20:53:43]

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

[Randy Smith (Not in Mondays), 2016-12-13 23:22:46]

On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> On 2016/12/08 16:02:36, mmenke wrote:
> > Random comment:  It turns out that this CL can be divided into some
> components:
> > 
> > 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed.  We can
do
> > some DCHECKing that the other methods aren't called in-band, but can't
really
> > have guaranteed failures, since we don't know if the class that invoked the
> > in-band methods actually was the class that last deferred the request.  We
> could
> > even add a separate path for out of band cancels, indepedently of the other
> > work.
> > 
> > 2)  Update tests.  It looks like I'll need a test ResourceHandler wrapper to
> > handle tracking/creation of the ResourceController, which I've already
started
> > working on.  Conveniently, the test wrapper's API can be made so that it
works
> > both before and after the refactor (Though it will seem like overengineering
a
> > bit with the old API).
> > 
> > I'm not sure if it's possible to sanely incrementally update new
> > ResourceHandlers to use the new API - we'd probably need to wrap the new API
> in
> > the old one, and the old API in the new one.  Going one way is reasonable,
but
> > both seems a bit much.  Maybe we could wrap the new API and the old one, and
> > start with terminal ResourceHandlers, and then do the rest in one go.  "The
> > rest" is where most of the ugliness is, but it would make the final CL touch
> > fewer files.  Anyhow, still working on basic functionality, don't think I'll
> > have something ready this year.
> 
> I think it makes sense to peel the above two items out of this CL.  My
> inclination is not to try to do an incremental update on the new API--the
extra
> work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I don't
> think there's a lot of churn in this area.  So I'm ok with keeping the meat of
> it in one cL.

What's the current status on this?  I'm ready to re-start review, but I'd like
to make sure I'm reviewing the right things.

[mmenke, 2016-12-14 03:22:54]

On 2016/12/13 23:22:46, Randy Smith - Not in Mondays wrote:
> On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> > On 2016/12/08 16:02:36, mmenke wrote:
> > > Random comment:  It turns out that this CL can be divided into some
> > components:
> > > 
> > > 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed.  We
can
> do
> > > some DCHECKing that the other methods aren't called in-band, but can't
> really
> > > have guaranteed failures, since we don't know if the class that invoked
the
> > > in-band methods actually was the class that last deferred the request.  We
> > could
> > > even add a separate path for out of band cancels, indepedently of the
other
> > > work.
> > > 
> > > 2)  Update tests.  It looks like I'll need a test ResourceHandler wrapper
to
> > > handle tracking/creation of the ResourceController, which I've already
> started
> > > working on.  Conveniently, the test wrapper's API can be made so that it
> works
> > > both before and after the refactor (Though it will seem like
overengineering
> a
> > > bit with the old API).
> > > 
> > > I'm not sure if it's possible to sanely incrementally update new
> > > ResourceHandlers to use the new API - we'd probably need to wrap the new
API
> > in
> > > the old one, and the old API in the new one.  Going one way is reasonable,
> but
> > > both seems a bit much.  Maybe we could wrap the new API and the old one,
and
> > > start with terminal ResourceHandlers, and then do the rest in one go. 
"The
> > > rest" is where most of the ugliness is, but it would make the final CL
touch
> > > fewer files.  Anyhow, still working on basic functionality, don't think
I'll
> > > have something ready this year.
> > 
> > I think it makes sense to peel the above two items out of this CL.  My
> > inclination is not to try to do an incremental update on the new API--the
> extra
> > work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I don't
> > think there's a lot of churn in this area.  So I'm ok with keeping the meat
of
> > it in one cL.
> 
> What's the current status on this?  I'm ready to re-start review, but I'd like
> to make sure I'm reviewing the right things.

A bunch of tests still need to be updated, ThrottlingResourceHandler lacks tests
(And it's a class that always double cancels on sync cancellation, and is
potentially dealing with both in-band and out-of-band cancels, without a clear
signal as to which it's doing)!

I don't think I'm going to write out-of-band cancel tests for the current API,
since behavior (And thus the tests) will be so different, but want to at least
write in-band ThrottlingResourceHandler tests, and introduce a test helper class
in that CL that can be updated to use the new API, so that's what I'm doing.  I
don't expect the non-test files to change significantly, other than merging and
fixing any bugs I uncover.

So:  Feel free to either start reviewing non-test changes (Well,
non-mojo_async_resource_handler_unittest.cc test changes, those are the only
ones I'm working on), or feel free to put it off.  Hope to get something out in
the next day or two.

I'm wondering about testing the out-of-band cancel tests for the new API.  Much
as I hate doing so, I may land this without them, and then add them in, I think
it's simplest if we wait until we change out-of-band throttling behavior to be a
sync teardown of everything before writing those tests.

[mmenke, 2016-12-14 03:33:02]

On 2016/12/14 03:22:54, mmenke wrote:
> On 2016/12/13 23:22:46, Randy Smith - Not in Mondays wrote:
> > On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> > > On 2016/12/08 16:02:36, mmenke wrote:
> > > > Random comment:  It turns out that this CL can be divided into some
> > > components:
> > > > 
> > > > 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed.  We
> can
> > do
> > > > some DCHECKing that the other methods aren't called in-band, but can't
> > really
> > > > have guaranteed failures, since we don't know if the class that invoked
> the
> > > > in-band methods actually was the class that last deferred the request. 
We
> > > could
> > > > even add a separate path for out of band cancels, indepedently of the
> other
> > > > work.
> > > > 
> > > > 2)  Update tests.  It looks like I'll need a test ResourceHandler
wrapper
> to
> > > > handle tracking/creation of the ResourceController, which I've already
> > started
> > > > working on.  Conveniently, the test wrapper's API can be made so that it
> > works
> > > > both before and after the refactor (Though it will seem like
> overengineering
> > a
> > > > bit with the old API).
> > > > 
> > > > I'm not sure if it's possible to sanely incrementally update new
> > > > ResourceHandlers to use the new API - we'd probably need to wrap the new
> API
> > > in
> > > > the old one, and the old API in the new one.  Going one way is
reasonable,
> > but
> > > > both seems a bit much.  Maybe we could wrap the new API and the old one,
> and
> > > > start with terminal ResourceHandlers, and then do the rest in one go. 
> "The
> > > > rest" is where most of the ugliness is, but it would make the final CL
> touch
> > > > fewer files.  Anyhow, still working on basic functionality, don't think
> I'll
> > > > have something ready this year.
> > > 
> > > I think it makes sense to peel the above two items out of this CL.  My
> > > inclination is not to try to do an incremental update on the new API--the
> > extra
> > > work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I
don't
> > > think there's a lot of churn in this area.  So I'm ok with keeping the
meat
> of
> > > it in one cL.
> > 
> > What's the current status on this?  I'm ready to re-start review, but I'd
like
> > to make sure I'm reviewing the right things.
> 
> A bunch of tests still need to be updated, ThrottlingResourceHandler lacks
tests
> (And it's a class that always double cancels on sync cancellation, and is
> potentially dealing with both in-band and out-of-band cancels, without a clear
> signal as to which it's doing)!
> 
> I don't think I'm going to write out-of-band cancel tests for the current API,
> since behavior (And thus the tests) will be so different, but want to at least
> write in-band ThrottlingResourceHandler tests, and introduce a test helper
class
> in that CL that can be updated to use the new API, so that's what I'm doing. 
I
> don't expect the non-test files to change significantly, other than merging
and
> fixing any bugs I uncover.
> 
> So:  Feel free to either start reviewing non-test changes (Well,
> non-mojo_async_resource_handler_unittest.cc test changes, those are the only
> ones I'm working on), or feel free to put it off.  Hope to get something out
in
> the next day or two.
> 
> I'm wondering about testing the out-of-band cancel tests for the new API. 
Much
> as I hate doing so, I may land this without them, and then add them in, I
think
> it's simplest if we wait until we change out-of-band throttling behavior to be
a
> sync teardown of everything before writing those tests.

Also unclear what ThrottlingResourceHandler should do - if there are multiple
throttles, one may cancel it out of band, and then another resumes it.  The
ThrottlingResourceHandler doesn't know it's an out-of-band-cancel, so we have a
couple options:

1)  Make callers explicitly tell us it's an out-of-band cancel.  There are
enough people writing ResourceThrottles, that I don't think this is a good idea.
2)  If it might be an in-band cancel, cancel the request asynchronously, but
ignore any resume message we get in the meantime, and if it is an out-of-band
cancel...Unclear.  Sync teardown?  Throttles may not be expecting that. 
Asyncronously do an out-of-band teardown, while ignoring all incoming events may
be better.
3)  Synchronously destroy all ResourceThrottles on cancel, sync or async, and
then post the sync/async cancel message immediately.  Act like a pass through
handler after that (Which is what we are, with no throttles).

Anyhow, not set on anything yet, and this is for a refactor *after* this one.

[mmenke, 2016-12-16 18:59:48]

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

[commit-bot: I haz the power, 2016-12-16 19:00:40]

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-12-16 19:03:54]

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

[commit-bot: I haz the power, 2016-12-16 19:03: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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mmenke, 2016-12-16 19:22:20]

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

[commit-bot: I haz the power, 2016-12-16 19:22: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-12-16 19:51:08]

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

[commit-bot: I haz the power, 2016-12-16 19:51:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[mmenke, 2016-12-16 20:32:18]

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

[commit-bot: I haz the power, 2016-12-16 20:34:21]

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-12-16 21:12:31]

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

[commit-bot: I haz the power, 2016-12-16 21:12:32]

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

[Randy Smith (Not in Mondays), 2016-12-16 21:37:27]

[Feel free to ignore the below; they were nits during my initial read-through.]

Would you be willing to rebase this before you disappear for the holidays?  I'd
like to get a patch to look at as I review, and it doesn't currently patch to
ToT.

https://codereview.chromium.org/2526983002/diff/20001/content/browser/downloa...
File content/browser/download/save_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/20001/content/browser/downloa...
content/browser/download/save_file_resource_handler.cc:98: controller->Resume();
On 2016/12/01 17:20:46, mmenke wrote:
> Ahh...This is the one without back pressure.

Ah, yuck.

On the bright side, It's only involved after the resource has been downloaded
once, and only in response to user request.  So something like a GZIP bomb is
unlikely.

I'll file a bug on this.

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:412: // TODO(mmenke):  Should
fail with ERR_INSUFFICIENT_RESOURCES here.
Is there a reason why CancelWithError() can't be called from here?  (This may be
"haven't gotten to it but plan to in this CL" issue, in which case feel free to
ignore this comment.)

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler.cc:75: // TODO(yhirano):
Retaining ownership from a raw pointer is bad.
A sense a disturbance in the force ... as if several kittens died when the CL
containing this line landed ....

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/mime_sniffing_resource_handler.h:53: class Controller;
Is this needed?  A forward decl that isn't referenced in the .h file?

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/redirect_to_file_resource_handler.cc:231: // Note that
|controller_| can't be nullptr here.
I don't follow this comment; it looks like controller_ *is* nullptr here.

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/resource_handler.h:49: // which point the request will be
resumed synchronously.
nit: Implies Cancel() will resume the request, which I don't think is what you
intended?

[mmenke, 2016-12-16 21:54:08]

On 2016/12/16 21:37:27, Randy Smith - Not in Mondays wrote:
> [Feel free to ignore the below; they were nits during my initial
read-through.]
> 
> Would you be willing to rebase this before you disappear for the holidays? 
I'd
> like to get a patch to look at as I review, and it doesn't currently patch to
> ToT.

I merged it 2 hours ago...Do you mean merge again, or did you run into issues
more recently than that?

[Randy Smith (Not in Mondays), 2016-12-16 22:01:45]

On 2016/12/16 21:54:08, mmenke (Out Dec 17 to Jan 2) wrote:
> On 2016/12/16 21:37:27, Randy Smith - Not in Mondays wrote:
> > [Feel free to ignore the below; they were nits during my initial
> read-through.]
> > 
> > Would you be willing to rebase this before you disappear for the holidays? 
> I'd
> > like to get a patch to look at as I review, and it doesn't currently patch
to
> > ToT.
> 
> I merged it 2 hours ago...Do you mean merge again, or did you run into issues
> more recently than that?

Arggh, I think it's the dependent patchset adding files modified in this
patchset problem.  (Filed a bug, changing code review tools, not really looked
at.)  I'll merge by hand.

[Randy Smith (Not in Mondays), 2016-12-16 22:06:32]

On 2016/12/14 03:33:02, mmenke (Out Dec 17 to Jan 2) wrote:
> On 2016/12/14 03:22:54, mmenke wrote:
> > On 2016/12/13 23:22:46, Randy Smith - Not in Mondays wrote:
> > > On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> > > > On 2016/12/08 16:02:36, mmenke wrote:
> > > > > Random comment:  It turns out that this CL can be divided into some
> > > > components:
> > > > > 
> > > > > 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed. 
We
> > can
> > > do
> > > > > some DCHECKing that the other methods aren't called in-band, but can't
> > > really
> > > > > have guaranteed failures, since we don't know if the class that
invoked
> > the
> > > > > in-band methods actually was the class that last deferred the request.

> We
> > > > could
> > > > > even add a separate path for out of band cancels, indepedently of the
> > other
> > > > > work.
> > > > > 
> > > > > 2)  Update tests.  It looks like I'll need a test ResourceHandler
> wrapper
> > to
> > > > > handle tracking/creation of the ResourceController, which I've already
> > > started
> > > > > working on.  Conveniently, the test wrapper's API can be made so that
it
> > > works
> > > > > both before and after the refactor (Though it will seem like
> > overengineering
> > > a
> > > > > bit with the old API).
> > > > > 
> > > > > I'm not sure if it's possible to sanely incrementally update new
> > > > > ResourceHandlers to use the new API - we'd probably need to wrap the
new
> > API
> > > > in
> > > > > the old one, and the old API in the new one.  Going one way is
> reasonable,
> > > but
> > > > > both seems a bit much.  Maybe we could wrap the new API and the old
one,
> > and
> > > > > start with terminal ResourceHandlers, and then do the rest in one go. 
> > "The
> > > > > rest" is where most of the ugliness is, but it would make the final CL
> > touch
> > > > > fewer files.  Anyhow, still working on basic functionality, don't
think
> > I'll
> > > > > have something ready this year.
> > > > 
> > > > I think it makes sense to peel the above two items out of this CL.  My
> > > > inclination is not to try to do an incremental update on the new
API--the
> > > extra
> > > > work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I
> don't
> > > > think there's a lot of churn in this area.  So I'm ok with keeping the
> meat
> > of
> > > > it in one cL.
> > > 
> > > What's the current status on this?  I'm ready to re-start review, but I'd
> like
> > > to make sure I'm reviewing the right things.
> > 
> > A bunch of tests still need to be updated, ThrottlingResourceHandler lacks
> tests
> > (And it's a class that always double cancels on sync cancellation, and is
> > potentially dealing with both in-band and out-of-band cancels, without a
clear
> > signal as to which it's doing)!
> > 
> > I don't think I'm going to write out-of-band cancel tests for the current
API,
> > since behavior (And thus the tests) will be so different, but want to at
least
> > write in-band ThrottlingResourceHandler tests, and introduce a test helper
> class
> > in that CL that can be updated to use the new API, so that's what I'm doing.

> I
> > don't expect the non-test files to change significantly, other than merging
> and
> > fixing any bugs I uncover.
> > 
> > So:  Feel free to either start reviewing non-test changes (Well,
> > non-mojo_async_resource_handler_unittest.cc test changes, those are the only
> > ones I'm working on), or feel free to put it off.  Hope to get something out
> in
> > the next day or two.
> > 
> > I'm wondering about testing the out-of-band cancel tests for the new API. 
> Much
> > as I hate doing so, I may land this without them, and then add them in, I
> think
> > it's simplest if we wait until we change out-of-band throttling behavior to
be
> a
> > sync teardown of everything before writing those tests.
> 
> Also unclear what ThrottlingResourceHandler should do - if there are multiple
> throttles, one may cancel it out of band, and then another resumes it.  

What are the use cases for ResourceThrottles doing OOBC?  I think of throttles
as making a go/no-go decision, and after that being hands off, and if they need
to make the go/no-go decision asynchronously, deferring (in which case they
should be the only ones to resume the request, or cancel it).  

> The
> ThrottlingResourceHandler doesn't know it's an out-of-band-cancel, so we have
a
> couple options:
> 
> 1)  Make callers explicitly tell us it's an out-of-band cancel.  There are
> enough people writing ResourceThrottles, that I don't think this is a good
idea.
> 2)  If it might be an in-band cancel, cancel the request asynchronously, but
> ignore any resume message we get in the meantime, and if it is an out-of-band
> cancel...Unclear.  Sync teardown?  Throttles may not be expecting that. 
> Asyncronously do an out-of-band teardown, while ignoring all incoming events
may
> be better.
> 3)  Synchronously destroy all ResourceThrottles on cancel, sync or async, and
> then post the sync/async cancel message immediately.  Act like a pass through
> handler after that (Which is what we are, with no throttles).
> 
> Anyhow, not set on anything yet, and this is for a refactor *after* this one.

[mmenke, 2016-12-16 22:06:59]

On 2016/12/16 22:01:45, Randy Smith - Not in Mondays wrote:
> On 2016/12/16 21:54:08, mmenke (Out Dec 17 to Jan 2) wrote:
> > On 2016/12/16 21:37:27, Randy Smith - Not in Mondays wrote:
> > > [Feel free to ignore the below; they were nits during my initial
> > read-through.]
> > > 
> > > Would you be willing to rebase this before you disappear for the holidays?

> > I'd
> > > like to get a patch to look at as I review, and it doesn't currently patch
> to
> > > ToT.
> > 
> > I merged it 2 hours ago...Do you mean merge again, or did you run into
issues
> > more recently than that?
> 
> Arggh, I think it's the dependent patchset adding files modified in this
> patchset problem.  (Filed a bug, changing code review tools, not really looked
> at.)  I'll merge by hand.

I have one more depdendent patchset I want to upload today, not sure I'll quite
get it + merge done before I head out, though.

[mmenke, 2016-12-16 22:09:12]

On 2016/12/16 22:06:32, Randy Smith - Not in Mondays wrote:
> On 2016/12/14 03:33:02, mmenke (Out Dec 17 to Jan 2) wrote:
> > On 2016/12/14 03:22:54, mmenke wrote:
> > > On 2016/12/13 23:22:46, Randy Smith - Not in Mondays wrote:
> > > > On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> > > > > On 2016/12/08 16:02:36, mmenke wrote:
> > > > > > Random comment:  It turns out that this CL can be divided into some
> > > > > components:
> > > > > > 
> > > > > > 1)  Add ResourceHandler::OutOfBandCancel, and call it where needed. 
> We
> > > can
> > > > do
> > > > > > some DCHECKing that the other methods aren't called in-band, but
can't
> > > > really
> > > > > > have guaranteed failures, since we don't know if the class that
> invoked
> > > the
> > > > > > in-band methods actually was the class that last deferred the
request.
> 
> > We
> > > > > could
> > > > > > even add a separate path for out of band cancels, indepedently of
the
> > > other
> > > > > > work.
> > > > > > 
> > > > > > 2)  Update tests.  It looks like I'll need a test ResourceHandler
> > wrapper
> > > to
> > > > > > handle tracking/creation of the ResourceController, which I've
already
> > > > started
> > > > > > working on.  Conveniently, the test wrapper's API can be made so
that
> it
> > > > works
> > > > > > both before and after the refactor (Though it will seem like
> > > overengineering
> > > > a
> > > > > > bit with the old API).
> > > > > > 
> > > > > > I'm not sure if it's possible to sanely incrementally update new
> > > > > > ResourceHandlers to use the new API - we'd probably need to wrap the
> new
> > > API
> > > > > in
> > > > > > the old one, and the old API in the new one.  Going one way is
> > reasonable,
> > > > but
> > > > > > both seems a bit much.  Maybe we could wrap the new API and the old
> one,
> > > and
> > > > > > start with terminal ResourceHandlers, and then do the rest in one
go. 
> > > "The
> > > > > > rest" is where most of the ugliness is, but it would make the final
CL
> > > touch
> > > > > > fewer files.  Anyhow, still working on basic functionality, don't
> think
> > > I'll
> > > > > > have something ready this year.
> > > > > 
> > > > > I think it makes sense to peel the above two items out of this CL.  My
> > > > > inclination is not to try to do an incremental update on the new
> API--the
> > > > extra
> > > > > work just doesn't seem worth it.  This CL isn't *that* (:-}) big and I
> > don't
> > > > > think there's a lot of churn in this area.  So I'm ok with keeping the
> > meat
> > > of
> > > > > it in one cL.
> > > > 
> > > > What's the current status on this?  I'm ready to re-start review, but
I'd
> > like
> > > > to make sure I'm reviewing the right things.
> > > 
> > > A bunch of tests still need to be updated, ThrottlingResourceHandler lacks
> > tests
> > > (And it's a class that always double cancels on sync cancellation, and is
> > > potentially dealing with both in-band and out-of-band cancels, without a
> clear
> > > signal as to which it's doing)!
> > > 
> > > I don't think I'm going to write out-of-band cancel tests for the current
> API,
> > > since behavior (And thus the tests) will be so different, but want to at
> least
> > > write in-band ThrottlingResourceHandler tests, and introduce a test helper
> > class
> > > in that CL that can be updated to use the new API, so that's what I'm
doing.
> 
> > I
> > > don't expect the non-test files to change significantly, other than
merging
> > and
> > > fixing any bugs I uncover.
> > > 
> > > So:  Feel free to either start reviewing non-test changes (Well,
> > > non-mojo_async_resource_handler_unittest.cc test changes, those are the
only
> > > ones I'm working on), or feel free to put it off.  Hope to get something
out
> > in
> > > the next day or two.
> > > 
> > > I'm wondering about testing the out-of-band cancel tests for the new API. 
> > Much
> > > as I hate doing so, I may land this without them, and then add them in, I
> > think
> > > it's simplest if we wait until we change out-of-band throttling behavior
to
> be
> > a
> > > sync teardown of everything before writing those tests.
> > 
> > Also unclear what ThrottlingResourceHandler should do - if there are
multiple
> > throttles, one may cancel it out of band, and then another resumes it.  
> 
> What are the use cases for ResourceThrottles doing OOBC?  I think of throttles
> as making a go/no-go decision, and after that being hands off, and if they
need
> to make the go/no-go decision asynchronously, deferring (in which case they
> should be the only ones to resume the request, or cancel it).  

The NavigationResourceThrottle cancels in the case of navigation cancellation. 
I think we may also race remote safe-browsing checks on Android, though not sure
about that.

> > The
> > ThrottlingResourceHandler doesn't know it's an out-of-band-cancel, so we
have
> a
> > couple options:
> > 
> > 1)  Make callers explicitly tell us it's an out-of-band cancel.  There are
> > enough people writing ResourceThrottles, that I don't think this is a good
> idea.
> > 2)  If it might be an in-band cancel, cancel the request asynchronously, but
> > ignore any resume message we get in the meantime, and if it is an
out-of-band
> > cancel...Unclear.  Sync teardown?  Throttles may not be expecting that. 
> > Asyncronously do an out-of-band teardown, while ignoring all incoming events
> may
> > be better.
> > 3)  Synchronously destroy all ResourceThrottles on cancel, sync or async,
and
> > then post the sync/async cancel message immediately.  Act like a pass
through
> > handler after that (Which is what we are, with no throttles).
> > 
> > Anyhow, not set on anything yet, and this is for a refactor *after* this
one.

[Randy Smith (Not in Mondays), 2016-12-20 20:54:19]

On 2016/12/16 22:09:12, mmenke (Out Dec 17 to Jan 2) wrote:
> On 2016/12/16 22:06:32, Randy Smith - Not in Mondays wrote:
> > On 2016/12/14 03:33:02, mmenke (Out Dec 17 to Jan 2) wrote:
> > > On 2016/12/14 03:22:54, mmenke wrote:
> > > > On 2016/12/13 23:22:46, Randy Smith - Not in Mondays wrote:
> > > > > On 2016/12/08 17:04:01, Randy Smith - Not in Mondays wrote:
> > > > > > On 2016/12/08 16:02:36, mmenke wrote:
> > > > > > > Random comment:  It turns out that this CL can be divided into
some
> > > > > > components:
> > > > > > > 
> > > > > > > 1)  Add ResourceHandler::OutOfBandCancel, and call it where
needed. 
> > We
> > > > can
> > > > > do
> > > > > > > some DCHECKing that the other methods aren't called in-band, but
> can't
> > > > > really
> > > > > > > have guaranteed failures, since we don't know if the class that
> > invoked
> > > > the
> > > > > > > in-band methods actually was the class that last deferred the
> request.
> > 
> > > We
> > > > > > could
> > > > > > > even add a separate path for out of band cancels, indepedently of
> the
> > > > other
> > > > > > > work.
> > > > > > > 
> > > > > > > 2)  Update tests.  It looks like I'll need a test ResourceHandler
> > > wrapper
> > > > to
> > > > > > > handle tracking/creation of the ResourceController, which I've
> already
> > > > > started
> > > > > > > working on.  Conveniently, the test wrapper's API can be made so
> that
> > it
> > > > > works
> > > > > > > both before and after the refactor (Though it will seem like
> > > > overengineering
> > > > > a
> > > > > > > bit with the old API).
> > > > > > > 
> > > > > > > I'm not sure if it's possible to sanely incrementally update new
> > > > > > > ResourceHandlers to use the new API - we'd probably need to wrap
the
> > new
> > > > API
> > > > > > in
> > > > > > > the old one, and the old API in the new one.  Going one way is
> > > reasonable,
> > > > > but
> > > > > > > both seems a bit much.  Maybe we could wrap the new API and the
old
> > one,
> > > > and
> > > > > > > start with terminal ResourceHandlers, and then do the rest in one
> go. 
> > > > "The
> > > > > > > rest" is where most of the ugliness is, but it would make the
final
> CL
> > > > touch
> > > > > > > fewer files.  Anyhow, still working on basic functionality, don't
> > think
> > > > I'll
> > > > > > > have something ready this year.
> > > > > > 
> > > > > > I think it makes sense to peel the above two items out of this CL. 
My
> > > > > > inclination is not to try to do an incremental update on the new
> > API--the
> > > > > extra
> > > > > > work just doesn't seem worth it.  This CL isn't *that* (:-}) big and
I
> > > don't
> > > > > > think there's a lot of churn in this area.  So I'm ok with keeping
the
> > > meat
> > > > of
> > > > > > it in one cL.
> > > > > 
> > > > > What's the current status on this?  I'm ready to re-start review, but
> I'd
> > > like
> > > > > to make sure I'm reviewing the right things.
> > > > 
> > > > A bunch of tests still need to be updated, ThrottlingResourceHandler
lacks
> > > tests
> > > > (And it's a class that always double cancels on sync cancellation, and
is
> > > > potentially dealing with both in-band and out-of-band cancels, without a
> > clear
> > > > signal as to which it's doing)!
> > > > 
> > > > I don't think I'm going to write out-of-band cancel tests for the
current
> > API,
> > > > since behavior (And thus the tests) will be so different, but want to at
> > least
> > > > write in-band ThrottlingResourceHandler tests, and introduce a test
helper
> > > class
> > > > in that CL that can be updated to use the new API, so that's what I'm
> doing.
> > 
> > > I
> > > > don't expect the non-test files to change significantly, other than
> merging
> > > and
> > > > fixing any bugs I uncover.
> > > > 
> > > > So:  Feel free to either start reviewing non-test changes (Well,
> > > > non-mojo_async_resource_handler_unittest.cc test changes, those are the
> only
> > > > ones I'm working on), or feel free to put it off.  Hope to get something
> out
> > > in
> > > > the next day or two.
> > > > 
> > > > I'm wondering about testing the out-of-band cancel tests for the new
API. 
> > > Much
> > > > as I hate doing so, I may land this without them, and then add them in,
I
> > > think
> > > > it's simplest if we wait until we change out-of-band throttling behavior
> to
> > be
> > > a
> > > > sync teardown of everything before writing those tests.
> > > 
> > > Also unclear what ThrottlingResourceHandler should do - if there are
> multiple
> > > throttles, one may cancel it out of band, and then another resumes it.  
> > 
> > What are the use cases for ResourceThrottles doing OOBC?  I think of
throttles
> > as making a go/no-go decision, and after that being hands off, and if they
> need
> > to make the go/no-go decision asynchronously, deferring (in which case they
> > should be the only ones to resume the request, or cancel it).  
> 
> The NavigationResourceThrottle cancels in the case of navigation cancellation.


I need to work on my the precision of my terminology :-}.  It looks to me from
reading the code as if the NavigationResourceThrottle only cancels if it has
deferred the request, which I'd call async cancellation but not OOBC. 
Specifically, I don't think cancels from a RT that has deferred the request have
the Resume/Cancel race possibility, given that the throttles are run
sequentially, and a throttle deferring a request blocks later throttles from
being run?

(I'm finding myself wanting to reduce the set of out of band cancels small
enough that we can special case them, and possibly make them completely
synchronous.  This is sorta what we're doing on the ResourceHandler stack; I
would think we could extend that to ResourceThrottle.) . 

> I think we may also race remote safe-browsing checks on Android, though not
sure
> about that.
> 
> > > The
> > > ThrottlingResourceHandler doesn't know it's an out-of-band-cancel, so we
> have
> > a
> > > couple options:
> > > 
> > > 1)  Make callers explicitly tell us it's an out-of-band cancel.  There are
> > > enough people writing ResourceThrottles, that I don't think this is a good
> > idea.
> > > 2)  If it might be an in-band cancel, cancel the request asynchronously,
but
> > > ignore any resume message we get in the meantime, and if it is an
> out-of-band
> > > cancel...Unclear.  Sync teardown?  Throttles may not be expecting that. 
> > > Asyncronously do an out-of-band teardown, while ignoring all incoming
events
> > may
> > > be better.
> > > 3)  Synchronously destroy all ResourceThrottles on cancel, sync or async,
> and
> > > then post the sync/async cancel message immediately.  Act like a pass
> through
> > > handler after that (Which is what we are, with no throttles).
> > > 
> > > Anyhow, not set on anything yet, and this is for a refactor *after* this
> one.

[mmenke, 2016-12-22 16:29:35]

Quick responses while I'm checking my email anyways...

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/async_resource_handler.cc:412: // TODO(mmenke):  Should
fail with ERR_INSUFFICIENT_RESOURCES here.
On 2016/12/16 21:37:26, Randy Smith - Not in Mondays wrote:
> Is there a reason why CancelWithError() can't be called from here?  (This may
be
> "haven't gotten to it but plan to in this CL" issue, in which case feel free
to
> ignore this comment.)

We don't have a ResourceController to call it on.  OnWillRead always completes
synchronously and returns a bool.  I want to make it take a controller like
everything else, but that's well beyond the scope of this CL.  The fact that
this code called cancel and returned false is a bug.

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/intercepting_resource_handler.cc:75: // TODO(yhirano):
Retaining ownership from a raw pointer is bad.
On 2016/12/16 21:37:26, Randy Smith - Not in Mondays wrote:
> A sense a disturbance in the force ... as if several kittens died when the CL
> containing this line landed ....

We were already doing it in the MimeBlahResourceHandler, so it's more their
corpses were dug up and buried elsewhere.

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/mime_sniffing_resource_handler.h:53: class Controller;
On 2016/12/16 21:37:26, Randy Smith - Not in Mondays wrote:
> Is this needed?  A forward decl that isn't referenced in the .h file?

It calls private methods, so has to be an inner class (Or a friend class) to
access them.  Either way, has to appear in this file.

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

https://codereview.chromium.org/2526983002/diff/20001/content/browser/loader/...
content/browser/loader/redirect_to_file_resource_handler.cc:231: // Note that
|controller_| can't be nullptr here.
On 2016/12/16 21:37:26, Randy Smith - Not in Mondays wrote:
> I don't follow this comment; it looks like controller_ *is* nullptr here.

|controller| vs |controller_|.  If a read completes, one or the other is not
nullptr.  (i.e., if this is the BufIsFull() path, we've set controller_ but not
destroyed it/used it in WriteMore()).

[Randy Smith (Not in Mondays), 2016-12-31 22:00:25]

Ok, I've done another round of review (though still at a high level).  I'm
getting close to being willing to let go of the high level API redesign
questions, at which point I think the review cycles will go noticeably faster
:-}.  But before that, a couple of thoughts/questions:

* I find myself wondering if the lifetime of the ResourceController in the
ResourceHandler stack can be used instead of having any kind of explicit
*defer/Resume() functionality.  The idea here would be that while the
ResourceController passed into the handler chain was alive, the ResourceLoader
would not further drive the request, and when it died, that would be a signal to
restart driving the requests.   I believe that there's a 1-1 match between
holding onto the controller and wanting the ResourceLoader (or other
ResourceControllers) to hold off on further driving the requests.  There would a
bit of tricky plumbing if we wanted to avoid the PostTask in the (presumably
common) synchronous case, but I suspect that could be done.  And I think it
simplifies the code.

* I find myself disliking both the out of band cancel call and the
ResourceHandler::Delegate class that is used for propagating it.  But I think
figuring out something better is in many ways its own refactor, and this CL (+ a
additional CL I think you're planning  that propagates this new API into the
ResourceThrottles) will make that analysis and refactor much easier.  So for the
moment I'll let go of trying to rethink OOBC, and if I come back to it it'll be
with an eye on another CL.  

* Because of the above point this is lower priority, but you didn't respond to
my question about NavigationResourceThrottles cancel in c#54.  I read it as
being async, but not OOB; willing to clarify why you think of it as OOB?  Or is
this just a matter of the API rework not being pushed into the throttles yet?

Thanks much for your patience.

https://codereview.chromium.org/2526983002/diff/220001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/220001/content/browser/loader...
content/browser/loader/resource_handler.h:72: // which point the request will be
resumed synchronously.
I'm struggling with the actual behavior and the appropriate documentation around
the resumption.  The implementation of Resume in ResourceLoader::Controller
looks asynchronous to me (it does a PostTask).  But I'm not sure whether that's
something that should be documented at this level or not.  But if it shouldn't
be, I don't understand what describing the request as being resumed
synchronously means.  WDYT?

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/redirect_to_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/redirect_to_file_resource_handler.cc:217: if
(!WriteMore()) {
Why the move before BufIsFull()?  It looks like a functional change (albeit a
good one, as it ups the chances of the buffer not being full).

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/resource_handler.h:103: // succeed asynchronously.
I could imagine that doing this change at the conceptual level (which I think is
primarily handling a defer response at the ResourceLoader level and in the other
ResourceControllers) might be a smaller change than this one.  Maybe worth doing
before this CL so that this CL can be a consistent API change?  Up to you.

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/resource_handler.h:136: //  into deleting objects.
Suggestion: I was tripped up a little bit in my review by the lack of obvious
symmetry between set_controller() and TakeController().  Maybe worthwhile
renaming to SetController (or maybe HoldController/ReleaseController()?) to make
the pattern clearer?

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/throttling_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/throttling_resource_handler.cc:46:
throttles_[index]->WillRedirectRequest(redirect_info, &defer);
Is it your plan to push the API change into the throttles?  If so, is there a
reason to do RH->RT rather than the reverse?  (I don't have an opinion, just
wanted to make sure I understand the plan and its motivations.)

[mmenke, 2017-01-05 18:46:20]

Description was changed from

==========
Rework ResourceHandler chain.

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

to

==========
Rework ResourceHandler chain.

BUG=659317
==========

[mmenke, 2017-01-06 17:32:45]

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

[commit-bot: I haz the power, 2017-01-06 17:33: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, 2017-01-06 18:18:07]

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

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

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, 2017-01-06 18:46:58]

Going to dig into these test failures next (And then into the RedirectToFile
issue I mentioned on the other CL).

https://codereview.chromium.org/2526983002/diff/220001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/220001/content/browser/loader...
content/browser/loader/resource_handler.h:72: // which point the request will be
resumed synchronously.
On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> I'm struggling with the actual behavior and the appropriate documentation
around
> the resumption.  The implementation of Resume in ResourceLoader::Controller
> looks asynchronous to me (it does a PostTask).  But I'm not sure whether
that's
> something that should be documented at this level or not.  But if it shouldn't
> be, I don't understand what describing the request as being resumed
> synchronously means.  WDYT?

There isn't a PostTask:  If we call Resume() immediately on the
ResourceLoader::Controller (Which is in state DEFERRED_SYNC), the controller
does nothing but update its state.  Then when we're back up the stack in
ResourceLoader, it synchronously continues the request.

If we call Resume() asynchronously, then ResourceLoader::Resume() will resume
the request synchronously in exactly the case where it's called from the
ResourceLoader.  However, we're careful not to call back into a ResourceHandler
in that case.

That having been said, that seems more an implementation detail that the RH
shouldn't care about.  All it should know is that it won't be called into
synchronously, so I've updated the comments.

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/redirect_to_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/redirect_to_file_resource_handler.cc:217: if
(!WriteMore()) {
On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> Why the move before BufIsFull()?  It looks like a functional change (albeit a
> good one, as it ups the chances of the buffer not being full).

Because it makes control flow simpler.  I really don't like:

if (BufIsFull()) {
  set_controller(std::move(controller));
  ...
}

if (!WriteMore()) {
  if (has_controller()) {
    controller_->Cancel();
  } else {
    controller->Cancel();
  }
}

if (!has_controller())
  controller->Resume();

(Note that because std::move is odd, have to check has_controller() instead of
controller, though even if that wasn't the case, this would still be really
ugly)

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/resource_handler.h:103: // succeed asynchronously.
On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> I could imagine that doing this change at the conceptual level (which I think
is
> primarily handling a defer response at the ResourceLoader level and in the
other
> ResourceControllers) might be a smaller change than this one.  Maybe worth
doing
> before this CL so that this CL can be a consistent API change?  Up to you.

I think it's less work to convert to the new API first, then convert this method
over.  It'll certainly be a smaller change than this one, but I'll have to go
over all implementations twice to do the change (Once to be able to defer, once
to use the new API - and write tests for the old API, and update them all to the
new one).

I'm also planning on switching the Mojo handler over to taking advantage of it
in the same CL I add the new API in, as well as add a bunch of tests, so it's
actually going to be a good sized CL.

Admittedly, I'm in a bit of a hurry to just finally land this CL, given how long
it's been since I started working on it.  If you feel strongly enough that this
method should be updated first, though, I can certainly do that.

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/resource_handler.h:136: //  into deleting objects.
On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> Suggestion: I was tripped up a little bit in my review by the lack of obvious
> symmetry between set_controller() and TakeController().  Maybe worthwhile
> renaming to SetController (or maybe HoldController/ReleaseController()?) to
make
> the pattern clearer?

Good idea.  I've gone with HoldController()/ReleaseController() (Good idea about
renaming "TakeController", too - it's a very ambiguous name).

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
File content/browser/loader/throttling_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/240001/content/browser/loader...
content/browser/loader/throttling_resource_handler.cc:46:
throttles_[index]->WillRedirectRequest(redirect_info, &defer);
On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> Is it your plan to push the API change into the throttles?  If so, is there a
> reason to do RH->RT rather than the reverse?  (I don't have an opinion, just
> wanted to make sure I understand the plan and its motivations.)

I'm not currently planning on changing the API here, though may want to either
ban OOB-cancels from them or do sync destruction in that case.  Since RH's are
mostly loader-implemented, and RT's are embedded-implemented, I think we want to
bend over backwards a bit to make the RT API dead simple, and separating out
cancel types seems to go against that.

I do want to clean up this class (And maybe the API as well), but I'm thinking
that should be done after OOB-cancellation becomes synchronous.  I've also kinda
forgotten just how I was thinking on cleaning this class up...

[mmenke, 2017-01-06 20:18:11]

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

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

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, 2017-01-06 20:58:29]

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

[commit-bot: I haz the power, 2017-01-06 20:58:30]

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, 2017-01-06 21:38:13]

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

[commit-bot: I haz the power, 2017-01-06 21:38:35]

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

[mmenke, 2017-01-06 22:18:15]

Sorry, forgot to respond your bullet points.

On 2016/12/31 22:00:25, Randy Smith - Not in Mondays wrote:
> Ok, I've done another round of review (though still at a high level).  I'm
> getting close to being willing to let go of the high level API redesign
> questions, at which point I think the review cycles will go noticeably faster
> :-}.  But before that, a couple of thoughts/questions:
> 
> * I find myself wondering if the lifetime of the ResourceController in the
> ResourceHandler stack can be used instead of having any kind of explicit
> *defer/Resume() functionality.  The idea here would be that while the
> ResourceController passed into the handler chain was alive, the ResourceLoader
> would not further drive the request, and when it died, that would be a signal
to
> restart driving the requests.   I believe that there's a 1-1 match between
> holding onto the controller and wanting the ResourceLoader (or other
> ResourceControllers) to hold off on further driving the requests.  There would
a
> bit of tricky plumbing if we wanted to avoid the PostTask in the (presumably
> common) synchronous case, but I suspect that could be done.  And I think it
> simplifies the code.

I did consider doing this, but decided against it for two reasons:

1)  Cancellation would still need a call, which seems asymmetric and a bit
weird, though suppose we could do cancel and then destroy.
2)  Starting things in destructors seems like an anti-pattern to me.  I'd be
happier with destruction meaning cancel than resume.

I think that while it may make code look a bit simpler, but it would be at the
cost of making how the code works more subtle and complex.  Happy to talk
further about going this direction, though, I could certainly be wrong.

> * I find myself disliking both the out of band cancel call and the
> ResourceHandler::Delegate class that is used for propagating it.  But I think
> figuring out something better is in many ways its own refactor, and this CL (+
a
> additional CL I think you're planning  that propagates this new API into the
> ResourceThrottles) will make that analysis and refactor much easier.  So for
the
> moment I'll let go of trying to rethink OOBC, and if I come back to it it'll
be
> with an eye on another CL.  

I'm also not a fan of OOB cancel, and having to make an RH::Delegate, but I
actually do think explicit OOB cancels are better than what we have now.  Once
OOB cancels can synchronously delete the ResourceLoader, we can switch the
AsyncResourceHandlers to using it for their sync teardown paths, which I think
is much better than having them go through RDH to do this.  This gets us some
ways in decoupling the RL stack from the RDH, and makes things a bit more
self-contained, which I think is good.

> * Because of the above point this is lower priority, but you didn't respond to
> my question about NavigationResourceThrottles cancel in c#54.  I read it as
> being async, but not OOB; willing to clarify why you think of it as OOB?  Or
is
> this just a matter of the API rework not being pushed into the throttles yet?

Mentioned this somewhere already, but just to have it coupled with your
question:  You're right, I was thinking of the NavigationRH.

> Thanks much for your patience.

[commit-bot: I haz the power, 2017-01-06 23:23:38]

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

[commit-bot: I haz the power, 2017-01-06 23:23:39]

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

[mmenke, 2017-01-13 16:46:35]

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

[commit-bot: I haz the power, 2017-01-13 16:46:59]

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, 2017-01-13 17:02:42]

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

[commit-bot: I haz the power, 2017-01-13 17:02:44]

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

[mmenke, 2017-01-13 20:20:02]

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

[commit-bot: I haz the power, 2017-01-13 20:20:54]

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, 2017-01-13 20:48:35]

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

[commit-bot: I haz the power, 2017-01-13 20:48:37]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mmenke, 2017-01-13 20:57:38]

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

[commit-bot: I haz the power, 2017-01-13 20:58:34]

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, 2017-01-13 21:20:49]

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

[commit-bot: I haz the power, 2017-01-13 21:20:50]

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, 2017-01-13 21:43:04]

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

[commit-bot: I haz the power, 2017-01-13 21:43:47]

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, 2017-01-13 22:27:16]

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

[commit-bot: I haz the power, 2017-01-13 22:27:18]

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, 2017-01-17 22:11:16]

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

[commit-bot: I haz the power, 2017-01-17 22:11: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, 2017-01-18 00:13:19]

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

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

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

[mmenke, 2017-01-18 20:11:12]

Randy:  Should I toss this to Charles?  Just thinking that since you'll be out
soon, and I want to land this before I'm out, may be simplest for all involved. 
(There's also yet one more preliminary CL I'm working on, and hope to get out
for review this week, though it should hopefully result in no changes to this
CL, or at most minimal ones).

[Randy Smith (Not in Mondays), 2017-01-18 20:14:06]

On 2017/01/18 20:11:12, mmenke wrote:
> Randy:  Should I toss this to Charles?  Just thinking that since you'll be out
> soon, and I want to land this before I'm out, may be simplest for all
involved. 
> (There's also yet one more preliminary CL I'm working on, and hope to get out
> for review this week, though it should hopefully result in no changes to this
> CL, or at most minimal ones).

Probably wise :-{.  I'm bogged down at the moment reviewing the cache lock CL,
and there are things I really should do while I'm in CA to take advantage of the
fact that I'm in CA.  I'll try to do another review of this before the end of
the week anyway.  So yes, if you want to get this in this month, Charles should
be a reviewer and you shouldn't block on me.  

Sorry :-J.

[mmenke, 2017-01-18 20:15:54]

On 2017/01/18 20:14:06, Randy Smith - Not in Mondays wrote:
> On 2017/01/18 20:11:12, mmenke wrote:
> > Randy:  Should I toss this to Charles?  Just thinking that since you'll be
out
> > soon, and I want to land this before I'm out, may be simplest for all
> involved. 
> > (There's also yet one more preliminary CL I'm working on, and hope to get
out
> > for review this week, though it should hopefully result in no changes to
this
> > CL, or at most minimal ones).
> 
> Probably wise :-{.  I'm bogged down at the moment reviewing the cache lock CL,
> and there are things I really should do while I'm in CA to take advantage of
the
> fact that I'm in CA.  I'll try to do another review of this before the end of
> the week anyway.  So yes, if you want to get this in this month, Charles
should
> be a reviewer and you shouldn't block on me.  
> 
> Sorry :-J.

No problem, it happens, and I certainly understand both being bogged down by
reviews, and putting off big reviews (Particularly those that depend on other
CLs that aren't even out for review yet).

[mmenke, 2017-01-18 20:16:20]

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

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

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, 2017-01-18 22:10:25]

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

[commit-bot: I haz the power, 2017-01-18 22:10:27]

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

[mmenke, 2017-01-19 17:29:22]

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

[commit-bot: I haz the power, 2017-01-19 17:29:59]

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

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

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

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

[mmenke, 2017-01-23 23:15:16]

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

[mmenke, 2017-01-23 23:15:18]

[+csharrison]:  Mind taking a look, now that Randy's out?  I had been hoping to
land this before I leave for vacation, but given its size, I'm not sure how
likely that is.

I still want to land one more CL before this, which will add a bunch of
RedirectToFile unittests, so that code is subject to change, if those tests
detect any issues, or some reworking of things make it more friendly for those
tests.

The place to start reviewing is ResourceHandler/ResourceController and then
ResourceLoader.  Changes to other files can be largely considered in isolation
(Given the ResourceHandler changes).

https://codereview.chromium.org/2526983002/diff/520001/content/browser/loader...
File content/browser/loader/null_resource_controller.h (right):

https://codereview.chromium.org/2526983002/diff/520001/content/browser/loader...
content/browser/loader/null_resource_controller.h:13: class
NullResourceController : public ResourceController {
I really want to get rid of this class in the future.

[mmenke, 2017-01-23 23:18:06]

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

[commit-bot: I haz the power, 2017-01-23 23:18:57]

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, 2017-01-23 23:23:02]

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

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

Dry run: 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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-01-23 23:23:59]

Sure, I can look at this tomorrow.

[mmenke, 2017-01-24 18:25:33]

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

[commit-bot: I haz the power, 2017-01-24 18:25:52]

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, 2017-01-24 19:50:24]

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

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

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-24 23:27:53]

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

[mmenke, 2017-01-24 23:28:31]

Patchset #29 (id:560001) has been deleted

[commit-bot: I haz the power, 2017-01-24 23:29:39]

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, 2017-01-24 23:36:06]

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

[commit-bot: I haz the power, 2017-01-24 23:36:08]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-25 19:32:46]

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

[commit-bot: I haz the power, 2017-01-25 19:33:20]

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

[Charlie Harrison, 2017-01-25 20:23:00]

This looks great. I read through some of the old comments, and I support just
landing this and worrying about beautifying the API later.

When do you expect to fix the insufficient resources bug this introduces?

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
content/browser/download/download_resource_handler.cc:192: if (!defer) {
optional: invert conditions so the positive one is first.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
File content/browser/download/save_file_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
content/browser/download/save_file_resource_handler.h:56:
std::unique_ptr<ResourceController> controller) override;
nit: fwd declare ResourceController.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/DEPS:231:
"+content/browser/loader/resource_dispatcher_host_impl.h",
How is this a DEP?

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:282: if (!filter->Send(new
ResourceMsg_ReceivedRedirect(
optional: would prefer reversed conditional.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:287:
HoldController(std::move(controller));
optional: I think OnDefer should call HoldController. Feel free to disagree.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:115: // Resume.
nit: remove redundant comment.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:224:
OutOfBandCancel(net::ERR_ABORTED, true);
would appreciate a /* tell_renderer */ here.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:51: bool used_ = false;
I hate to recommend it but since this member is only used for DCHECKs I'd wrap
it in #if DCHECK_IS_ON(). Same for your other controllers. You could write a
method:

void CheckUse() {
#if DCHECK_IS_ON()
  DCHECK(!used_);
  used_ = true;
#endif
}

The empty method will probably be elided.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:174: // If called
recusively from a DoLoop, advance state when return to the loop.
nit: s/return/returning

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:200: in_do_loop_ = true;
optional: use base::AutoReset?

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:57: void
ResumeInternal();
Not part of ResourceHandler implementation, add newline above.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:148: // True if the code
is currently withing a DoLoop.
s/withing/within

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:69: class
MimeSniffingResourceHandler::Controller : public ResourceController {
This looks just like a couple of other controllers, I would support merging
logic. The only issue is "ResumeInternal" though. Still, even with a base class
that implements everything else but makes Resume() virtual would be good.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:286: NOTREACHED();
Unnecessary NOTREACHED() with the DCHECK above. It is against chromium style to
need the early return for this "unreachable" code so I would also like that
removed (or turn the DCHECK into a CHECK if we aren't sure).

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.h:180: // returning to the
parent advance state loop, will synchronously advance to
s/advance state/AdvanceState for consistency.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:142: if
(!info->is_stream() && !info->IsDownload()) {
nit: Invert the if statement:

if (info->is_stream() || info->IsDownload())
  controller->Resume()
else
  HoldController(std::move(controller))

Makes a lot more sense (resume streams / downloads, hold everything else).

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/null_resource_controller.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

happy new year!

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/null_resource_controller.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:13: class
NullResourceController : public ResourceController {
A class comment would be nice.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:17: 
// ResourceController: 
or 
// ResourceController implementation:

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/redirect_to_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/redirect_to_file_resource_handler.cc:227: if
(!WriteMore()) {
Can you comment that WriteMore will resume the request if there is more data
/buffer space?

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.cc:33: // |controller_| is null or not.
Are you sure? I thought move semantics guaranteed that comparison with nullptr
would work after moving.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.h:51: class CONTENT_EXPORT Delegate {
A class comment describing the delegate would be nice (essentially, to allow for
OOB cancel). Optionally, I would also support renaming this from the generic
"Delegate" to something a bit more specific.
"ResourceHandlerOutOfBandCanceller"? Might be too far, but my general point is
that scanning through a class that implements ResourceHandler::Delegate doesn't
say much without clicking through.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.h:154: // oassed to HoldController and
then destroy it.
s/oassed/passed

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:140: ~Controller() override {}
Can we DCHECK(done_) here? I'm nervous about people forgetting to resume /
cancel.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:178: // 1) If the ResourceLoader has a
deferred stage of DEFERRED_NONE, sets the
Can the ResourceLoader have deferred stage apart from NONE or SYNC? Of not,
let's DCHECK it.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:347: if (!is_deferred()) {
optional: I don't think early-return is a more readable pattern here. I would
prefer:

if (is_deferred()) {
  *defer = true;
  deferred_redirect_url_ = redirect_info.new_url;
  deferred_stage_ = DEFERRED_REDIRECT;
} else {
  *defer = false;
  if (delegate->HandleExternalProtocol(...))
    CancelAndIgnore();
}

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:494: // Starting requests completes
asynchronously, so starting the request now
nit: "so start the request now"? The sentence confuses me.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:501: if
(!called_from_resource_controller) {
nit: I would prefer the conditional to be reversed, with the positive case
first, same for all these cases.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:661: OnReadCompleted(request_.get(),
result);
Isn't it breaking the contract to call OnReadCompleted if
called_from_resource_controller is true? If that contract does not apply to
ReadMore, it should be specified in the header.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.h:93: void Resume(bool
called_from_resource_controller);
Personal preference: I really dislike bare bools passed into methods like
Resume(true). What is true? I would prefer that callsites do
Resume(true /* called_from_resource_controller */), or use an enum.

Ofc this is just personal preference, so feel free to disagree.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/throttling_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/throttling_resource_handler.cc:121:
CancelWithError(net::ERR_ABORTED);
I know ReleaseController() already DCHECKs but a DCHECK(has_controller()) would
still improve readability.

[mmenke, 2017-01-25 20:33:00]

On 2017/01/25 20:23:00, Charlie Harrison wrote:
> This looks great. I read through some of the old comments, and I support just
> landing this and worrying about beautifying the API later.
> 
> When do you expect to fix the insufficient resources bug this introduces?

I've been thinking I'd work on it after the sync destruction on OOB cancel CL,
which I hope to get out next week (so probably not until after I get back after
the break).

[Charlie Harrison, 2017-01-25 20:37:33]

On 2017/01/25 20:33:00, mmenke wrote:
> On 2017/01/25 20:23:00, Charlie Harrison wrote:
> > This looks great. I read through some of the old comments, and I support
just
> > landing this and worrying about beautifying the API later.
> > 
> > When do you expect to fix the insufficient resources bug this introduces?
> 
> I've been thinking I'd work on it after the sync destruction on OOB cancel CL,
> which I hope to get out next week (so probably not until after I get back
after
> the break).

How do you expect metrics to shift as a result if the fix does not make it to
M58 branch cut? Worth mentioning to net-dev if it happens.

[Charlie Harrison, 2017-01-25 20:41:40]

Oh, and would you mind writing up a CL description for this change?

[mmenke, 2017-01-25 20:49:30]

On 2017/01/25 20:37:33, Charlie Harrison wrote:
> On 2017/01/25 20:33:00, mmenke wrote:
> > On 2017/01/25 20:23:00, Charlie Harrison wrote:
> > > This looks great. I read through some of the old comments, and I support
> just
> > > landing this and worrying about beautifying the API later.
> > > 
> > > When do you expect to fix the insufficient resources bug this introduces?
> > 
> > I've been thinking I'd work on it after the sync destruction on OOB cancel
CL,
> > which I hope to get out next week (so probably not until after I get back
> after
> > the break).
> 
> How do you expect metrics to shift as a result if the fix does not make it to
> M58 branch cut? Worth mentioning to net-dev if it happens.

I suspect no significant differences, in aggregate.  Of course,
ERR_INSUFFICIENT_RESOURCES is sufficiently rare and obscure this may be a
significant contributor to the few we do see.

[mmenke, 2017-01-25 20:53:33]

Description was changed from

==========
Rework ResourceHandler chain.

BUG=659317
==========

to

==========
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
==========

[mmenke, 2017-01-25 20:53:48]

On 2017/01/25 20:41:40, Charlie Harrison wrote:
> Oh, and would you mind writing up a CL description for this change?

Done.

[commit-bot: I haz the power, 2017-01-25 21:50:40]

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

[commit-bot: I haz the power, 2017-01-25 21:50:41]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-25 22:08:00]

Thanks for all the great feedback!  I'm impressed you got through it so quickly!

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
File content/browser/download/download_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
content/browser/download/download_resource_handler.cc:192: if (!defer) {
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> optional: invert conditions so the positive one is first.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
File content/browser/download/save_file_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/downlo...
content/browser/download/save_file_resource_handler.h:56:
std::unique_ptr<ResourceController> controller) override;
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> nit: fwd declare ResourceController.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/DEPS:231:
"+content/browser/loader/resource_dispatcher_host_impl.h",
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> How is this a DEP?

Done.  (An earlier draft had OOB removal going through it, instead of the
ResourceHandler::Delegate approach I use now).

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:282: if (!filter->Send(new
ResourceMsg_ReceivedRedirect(
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> optional: would prefer reversed conditional.

Done.  Interesting, my preference is for error cases first, since they generally
require less code (And more easily switch to early returns, instead of
else/ifs).  Mostly agree with your ordering comments, but not with all of them.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:287:
HoldController(std::move(controller));
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> optional: I think OnDefer should call HoldController. Feel free to disagree.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:115: // Resume.
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> nit: remove redundant comment.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:224:
OutOfBandCancel(net::ERR_ABORTED, true);
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> would appreciate a /* tell_renderer */ here.

Done, good idea.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:51: bool used_ = false;
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> I hate to recommend it but since this member is only used for DCHECKs I'd wrap
> it in #if DCHECK_IS_ON(). Same for your other controllers. You could write a
> method:
> 
> void CheckUse() {
> #if DCHECK_IS_ON()
>   DCHECK(!used_);
>   used_ = true;
> #endif
> }
> 
> The empty method will probably be elided.

Done, but it really seems like overkill to me.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:174: // If called
recusively from a DoLoop, advance state when return to the loop.
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> nit: s/return/returning

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:200: in_do_loop_ = true;
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> optional: use base::AutoReset?

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:57: void
ResumeInternal();
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> Not part of ResourceHandler implementation, add newline above.

There is already one, actually.  I went ahead and made this private, though (Not
sure why I didn't make it private in the first place)

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:148: // True if the code
is currently withing a DoLoop.
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> s/withing/within

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:69: class
MimeSniffingResourceHandler::Controller : public ResourceController {
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> This looks just like a couple of other controllers, I would support merging
> logic. The only issue is "ResumeInternal" though. Still, even with a base
class
> that implements everything else but makes Resume() virtual would be good. 

I think there are only two classes like this?  ResourceLoader's is kinda like
this, but it's not a ResourceHandler, and having a class implement two
interfaces with the same method names is just not good.  I'd have to make 4
virtual methods in the parent class, not one.  Given that, I'm not sure it's
worth it...Particularly as it's not a pattern I want to encourage, and the
InterceptingResourceHandler should ideally go away, some day, assuming all the
Mojo stuff goes as planned.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:286: NOTREACHED();
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> Unnecessary NOTREACHED() with the DCHECK above. It is against chromium style
to
> need the early return for this "unreachable" code so I would also like that
> removed (or turn the DCHECK into a CHECK if we aren't sure).

I left this as it is mostly because of the sheer size of this CL.  If we want to
clean up the state machine, which I'm fine with, I think we want to do it in
another CL.  I really don't want want to have to re-land this CL.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.h:180: // returning to the
parent advance state loop, will synchronously advance to
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> s/advance state/AdvanceState for consistency.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/navigation_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/navigation_resource_handler.cc:142: if
(!info->is_stream() && !info->IsDownload()) {
On 2017/01/25 20:22:59, Charlie Harrison wrote:
> nit: Invert the if statement:
> 
> if (info->is_stream() || info->IsDownload())
>   controller->Resume()
> else
>   HoldController(std::move(controller))
> 
> Makes a lot more sense (resume streams / downloads, hold everything else).

Done.  Interesting, I think of Resume() as more of the success case / continue
doing stuff case, and HoldController as more of the pseudo-error case, so think
the opposite order makes more sense (Though x || y is much clearer than !x &&
!y)

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/null_resource_controller.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> 2017

Done, though not entirely sure what's right - I did write this file in 2016.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:13: class
NullResourceController : public ResourceController {
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> A class comment would be nice.

Done, though I'm not sure this class is worthy of a comment.  :)

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/null_resource_controller.h:17: 
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> // ResourceController: 
> or 
> // ResourceController implementation:

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/redirect_to_file_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/redirect_to_file_resource_handler.cc:227: if
(!WriteMore()) {
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Can you comment that WriteMore will resume the request if there is more data
> /buffer space?

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.cc:33: // |controller_| is null or not.
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Are you sure? I thought move semantics guaranteed that comparison with nullptr
> would work after moving.

You're right...It's not guaranteed be std::move in general, but it is guaranteed
for std::move(unique_ptr).

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.h:51: class CONTENT_EXPORT Delegate {
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> A class comment describing the delegate would be nice (essentially, to allow
for
> OOB cancel). Optionally, I would also support renaming this from the generic
> "Delegate" to something a bit more specific.
> "ResourceHandlerOutOfBandCanceller"? Might be too far, but my general point is
> that scanning through a class that implements ResourceHandler::Delegate
doesn't
> say much without clicking through.

Done.  Just added a comment.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_handler.h:154: // oassed to HoldController and
then destroy it.
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> s/oassed/passed

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:140: ~Controller() override {}
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Can we DCHECK(done_) here? I'm nervous about people forgetting to resume /
> cancel.

Unfortunately not, due to out-of-band cancels.  :(

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:178: // 1) If the ResourceLoader has a
deferred stage of DEFERRED_NONE, sets the
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Can the ResourceLoader have deferred stage apart from NONE or SYNC? Of not,
> let's DCHECK it.

Good idea, done.  Wonder if it would be cleaner to switch this class to a do
loop and have it work like the others (At some point, not in this CL) - the
ScopedDeferral stuff is a bit unintuitive.  :(

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:347: if (!is_deferred()) {
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> optional: I don't think early-return is a more readable pattern here. I would
> prefer:
> 
> if (is_deferred()) {
>   *defer = true;
>   deferred_redirect_url_ = redirect_info.new_url;
>   deferred_stage_ = DEFERRED_REDIRECT;
> } else {
>   *defer = false;
>   if (delegate->HandleExternalProtocol(...))
>     CancelAndIgnore();
> }

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:494: // Starting requests completes
asynchronously, so starting the request now
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> nit: "so start the request now"? The sentence confuses me.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:501: if
(!called_from_resource_controller) {
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> nit: I would prefer the conditional to be reversed, with the positive case
> first, same for all these cases.

Done.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:661: OnReadCompleted(request_.get(),
result);
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Isn't it breaking the contract to call OnReadCompleted if
> called_from_resource_controller is true? If that contract does not apply to
> ReadMore, it should be specified in the header.

Great catch!  I was keeping old behavior here, but you're right.  I don't think
it should be fixed in this CL, though.  CL is too big already, and it needs
tests.

I think we could land this CL without fixing this first, because I don't think
this CL makes things any more likely to crash if we violate that part of the
contract, but it's probably best to land the first first, just in case there's
some implicit assumption I'm unaware of.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.h:93: void Resume(bool
called_from_resource_controller);
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> Personal preference: I really dislike bare bools passed into methods like
> Resume(true). What is true? I would prefer that callsites do
> Resume(true /* called_from_resource_controller */), or use an enum.
> 
> Ofc this is just personal preference, so feel free to disagree.

I think this is reasonable.  I went with labeling callsites.  (An enum is
clearer, but ResumeType::CalledFromResourceController seems to wordy, and plain
enums are icky)

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/throttling_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/throttling_resource_handler.cc:121:
CancelWithError(net::ERR_ABORTED);
On 2017/01/25 20:23:00, Charlie Harrison wrote:
> I know ReleaseController() already DCHECKs but a DCHECK(has_controller())
would
> still improve readability.

This actually calls ThrottlingResourceHAndler::CancelWithError, so the DCHECK
would be incorrect.  I agree that this is very confusing.  I went ahead and
duplicated the code CancelWithError instead.

[mmenke, 2017-01-25 22:38:04]

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:661: OnReadCompleted(request_.get(),
result);
On 2017/01/25 22:07:59, mmenke wrote:
> On 2017/01/25 20:23:00, Charlie Harrison wrote:
> > Isn't it breaking the contract to call OnReadCompleted if
> > called_from_resource_controller is true? If that contract does not apply to
> > ReadMore, it should be specified in the header.
> 
> Great catch!  I was keeping old behavior here, but you're right.  I don't
think
> it should be fixed in this CL, though.  CL is too big already, and it needs
> tests.
> 
> I think we could land this CL without fixing this first, because I don't think
> this CL makes things any more likely to crash if we violate that part of the
> contract, but it's probably best to land the first first, just in case there's
> some implicit assumption I'm unaware of.

One other thing I noticed - the old code mentions explicitly that it also wants 
to limit the number of reads we do to 1 before we do a posttask.  This code
doesn't do that.  I'll also look into fixing that.

[Charlie Harrison, 2017-01-26 00:25:51]

I'll go over it again tomorrow.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:69: class
MimeSniffingResourceHandler::Controller : public ResourceController {
On 2017/01/25 22:07:59, mmenke wrote:
> On 2017/01/25 20:22:59, Charlie Harrison wrote:
> > This looks just like a couple of other controllers, I would support merging
> > logic. The only issue is "ResumeInternal" though. Still, even with a base
> class
> > that implements everything else but makes Resume() virtual would be good. 
> 
> I think there are only two classes like this?  ResourceLoader's is kinda like
> this, but it's not a ResourceHandler, and having a class implement two
> interfaces with the same method names is just not good.  I'd have to make 4
> virtual methods in the parent class, not one.  Given that, I'm not sure it's
> worth it...Particularly as it's not a pattern I want to encourage, and the
> InterceptingResourceHandler should ideally go away, some day, assuming all the
> Mojo stuff goes as planned.

Yeah, I probably just looked at this one 5 times and misattributed. SGTM

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:286: NOTREACHED();
On 2017/01/25 22:07:59, mmenke wrote:
> On 2017/01/25 20:22:59, Charlie Harrison wrote:
> > Unnecessary NOTREACHED() with the DCHECK above. It is against chromium style
> to
> > need the early return for this "unreachable" code so I would also like that
> > removed (or turn the DCHECK into a CHECK if we aren't sure).
> 
> I left this as it is mostly because of the sheer size of this CL.  If we want
to
> clean up the state machine, which I'm fine with, I think we want to do it in
> another CL.  I really don't want want to have to re-land this CL.

Acknowledged.

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:661: OnReadCompleted(request_.get(),
result);
On 2017/01/25 22:07:59, mmenke wrote:
> On 2017/01/25 20:23:00, Charlie Harrison wrote:
> > Isn't it breaking the contract to call OnReadCompleted if
> > called_from_resource_controller is true? If that contract does not apply to
> > ReadMore, it should be specified in the header.
> 
> Great catch!  I was keeping old behavior here, but you're right.  I don't
think
> it should be fixed in this CL, though.  CL is too big already, and it needs
> tests.
> 
> I think we could land this CL without fixing this first, because I don't think
> this CL makes things any more likely to crash if we violate that part of the
> contract, but it's probably best to land the first first, just in case there's
> some implicit assumption I'm unaware of.

Acknowledged

[mmenke, 2017-01-27 19:14:12]

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

[mmenke, 2017-01-27 19:14:29]

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/540001/content/browser/loader...
content/browser/loader/resource_loader.cc:661: OnReadCompleted(request_.get(),
result);
On 2017/01/26 00:25:50, Charlie Harrison wrote:
> On 2017/01/25 22:07:59, mmenke wrote:
> > On 2017/01/25 20:23:00, Charlie Harrison wrote:
> > > Isn't it breaking the contract to call OnReadCompleted if
> > > called_from_resource_controller is true? If that contract does not apply
to
> > > ReadMore, it should be specified in the header.
> > 
> > Great catch!  I was keeping old behavior here, but you're right.  I don't
> think
> > it should be fixed in this CL, though.  CL is too big already, and it needs
> > tests.
> > 
> > I think we could land this CL without fixing this first, because I don't
think
> > this CL makes things any more likely to crash if we violate that part of the
> > contract, but it's probably best to land the first first, just in case
there's
> > some implicit assumption I'm unaware of.
> 
> Acknowledged

Ok, I looked into this:  The reason why I couldn't repro is that this wasn't an
issue before this patch, and with this patch, called_from_resource_controller is
always false, anyways, because of the PostTask in Resume().

I've removed the result <= 0 check, gotten rid of that PostTask, and now call
ReadMore with true in Resume, unconditionally, to match (I think?) old behavior,
which was designed to avoid getting stuck in a read loop.

We did have a test that caught this, once I made this an actual bug -
ResourceLoaderTest.AsyncResourceHandler, so I'm not going to add any tests.

I also modified the ResourceHandler API to allow re-entrant calls to OnWillRead
(Which that test also caught), and modified TestResourceHandler not to catch
them.

Sorry for the merge + fixes in the same CL.  Only the resource_loader.cc,
mock_resource_loader.cc, and resource_handler.h changes are relevant (Only merge
conflicts were with redirect_to_file_resource_handler).

[commit-bot: I haz the power, 2017-01-27 19:14:40]

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, 2017-01-27 20:04:58]

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

[commit-bot: I haz the power, 2017-01-27 20:05:01]

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, 2017-01-27 21:25:35]

Ok, the result >= 0 was actually correct, I'd just misunderstood the old code,
so confused my variable names.:

On sync ResourceHandler success (For sync/async reads), we'd call
ReadMore(true), which would start the next read immediately, and PostTask on
success unless we ran into the end of the stream or error, which would be
handled synchronously.

On async ResourceHandler success, we'd always post a task to call
ReadMore(false).  So ReadMore actually should be the opposite of
called_from_resource_controller.  The earlier code was right, in terms of
behaving the same as before, the variable name was just messed up.

We may want to make behavior more sane, but in the interest of not regressing
performance in this CL, I've returned to the earlier behavior.

Tests should all be passing now.

[mmenke, 2017-01-27 21:25:40]

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

[commit-bot: I haz the power, 2017-01-27 21:26:32]

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

[mmenke, 2017-01-27 21:26:33]

On 2017/01/27 21:25:35, mmenke (Out Feb 4 to March 5) wrote:
> Ok, the result >= 0 was actually correct, I'd just misunderstood the old code,
> so confused my variable names.:

result <= 0, rather.

> On sync ResourceHandler success (For sync/async reads), we'd call
> ReadMore(true), which would start the next read immediately, and PostTask on
> success unless we ran into the end of the stream or error, which would be
> handled synchronously.
> 
> On async ResourceHandler success, we'd always post a task to call
> ReadMore(false).  So ReadMore actually should be the opposite of
> called_from_resource_controller.  The earlier code was right, in terms of
> behaving the same as before, the variable name was just messed up.
> 
> We may want to make behavior more sane, but in the interest of not regressing
> performance in this CL, I've returned to the earlier behavior.
> 
> Tests should all be passing now.

[commit-bot: I haz the power, 2017-01-27 22:36:01]

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

[commit-bot: I haz the power, 2017-01-27 22:36:02]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-27 22:48:05]

LG except I am confused by the new code.

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:37: void Resume() override
{ detachable_handler_->Resume(); }
Can e use the MarkAsUsed pattern here?

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:208: ResumeInternal();
just move one ResumeInternal to the bottom of this case?

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/resource_handler.h:71: // Sets the
ResourceHandler::Delegate, which handles out-of-bounds
out of band?

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/resource_loader.cc:201: old_deferred_stage ==
DEFERRED_SYNC);
nit: DCHECK(...) << old_deferred_stage so if this fails we know what the stage
is.

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:525: // aboid blocking the IO thread.
avoid

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:680: if (handle_result_asyncronously
|| result <= 0) {
Do you mean !handle_result_asynchronously? This looks wrong.

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:701: ReadMore(false);  // Read the
next chunk (OK to complete synchronously).
Remove "(Ok to complete synchronously)" and add /* handle_result_asyncronously
*/ inline

[mmenke, 2017-01-27 23:02:15]

On 2017/01/27 22:48:05, Charlie Harrison wrote:
> LG except I am confused by the new code.

Anything I can help with?  Basically I tried to match the old pattern of when we
post task and when we don't (ResourceLoader tests fail with extra post tasks,
and adding them has a higher chance of adding a perf regression).


Basically if Resume() is called synchronously on read, when we work our way back
up the callstack, we need to read synchronously and only call into the next
handler synchronously on completion.

If Resume() is called asynchronously, we post a task to do the next read (And if
that read completes synchronously, we call into the ResourceHandler
synchronously regardless).


Could imagine the the latter case reading synchronously but calling into the
Handler asynchronously on sync read success, but decided that was too
complicated for now, and just to keep the pattern the same.

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:37: void Resume() override
{ detachable_handler_->Resume(); }
On 2017/01/27 22:48:04, Charlie Harrison wrote:
> Can e use the MarkAsUsed pattern here?

Done.

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:208: ResumeInternal();
On 2017/01/27 22:48:04, Charlie Harrison wrote:
> just move one ResumeInternal to the bottom of this case?

Done.

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/resource_handler.h:71: // Sets the
ResourceHandler::Delegate, which handles out-of-bounds
On 2017/01/27 22:48:04, Charlie Harrison wrote:
> out of band?

Done.  Though worth noting that if it is cancelled out of bounds, FireFox gets a
free throw.

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/600001/content/browser/loader...
content/browser/loader/resource_loader.cc:201: old_deferred_stage ==
DEFERRED_SYNC);
On 2017/01/27 22:48:04, Charlie Harrison wrote:
> nit: DCHECK(...) << old_deferred_stage so if this fails we know what the stage
> is.

Done.

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:525: // aboid blocking the IO thread.
On 2017/01/27 22:48:05, Charlie Harrison wrote:
> avoid

Done.

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:680: if (handle_result_asyncronously
|| result <= 0) {
On 2017/01/27 22:48:05, Charlie Harrison wrote:
> Do you mean !handle_result_asynchronously? This looks wrong.

Yes I do...

Worth noting this wasn't caught by tests (Because of how I changed things). 
Should probably write a new test for that.

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:701: ReadMore(false);  // Read the
next chunk (OK to complete synchronously).
On 2017/01/27 22:48:05, Charlie Harrison wrote:
> Remove "(Ok to complete synchronously)" and add /* handle_result_asyncronously
> */ inline

Done.

[mmenke, 2017-01-27 23:02:19]

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

[commit-bot: I haz the power, 2017-01-27 23:03:41]

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

[mmenke, 2017-01-27 23:05:10]

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/660001/content/browser/loader...
content/browser/loader/resource_loader.cc:680: if (handle_result_asyncronously
|| result <= 0) {
On 2017/01/27 23:02:15, mmenke (Out Feb 4 to March 5) wrote:
> On 2017/01/27 22:48:05, Charlie Harrison wrote:
> > Do you mean !handle_result_asynchronously? This looks wrong.
> 
> Yes I do...
> 
> Worth noting this wasn't caught by tests (Because of how I changed things). 
> Should probably write a new test for that.

Actually, this can't be tested - it's just to avoid blocking the message loop
while reading a file.  So I'd have to test how many times we re-entered the
message loop...ok, maybe testable, but such a test seems destined to be flaky
and weird.  WDYT?

[Charlie Harrison, 2017-01-27 23:15:27]

Yeah, the main thing confusing me was handle_asynchronously seemed reversed. One
nit and then LGTM. Let's make sure to watch for crashes.

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
content/browser/loader/resource_loader.cc:650: void
ResourceLoader::ReadMore(bool handle_result_asyncronously) {
nit: s/asyncronously/asynchronously and ditto below. Took me way too long to see
why that word looked weird to me.

[Charlie Harrison, 2017-01-27 23:18:11]

I think skipping out message loop spinning tests is okay for now. I don't want
to give up forever if we come up with a good solution though.

[commit-bot: I haz the power, 2017-01-28 00:41:33]

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

[commit-bot: I haz the power, 2017-01-28 00:41:35]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-28 04:49:35]

Thanks for the review!  I think I may land this Sunday, to give it an extra day
to percolate.

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
content/browser/loader/resource_loader.cc:650: void
ResourceLoader::ReadMore(bool handle_result_asyncronously) {
On 2017/01/27 23:15:27, Charlie Harrison wrote:
> nit: s/asyncronously/asynchronously and ditto below. Took me way too long to
see
> why that word looked weird to me.

Think I'll go with async.  :)

[mmenke, 2017-01-30 03:22:28]

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

[mmenke, 2017-01-30 03:22:30]

[+asanka]:  Please review content/browser/download.  The basic plan is to
separate out cancels from ResourceHandlers whose turn it is not currently, and
eventually delete the ResourceHandler chain synchronusly in those cases (As,
otherwise, ResourceHandlers whose turn it is can continue doing stuff, as the
request is asynchronously torn down - and teardown can even be deferred by
another ResourceHandler).  Also want to better protect against out-of-turn
resumes as well.

[mmenke, 2017-01-30 03:25:16]

On 2017/01/30 03:22:30, mmenke (Out Feb 4 to March 5) wrote:
> [+asanka]:  Please review content/browser/download.  The basic plan is to
> separate out cancels from ResourceHandlers whose turn it is not currently, and
> eventually delete the ResourceHandler chain synchronusly in those cases (As,
> otherwise, ResourceHandlers whose turn it is can continue doing stuff, as the
> request is asynchronously torn down - and teardown can even be deferred by
> another ResourceHandler).  Also want to better protect against out-of-turn
> resumes as well.

And a bit more elaboration:  Instead of passing around a defer bool, returning
an error on cancel, calling Resume/Cancel after making a decision (Sometimes
synchronously, sometimes asychronously), and calling cancel randomly out of
turn, the new API passes in a single-use ResourceController, which can be used
synchronously or asynchronously to resume or cancel the request.  Out of turn
cancels have to go through ResourceHandler::CancelOutOfBand.

[asanka, 2017-01-30 22:20:49]

/download/ lgtm

I only glanced through the rest of this CL.

Chatted with mmenke OOB and my understanding is that:

* We'll move towards a single cancellation pathway and it will be synchronous,
at which point Cancel() will move out of controller and exclusively into
Delegate.

* Controller::Resume() exists because implicit resumption on destruction of the
Controller is undesirable. I'm still not sold on this because "do your stuff and
don't block the request" should be the normal mode of operation for
ResourceHandler methods. This would also make no-op handlers have well defined
behavior. But perhaps not for this CL.

  To that end, I'd suggest DCHECKing on destruction that either
Resume()/Cancel*() was invoked on the Controller. It seems that's not the case
currently.

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:160:
next_handler_->OnResponseCompleted(
You probably want to swap out the delegate for the RH chain starting at
next_handler_. Otherwise any OOB cancellations would choke this call stack.

[mmenke, 2017-01-30 22:36:02]

On 2017/01/30 22:20:49, asanka wrote:
> /download/ lgtm
> 
> I only glanced through the rest of this CL.
> 
> Chatted with mmenke OOB and my understanding is that:
> 
> * We'll move towards a single cancellation pathway and it will be synchronous,
> at which point Cancel() will move out of controller and exclusively into
> Delegate.

I'd like to do this, and think it would make things much clearer though not sure
if/when we'll get to it.  The fact is, with mojo, the longer we put off this
stuff, the easier it should become, since Mojo will hopefully mean we have fewer
ResourceHandlers.

> * Controller::Resume() exists because implicit resumption on destruction of
the
> Controller is undesirable. I'm still not sold on this because "do your stuff
and
> don't block the request" should be the normal mode of operation for
> ResourceHandler methods. This would also make no-op handlers have well defined
> behavior. But perhaps not for this CL.
> 
>   To that end, I'd suggest DCHECKing on destruction that either
> Resume()/Cancel*() was invoked on the Controller. It seems that's not the case
> currently.

OOB cancels mean that they can be deleted without resuming (Which is another
issue with resuming on destruction), of course, we could have a ResourceLoader
set a bool on teardown to prevent a DCHECK.  Anyhow, we can't do this yet, but
it may be a good idea to get to the point where we can.

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2526983002/diff/680001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:160:
next_handler_->OnResponseCompleted(
On 2017/01/30 22:20:49, asanka wrote:
> You probably want to swap out the delegate for the RH chain starting at
> next_handler_. Otherwise any OOB cancellations would choke this call stack.

Note that |next_handler_| is destroyed 4 lines down, and in-band out-of-band
cancellation isn't allowed.  I'm reluctant to swap the RH::Delegate because
doing that is currently allowed, and I don't want to introduce it just for this
(If I swap it with anything, I'd actually swap it with nullptr, so we crash,
since cancel here is currently not handled - and cancel on OnResponseCompleted
is more generally not allowed, per the ResourceHandler API).

I'd actually like to get rid of NullResourceController entirely, just working
around it in the places that use it was too complicated for this already huge
CL.  I already have a TODO in one place to get rid of it, I've added them to all
callsites now.

I'd like to add a DCHECK that we don't OOB cancel synchronously, but
sufficiently nervous about this CL that I'm not going to add it now.

[mmenke, 2017-01-30 22:36:27]

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

[commit-bot: I haz the power, 2017-01-30 22:36:57]

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

[mmenke, 2017-01-30 22:38:37]

I'll plan on landing this tonight / tomorrow after a full CQ run (Including
layout tests).

[commit-bot: I haz the power, 2017-01-30 23:54:37]

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

[commit-bot: I haz the power, 2017-01-30 23:54:38]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-31 15:12:07]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-31 15:12:08]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2526983002/#ps700001
(title: "Response to comments")

[commit-bot: I haz the power, 2017-01-31 15:12:21]

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

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

[commit-bot: I haz the power, 2017-01-31 15:20:11]

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

[mmenke, 2017-01-31 15:26:20]

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

[mmenke, 2017-01-31 15:26:22]

[+jam]:  Please review content/browser/BUILD.gn.

There are a bunch of cases where we have two messages wandering through the
ResourceLoader stack at once, due to deferred out-of-band cancellation and
deferred OnResponseComplete, and we also have nothing to catch out-of-order
resumes.  This is a step towards eliminating those problems.  Want to eliminate
NullResourceController eventually, but not reasonable in this already huge CL.

[jam, 2017-01-31 16:04:55]

lgtm

[mmenke, 2017-01-31 16:06:09]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-01-31 16:06:26]

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-31 16:11:30]

CQ is committing da patch.

Bot data: {"patchset_id": 700001, "attempt_start_ts": 1485878769692980,
"parent_rev": "8422bf10bc4af33a0db2fde5117792ecde362863", "commit_rev":
"87f5c77aaa3541bee2f8d3b6b64d3879b8e81957"}

[commit-bot: I haz the power, 2017-01-31 16:12:03]

Description was changed from

==========
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
==========

to

==========
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}
Committed:
https://chromium.googlesource.com/chromium/src/+/87f5c77aaa3541bee2f8d3b6b64d...
==========

[commit-bot: I haz the power, 2017-01-31 16:12:06]

Committed patchset #35 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/87f5c77aaa3541bee2f8d3b6b64d...