Set isCancelling flag on WebURLError when request has net::ERR_ABORTED as an error_code

Set isCancelling flag on WebURLError when request has net::ERR_ABORTED as an error_code.

BUG=82422
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90532

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=92351

[gavinp, 2011-06-15 15:31:23]

This approach looks good to me, but I think the new function in rdh is
needlessly duplicative; with that fixed I am really happy to have this problem
gone.

http://codereview.chromium.org/6995118/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6995118/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/resource_dispatcher_host.cc:148: void
CancelRequestBeforeItStarts(ResourceMessageFilter* filter,
Instead of writing this function, add a bool or enumerated parameter to
AbortRequestBeforeItStarts, and maybe rename it to something more general.

[gavinp, 2011-06-15 15:31:44]

Also, see the webkit side of this at
https://bugs.webkit.org/show_bug.cgi?id=62396

[darin (slow to review), 2011-06-15 16:35:03]

http://codereview.chromium.org/6995118/diff/1/webkit/glue/weburlloader_impl.cc
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/6995118/diff/1/webkit/glue/weburlloader_impl.c...
webkit/glue/weburlloader_impl.cc:658: error.isCanceling = true;
The net error code ERR_ABORTED is ordinarily used to convey the canceled state. 
Can you use that?

You can see that when status.status() is HANDLED_EXTERNALLY that we force
error_code to be ERR_ABORTED.  You could do the same thing for the CANCELED
state.

Also, maybe I don't know what you are trying to do because I don't see anywhere
in this CL where you check isCanceling.  Is there another part to this CL?

[darin (slow to review), 2011-06-15 16:48:44]

http://codereview.chromium.org/6995118/diff/1/webkit/glue/weburlloader_impl.cc
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/6995118/diff/1/webkit/glue/weburlloader_impl.c...
webkit/glue/weburlloader_impl.cc:658: error.isCanceling = true;
On 2011/06/15 16:35:03, darin wrote:
> The net error code ERR_ABORTED is ordinarily used to convey the canceled
state. 
> Can you use that?
> 
> You can see that when status.status() is HANDLED_EXTERNALLY that we force
> error_code to be ERR_ABORTED.  You could do the same thing for the CANCELED
> state.
> 
> Also, maybe I don't know what you are trying to do because I don't see
anywhere
> in this CL where you check isCanceling.  Is there another part to this CL?

OK... after some more coffee, I now understand what is going on here.  Sorry for
the noise.

It seems like there are two options here:

1) Test URLRequestStatus::status() against URLRequestStatus::CANCELED as you are
doing here, or...

2) Test URLRequestStatus::os_error() against net::ERR_ABORTED.

It is important to understand that URLRequestStatus existed before we had
net_error_list.h.  In the old days, os_error() was a WinINet error code.  Now,
with net_error_list.h, the various status levels for URLRequestStatus are
largely redundant with actual network error codes.  We should be able to kill
off the status field in fact.

My point is that since net::ERR_ABORTED means "user cancellation", it would be
unfortunate if CANCELED were false when os_error is ERR_ABORTED.  Or, put
another way, it would be unfortunate to have a WebURLError conveying ERR_ABORTED
without isCancellation set to true.

So, I suspect that implementation #2 would actually be better.

[vsevik, 2011-06-15 21:27:17]

I don't think net::ERR_ABORTED means "user cancellation". Maybe it is supposed
to, but it doesn't or at least there are different types of "user cancellation".
Here
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/rende...
we are making a call to AbortRequestBeforeItStarts() based on the security
policy. It is reasonable to show console error in this case, so we should some
how distinguish these two cases
1) request is canceled/aborted because of (security) error - show console error;
2) request is canceled/aborted because it was handled somewhere else - do not
show console error;

Currently we are using net::URLRequestStatus::FAILED, net::ERR_ABORTED in both
cases and I tried to split them in this change. 

Maybe instead of setting URLRequestStatus::CANCELED we should add something like
net::ERR_CANCELED?

[darin (slow to review), 2011-06-15 22:06:18]

ERR_ABORTED is defined as "An operation was aborted (due to user action)."

I've always been careful to treat ERR_ABORTED this way.  If the code no
longer does,
then that is a bug.

It looks like ShouldServiceRequest is actually checking if the renderer has
permission
to make a particular request.  It is enforcing the rules of the sandbox.  In
this case, the
error to generate is probably something more along the lines of
ERR_ACCESS_DENIED.
However, a renderer making an illegal request should only happen if there
was a coding
error or a hacked renderer.  We might even consider nuking a renderer like
this!

I don't understand why you think we should show a console error in this
case.  It should
be something an end developer never sees.



On Wed, Jun 15, 2011 at 2:27 PM, <vsevik@chromium.org> wrote:

> I don't think net::ERR_ABORTED means "user cancellation". Maybe it is
> supposed
> to, but it doesn't or at least there are different types of "user
> cancellation".
> Here
> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc&l=**389<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/renderer_host/resource_dispatcher_host.cc&l=389>
> we are making a call to AbortRequestBeforeItStarts() based on the security
> policy. It is reasonable to show console error in this case, so we should
> some
> how distinguish these two cases
> 1) request is canceled/aborted because of (security) error - show console
> error;
>

The above case seems like a failure case, and ERR_ABORTED is probably not
the
right error code.  ERR_ACCESS_DENIED seems better.



> 2) request is canceled/aborted because it was handled somewhere else - do
> not
> show console error;
>

In this case, you can probably argue that the user is aware of the fact that
the request is
being handled somewhere else.  So, it probably does make sense to use
ERR_ABORTED.



>
> Currently we are using net::URLRequestStatus::FAILED, net::ERR_ABORTED in
> both
> cases and I tried to split them in this change.
>
> Maybe instead of setting URLRequestStatus::CANCELED we should add something
> like
> net::ERR_CANCELED?


ERR_CANCELED sounds like a synonym for ERR_ABORTED.  having both error codes
would be super confusing as you could not easily tell them apart.
 ERR_ABORTED is meant
to convey the same thing as URLRequestStatus::CANCELED.



>
>
http://codereview.chromium.**org/6995118/<http://codereview.chromium.org/6995...
>

[vsevik, 2011-06-16 19:12:50]

Darin, Gavin,

Could you please take another look?

[darin (slow to review), 2011-06-17 17:45:18]

looks good, except:

http://codereview.chromium.org/6995118/diff/8001/webkit/glue/weburlloader_imp...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/6995118/diff/8001/webkit/glue/weburlloader_imp...
webkit/glue/weburlloader_impl.cc:658: error.isCanceling = true;
I was arguing in the WebKit bug that we should name this field isCancellation to
match the term used in WebCore.

[vsevik, 2011-06-20 12:15:51]

I am sorry, I missed that. 
Renamed this field, could you please take another look?

[darin (slow to review), 2011-06-20 19:23:48]

LGTM

[gavinp, 2011-06-20 19:25:05]

On 2011/06/20 19:23:48, darin wrote:
> LGTM

LGTM.

[commit-bot: I haz the power, 2011-06-26 21:10:18]

Change committed as 90532

[commit-bot: I haz the power, 2011-07-13 10:54:51]

Change committed as 92351