Fix short error responses sniffed as downloads hanging.

Fix short error responses sniffed as downloads hanging.

When MimeSniffingResourceHandler sniffs an error responses request as a
download, it refuses to download the file and cancels the request with
an error instead.  It used to both pass cancellation up through the
ResourceHandler and cancel the URLRequest itself.  However, as of
https://codereview.chromium.org/2526983002, it just cancelled the
request, rather than passing the cancellation message up the
ResourceHandler stack.

This mostly works...Except in the case we had to read to the end of
the response before we could sniff the mime type. Completed requests
do nothing when canceled, so the request would hang. Without the
notification up through the ResourceHandler stack, the ResourceLoader
never learns of cancellation.

This CL replaces cancelling the URLRequest directly with cancelling
through the ResourceController (Which is what it should have been
doing, anyways).

BUG=738680
Review-Url: https://codereview.chromium.org/2974493002
Cr-Commit-Position: refs/heads/master@{#484720}
Committed: https://chromium.googlesource.com/chromium/src/+/72fd4f4bc6752250814df9ca73ab9786abc667dd

[mmenke, 2017-07-06 17:46:16]

Description was changed from

==========
.

BUG=
==========

to

==========
Fix short error responses sniffed as downloads hanging.

When MimeSniffingResourceHandler sniffs an error responses request as a
download, it refuses to download the file and cancels the request with
an error instead.  It used to both pass cancellation up through the
ResourceHandler and cancel the URLRequest itself.  However, as of
https://codereview.chromium.org/2526983002, it just cancelled the
request, rather than passing the cancellation message up the
ResourceHandler stack.

This mostly works...Except in the case we had to read to the end of
the response before we could sniff the mime type. Completed requests
do nothing when canceled, so the request would hang. Without the
notification up through the ResourceHandler stack, the ResourceLoader
never learns of cancellation.

This CL replaces cancelling the URLRequest directly with cancelling
through the ResourceController (Which is what it should have been
doing, anyways).

BUG=738680
==========

[mmenke, 2017-07-06 17:49:05]

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

[commit-bot: I haz the power, 2017-07-06 17:49:16]

Dry run: CQ is trying da patch.

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

[mmenke, 2017-07-06 17:51:56]

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

[mmenke, 2017-07-06 17:51:57]

I have the strangest feeling I've had to fix this case before.

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

https://codereview.chromium.org/2974493002/diff/20001/content/browser/loader/...
content/browser/loader/mime_sniffing_resource_handler.cc:578:
CancelWithError(net::ERR_INVALID_RESPONSE);
I verified that we should be holding onto a controller at both the places this
is called from.

[Charlie Harrison, 2017-07-06 18:09:38]

LGTM

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
File chrome/browser/net/errorpage_browsertest.cc (right):

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
chrome/browser/net/errorpage_browsertest.cc:336: new
net::test_server::RawHttpResponse("HTTP/1.1 500 Server Sad :(",
Can you use base::MakeUnique here to avoid raw new?

[mmenke, 2017-07-06 18:17:19]

Thanks!

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
File chrome/browser/net/errorpage_browsertest.cc (right):

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
chrome/browser/net/errorpage_browsertest.cc:336: new
net::test_server::RawHttpResponse("HTTP/1.1 500 Server Sad :(",
On 2017/07/06 18:09:37, Charlie Harrison wrote:
> Can you use base::MakeUnique here to avoid raw new?

No - MakeUnique doesn't handle upcasting (I'm creating a RawHttpResponse, and
returning it as an HttpResonse).  std::move does handle that, so I could make a
unique_ptr on the stack and then std::move it, but I don't think that's any
better, since std::moving return values isn't the preferred way to do things.

[mmenke, 2017-07-06 18:17:24]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-07-06 18:17:29]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-07-06 18:17:45]

CQ is trying da patch.

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

[Charlie Harrison, 2017-07-06 18:23:09]

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
File chrome/browser/net/errorpage_browsertest.cc (right):

https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
chrome/browser/net/errorpage_browsertest.cc:336: new
net::test_server::RawHttpResponse("HTTP/1.1 500 Server Sad :(",
On 2017/07/06 18:17:19, mmenke wrote:
> On 2017/07/06 18:09:37, Charlie Harrison wrote:
> > Can you use base::MakeUnique here to avoid raw new?
> 
> No - MakeUnique doesn't handle upcasting (I'm creating a RawHttpResponse, and
> returning it as an HttpResonse).  std::move does handle that, so I could make
a
> unique_ptr on the stack and then std::move it, but I don't think that's any
> better, since std::moving return values isn't the preferred way to do things.

I was thinking of just returning the base::MakeUnique<RawHttpResponse>(...),
since I thought the type conversion would be implicit.

[mmenke, 2017-07-06 18:24:36]

