Update MimeSniffingResourceHandler tests to use MockResourceLoader.

Update MimeSniffingResourceHandler tests to use MockResourceLoader.

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

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

BUG=659317
Review-Url: https://codereview.chromium.org/2626663002
Cr-Commit-Position: refs/heads/master@{#443360}
Committed: https://chromium.googlesource.com/chromium/src/+/2522425b429d16cfdc97f11e4bbb4ac28d1a20fd

[mmenke, 2017-01-10 17:22:52]

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

[commit-bot: I haz the power, 2017-01-10 17:23:16]

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-10 17:55:03]

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

[commit-bot: I haz the power, 2017-01-10 17:55:04]

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-10 18:34:24]

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

[commit-bot: I haz the power, 2017-01-10 18:34:47]

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-10 19:20:33]

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

[mmenke, 2017-01-10 19:20:34]

This depends on the other two test update CLs.

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

https://codereview.chromium.org/2626663002/diff/20001/content/browser/loader/...
content/browser/loader/test_resource_handler.cc:140: memset(buffer_->data(),
'\0', buffer_size_);
This change is to meet MockResourceLoader's expectation that these aren't set on
error (Which seems like a good idea to me...But then, I wrote that check)

[commit-bot: I haz the power, 2017-01-10 20:33:29]

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

[commit-bot: I haz the power, 2017-01-10 20:33:30]

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, 2017-01-10 21:06:39]

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

[commit-bot: I haz the power, 2017-01-10 21:07: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, 2017-01-10 22:45:46]

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

[commit-bot: I haz the power, 2017-01-10 22:45:47]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-10 23:46:56]

Randy:  Hold off on reviewing this one.  I discovered a bug-ish thing in
MimeSniffingResourceHandler after fixing a bug in my tests.

MimeSniffingResourceHandler::Resume synchronously calls into the ResourceHandler
below it in some cases.  While this isn't necessarily guaranteed by the API
contract, it's a property that ResourceLoader goes to a lot of trouble to
maintain, and I need to figure out what to do here (Also note that the old test.
 Feel free to review the ThrottlingResourceHandler tests in the meantime.

[mmenke, 2017-01-11 16:47:33]

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

[commit-bot: I haz the power, 2017-01-11 16:47:46]

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-11 17:23:53]

Ok, now this is ready for review.  The change to prevent re-entrancy was pretty
trivial, and I believe it should only PostTask in exactly the set of cases where
it's needed to prevent re-entrancy.

[commit-bot: I haz the power, 2017-01-11 18:29:07]

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

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

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-11 22:21:38]

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

[commit-bot: I haz the power, 2017-01-11 22:23:17]

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-11 22:56:09]

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

[commit-bot: I haz the power, 2017-01-11 22:56:10]

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

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

[commit-bot: I haz the power, 2017-01-12 15:48:02]

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

Patchset #8 (id:140001) has been deleted

[Randy Smith (Not in Mondays), 2017-01-12 18:44:29]

Fine except for my concern below, which I'm hoping is my missing something
subtle in the state machine.

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

https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:252:
weak_ptr_factory_.GetWeakPtr()));
Hmmm.  Your top level comment is that this PostTasks in exactly the cases in
which that's needed to avoid re-entrancy.  But it seems to me that if
MimeSniffingResourceHandler is acting in a pass through mode (or in another mode
where the only effect of AdvanceState() is to pass the Resume() up the stack)
that this'll be an extra PostTask.  I .... am concerned that that's a
performance hit exactly where we don't want it (i.e. on every request whenever
the request is paused and then resumed--for large requests that might be
frequently).  WDYT?

[mmenke, 2017-01-12 19:04:01]

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

https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:252:
weak_ptr_factory_.GetWeakPtr()));
On 2017/01/12 18:44:29, Randy Smith - Not in Mondays wrote:
> Hmmm.  Your top level comment is that this PostTasks in exactly the cases in
> which that's needed to avoid re-entrancy.  But it seems to me that if
> MimeSniffingResourceHandler is acting in a pass through mode (or in another
mode
> where the only effect of AdvanceState() is to pass the Resume() up the stack)
> that this'll be an extra PostTask.  I .... am concerned that that's a
> performance hit exactly where we don't want it (i.e. on every request whenever
> the request is paused and then resumed--for large requests that might be
> frequently).  WDYT?

I think what I said is correct, but it's not exactly obvious.

When we call AdvanceState, it will call ProcessState.  We call one of
MaybeIntercept, ReplayResponseReceived, or ReplayReadCompleted and they don't
defer.

So ReplayResponseReceived/ReplayReadCompleted always call into next_handler_, so
are clearly not safe to call synchronously.