On 2017/07/06 18:23:09, Charlie Harrison wrote:
>
https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
> File chrome/browser/net/errorpage_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
> chrome/browser/net/errorpage_browsertest.cc:336: new
> net::test_server::RawHttpResponse("HTTP/1.1 500 Server Sad :(",
> On 2017/07/06 18:17:19, mmenke wrote:
> > On 2017/07/06 18:09:37, Charlie Harrison wrote:
> > > Can you use base::MakeUnique here to avoid raw new?
> > 
> > No - MakeUnique doesn't handle upcasting (I'm creating a RawHttpResponse,
and
> > returning it as an HttpResonse).  std::move does handle that, so I could
make
> a
> > unique_ptr on the stack and then std::move it, but I don't think that's any
> > better, since std::moving return values isn't the preferred way to do
things.
> 
> I was thinking of just returning the base::MakeUnique<RawHttpResponse>(...),
> since I thought the type conversion would be implicit.

It's not, unfortunately.  You can't return a std::unique_ptr<bar> as a
std::unique_ptr<foo>, without an std::move.

[Charlie Harrison, 2017-07-06 18:25:26]

On 2017/07/06 18:24:36, mmenke wrote:
> On 2017/07/06 18:23:09, Charlie Harrison wrote:
> >
>
https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
> > File chrome/browser/net/errorpage_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2974493002/diff/20001/chrome/browser/net/erro...
> > chrome/browser/net/errorpage_browsertest.cc:336: new
> > net::test_server::RawHttpResponse("HTTP/1.1 500 Server Sad :(",
> > On 2017/07/06 18:17:19, mmenke wrote:
> > > On 2017/07/06 18:09:37, Charlie Harrison wrote:
> > > > Can you use base::MakeUnique here to avoid raw new?
> > > 
> > > No - MakeUnique doesn't handle upcasting (I'm creating a RawHttpResponse,
> and
> > > returning it as an HttpResonse).  std::move does handle that, so I could
> make
> > a
> > > unique_ptr on the stack and then std::move it, but I don't think that's
any
> > > better, since std::moving return values isn't the preferred way to do
> things.
> > 
> > I was thinking of just returning the base::MakeUnique<RawHttpResponse>(...),
> > since I thought the type conversion would be implicit.
> 
> It's not, unfortunately.  You can't return a std::unique_ptr<bar> as a
> std::unique_ptr<foo>, without an std::move.

I see, thanks for the clarification.

[commit-bot: I haz the power, 2017-07-06 20:03:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499365049598850,
"parent_rev": "f0dea336ab1ebe31359f0f833140a53bdd2992a9", "commit_rev":
"72fd4f4bc6752250814df9ca73ab9786abc667dd"}

[commit-bot: I haz the power, 2017-07-06 20:03:51]

Description was changed from

==========
Fix short error responses sniffed as downloads hanging.

When MimeSniffingResourceHandler sniffs an error responses request as a
download, it refuses to download the file and cancels the request with
an error instead.  It used to both pass cancellation up through the
ResourceHandler and cancel the URLRequest itself.  However, as of
https://codereview.chromium.org/2526983002, it just cancelled the
request, rather than passing the cancellation message up the
ResourceHandler stack.

This mostly works...Except in the case we had to read to the end of
the response before we could sniff the mime type. Completed requests
do nothing when canceled, so the request would hang. Without the
notification up through the ResourceHandler stack, the ResourceLoader
never learns of cancellation.

This CL replaces cancelling the URLRequest directly with cancelling
through the ResourceController (Which is what it should have been
doing, anyways).

BUG=738680
==========

to

==========
Fix short error responses sniffed as downloads hanging.

When MimeSniffingResourceHandler sniffs an error responses request as a
download, it refuses to download the file and cancels the request with
an error instead.  It used to both pass cancellation up through the
ResourceHandler and cancel the URLRequest itself.  However, as of
https://codereview.chromium.org/2526983002, it just cancelled the
request, rather than passing the cancellation message up the
ResourceHandler stack.

This mostly works...Except in the case we had to read to the end of
the response before we could sniff the mime type. Completed requests
do nothing when canceled, so the request would hang. Without the
notification up through the ResourceHandler stack, the ResourceLoader
never learns of cancellation.

This CL replaces cancelling the URLRequest directly with cancelling
through the ResourceController (Which is what it should have been
doing, anyways).

BUG=738680

Review-Url: https://codereview.chromium.org/2974493002
Cr-Commit-Position: refs/heads/master@{#484720}
Committed:
https://chromium.googlesource.com/chromium/src/+/72fd4f4bc6752250814df9ca73ab...
==========

[commit-bot: I haz the power, 2017-07-06 20:03:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/72fd4f4bc6752250814df9ca73ab...