That leaves MaybeIntercept.  If we're BUFFERING, we're reading data from the
server and not sending it to next_handler_...So Resume() can't be called when
we're in that state, and it's a moot point if we can re-entrantly call into
next_handler_ or not.  However, even if it could happen, we're only in
STATE_BUFFERING when reading data, so when we advance state, we're going to have
to replace that data (Even if it's just an EOF).

Or am I missing something?

[mmenke, 2017-01-12 19:06:32]

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

https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:252:
weak_ptr_factory_.GetWeakPtr()));
On 2017/01/12 19:04:01, mmenke wrote:
> On 2017/01/12 18:44:29, Randy Smith - Not in Mondays wrote:
> > Hmmm.  Your top level comment is that this PostTasks in exactly the cases in
> > which that's needed to avoid re-entrancy.  But it seems to me that if
> > MimeSniffingResourceHandler is acting in a pass through mode (or in another
> mode
> > where the only effect of AdvanceState() is to pass the Resume() up the
stack)
> > that this'll be an extra PostTask.  I .... am concerned that that's a
> > performance hit exactly where we don't want it (i.e. on every request
whenever
> > the request is paused and then resumed--for large requests that might be
> > frequently).  WDYT?
> 
> I think what I said is correct, but it's not exactly obvious.
> 
> When we call AdvanceState, it will call ProcessState.  We call one of
> MaybeIntercept, ReplayResponseReceived, or ReplayReadCompleted and they don't
> defer.
> 
> So ReplayResponseReceived/ReplayReadCompleted always call into next_handler_,
so
> are clearly not safe to call synchronously.
> 
> That leaves MaybeIntercept.  If we're BUFFERING, we're reading data from the
> server and not sending it to next_handler_...So Resume() can't be called when
> we're in that state, and it's a moot point if we can re-entrantly call into
> next_handler_ or not.  However, even if it could happen, we're only in
> STATE_BUFFERING when reading data, so when we advance state, we're going to
have
> to replace that data (Even if it's just an EOF).
> 
> Or am I missing something?

(Also note that in pass through mode, we call resume the request just above the
post task, so we only have to think about the case where we switch to pass
through mode in the AdvanceState call)

[Randy Smith (Not in Mondays), 2017-01-12 19:07:45]

On 2017/01/12 19:04:01, mmenke wrote:
>
https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
> File content/browser/loader/mime_sniffing_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
> content/browser/loader/mime_sniffing_resource_handler.cc:252:
> weak_ptr_factory_.GetWeakPtr()));
> On 2017/01/12 18:44:29, Randy Smith - Not in Mondays wrote:
> > Hmmm.  Your top level comment is that this PostTasks in exactly the cases in
> > which that's needed to avoid re-entrancy.  But it seems to me that if
> > MimeSniffingResourceHandler is acting in a pass through mode (or in another
> mode
> > where the only effect of AdvanceState() is to pass the Resume() up the
stack)
> > that this'll be an extra PostTask.  I .... am concerned that that's a
> > performance hit exactly where we don't want it (i.e. on every request
whenever
> > the request is paused and then resumed--for large requests that might be
> > frequently).  WDYT?
> 
> I think what I said is correct, but it's not exactly obvious.
> 
> When we call AdvanceState, it will call ProcessState.  We call one of
> MaybeIntercept, ReplayResponseReceived, or ReplayReadCompleted and they don't
> defer.
> 
> So ReplayResponseReceived/ReplayReadCompleted always call into next_handler_,
so
> are clearly not safe to call synchronously.
> 
> That leaves MaybeIntercept.  If we're BUFFERING, we're reading data from the
> server and not sending it to next_handler_...So Resume() can't be called when
> we're in that state, and it's a moot point if we can re-entrantly call into
> next_handler_ or not.  However, even if it could happen, we're only in
> STATE_BUFFERING when reading data, so when we advance state, we're going to
have
> to replace that data (Even if it's just an EOF).
> 
> Or am I missing something?

I haven't examined those states carefully, because I latched onto
STATE_STREAMING, which is the pass-through state that I expect
MimeSniffingResourceHandler to spend most of its time in.  In that state,
ProcessState() is a no-op, we pop back out to AdvanceState(), and call
controller()->Resume().  I presume that'll be the ResourceLoader, which will
PostTask(), and voila, we have two PostTasks.  

Am *I* missing something?

[Randy Smith (Not in Mondays), 2017-01-12 19:08:59]

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

https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:252:
weak_ptr_factory_.GetWeakPtr()));
On 2017/01/12 19:06:32, mmenke wrote:
> On 2017/01/12 19:04:01, mmenke wrote:
> > On 2017/01/12 18:44:29, Randy Smith - Not in Mondays wrote:
> > > Hmmm.  Your top level comment is that this PostTasks in exactly the cases
in
> > > which that's needed to avoid re-entrancy.  But it seems to me that if
> > > MimeSniffingResourceHandler is acting in a pass through mode (or in
another
> > mode
> > > where the only effect of AdvanceState() is to pass the Resume() up the
> stack)
> > > that this'll be an extra PostTask.  I .... am concerned that that's a
> > > performance hit exactly where we don't want it (i.e. on every request
> whenever
> > > the request is paused and then resumed--for large requests that might be
> > > frequently).  WDYT?
> > 
> > I think what I said is correct, but it's not exactly obvious.
> > 
> > When we call AdvanceState, it will call ProcessState.  We call one of
> > MaybeIntercept, ReplayResponseReceived, or ReplayReadCompleted and they
don't
> > defer.
> > 
> > So ReplayResponseReceived/ReplayReadCompleted always call into
next_handler_,
> so
> > are clearly not safe to call synchronously.
> > 
> > That leaves MaybeIntercept.  If we're BUFFERING, we're reading data from the
> > server and not sending it to next_handler_...So Resume() can't be called
when
> > we're in that state, and it's a moot point if we can re-entrantly call into
> > next_handler_ or not.  However, even if it could happen, we're only in
> > STATE_BUFFERING when reading data, so when we advance state, we're going to
> have
> > to replace that data (Even if it's just an EOF).
> > 
> > Or am I missing something?
> 
> (Also note that in pass through mode, we call resume the request just above
the
> post task, so we only have to think about the case where we switch to pass
> through mode in the AdvanceState call)

*bonk*  That's what I was missing.  Thank you.  LGTM.

[mmenke, 2017-01-12 19:17:15]

Thanks for the review!

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

https://codereview.chromium.org/2626663002/diff/120001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:252:
weak_ptr_factory_.GetWeakPtr()));
On 2017/01/12 19:08:58, Randy Smith - Not in Mondays wrote:
> On 2017/01/12 19:06:32, mmenke wrote:
> > On 2017/01/12 19:04:01, mmenke wrote:
> > > On 2017/01/12 18:44:29, Randy Smith - Not in Mondays wrote:
> > > > Hmmm.  Your top level comment is that this PostTasks in exactly the
cases
> in
> > > > which that's needed to avoid re-entrancy.  But it seems to me that if
> > > > MimeSniffingResourceHandler is acting in a pass through mode (or in
> another
> > > mode
> > > > where the only effect of AdvanceState() is to pass the Resume() up the
> > stack)
> > > > that this'll be an extra PostTask.  I .... am concerned that that's a
> > > > performance hit exactly where we don't want it (i.e. on every request
> > whenever
> > > > the request is paused and then resumed--for large requests that might be
> > > > frequently).  WDYT?
> > > 
> > > I think what I said is correct, but it's not exactly obvious.
> > > 
> > > When we call AdvanceState, it will call ProcessState.  We call one of
> > > MaybeIntercept, ReplayResponseReceived, or ReplayReadCompleted and they
> don't
> > > defer.
> > > 
> > > So ReplayResponseReceived/ReplayReadCompleted always call into
> next_handler_,
> > so
> > > are clearly not safe to call synchronously.
> > > 
> > > That leaves MaybeIntercept.  If we're BUFFERING, we're reading data from
the
> > > server and not sending it to next_handler_...So Resume() can't be called
> when
> > > we're in that state, and it's a moot point if we can re-entrantly call
into
> > > next_handler_ or not.  However, even if it could happen, we're only in
> > > STATE_BUFFERING when reading data, so when we advance state, we're going
to
> > have
> > > to replace that data (Even if it's just an EOF).
> > > 
> > > Or am I missing something?
> > 
> > (Also note that in pass through mode, we call resume the request just above
> the
> > post task, so we only have to think about the case where we switch to pass
> > through mode in the AdvanceState call)
> 
> *bonk*  That's what I was missing.  Thank you.  LGTM.

Note that there was a bug in my argument about the STATE_BUFFERING case - if it
wants more data, it completes synchronously.  But the "Resume not called while
buffering" argument still holds.  Anyways, CQing.

[mmenke, 2017-01-12 19:17:23]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-01-12 19:18:10]

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

[mmenke, 2017-01-12 19:21:34]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-01-12 19:21:58]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-12 19:21:59]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2626663002/#ps160001
(title: "Merge")

[commit-bot: I haz the power, 2017-01-12 19:22:47]

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

[mmenke, 2017-01-12 20:43:09]

Description was changed from

==========
Update MimeSniffingResourceHandler tests to use MockResourceLoader.

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

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

to

==========
Update MimeSniffingResourceHandler tests to use MockResourceLoader.

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

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

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

[commit-bot: I haz the power, 2017-01-12 21:19:00]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484248918964090,
"parent_rev": "e82471319e5760053bde3525acd5300bd4471327", "commit_rev":
"2522425b429d16cfdc97f11e4bbb4ac28d1a20fd"}

[commit-bot: I haz the power, 2017-01-12 21:19:30]

Description was changed from

==========
Update MimeSniffingResourceHandler tests to use MockResourceLoader.

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

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

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

to

==========
Update MimeSniffingResourceHandler tests to use MockResourceLoader.

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

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

BUG=659317

Review-Url: https://codereview.chromium.org/2626663002
Cr-Commit-Position: refs/heads/master@{#443360}
Committed:
https://chromium.googlesource.com/chromium/src/+/2522425b429d16cfdc97f11e4bbb...
==========

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

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2522425b429d16cfdc97f11e4bbb...