Send fatal proxy errors to the network delegate.

Send fatal proxy errors to the network delegate.

BUG=60099
TEST=browser test

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

[jochen (gone - plz use gerrit), 2011-02-18 13:54:10]

please review

mpcomplete extension bits
willchan - network delegate
eroman - proxy
battre - api design

Dominic, what's the bug #?

[Matt Perry, 2011-02-18 19:51:48]

http://codereview.chromium.org/6541021/diff/3015/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/proxy/events/test.js (right):

http://codereview.chromium.org/6541021/diff/3015/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/proxy/events/test.js:9: if (error.fatal &&
error.details == "")
use:
  chrome.test.assertTrue(error.fatal)
  chrome.test.assertEq("", error.details)
  chrome.test.notifyPass()

[willchan no longer on Chromium, 2011-02-18 21:02:45]

You're not intending to cover all proxy errors here, right? Because you've only
caught one. Just want to make sure you're planning on adding the other ones
later.

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request....
net/url_request/url_request.cc:490: if (context_ &&
context_->network_delegate())
context_ is always non-NULL.

[jochen (gone - plz use gerrit), 2011-02-18 21:06:14]

On 2011/02/18 21:02:45, willchan wrote:
> You're not intending to cover all proxy errors here, right? Because you've
only
> caught one. Just want to make sure you're planning on adding the other ones
> later.

I want to catch all fatal proxy errors, i.e. situations where you can't connect
to the internet.

I'll also add an event that is send on javascript runtime errors in the pac
script (these are non-fatal errors since chrome will fallback to direct, but
it's an interesting error nevertheless)

What other situation should I look out for?


> 
>
http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request.cc
> File net/url_request/url_request.cc (right):
> 
>
http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request....
> net/url_request/url_request.cc:490: if (context_ &&
> context_->network_delegate())
> context_ is always non-NULL.

[jochen (gone - plz use gerrit), 2011-02-18 21:38:40]

the general idea of this event is:

- report fatal errors, i.e. it's not possible to connect using the current proxy
config, since that's something an extension might want to do something about
(show an UI or similar)

- report javascript errors from the pac script (useful for debugging mainly)

[willchan no longer on Chromium, 2011-02-18 21:56:56]

ERR_PROXY_CONNECTION_FAILED just means that we could not actually TCP connect to
the proxy (could be because of host resolution failures or socket failures or
whatever).

How do you plan to handle the case when the proxy prompts for credentials that
we don't have? Is that safe to ignore?

You probably should handle ERR_PROXY_AUTH_UNSUPPORTED and
ERR_TUNNEL_CONNECTION_FAILED. Note that at least the latter error may not be
discovered until Read() is called.

On 2011/02/18 21:38:40, jochen wrote:
> the general idea of this event is:
> 
> - report fatal errors, i.e. it's not possible to connect using the current
proxy
> config, since that's something an extension might want to do something about
> (show an UI or similar)
> 
> - report javascript errors from the pac script (useful for debugging mainly)

[jochen (gone - plz use gerrit), 2011-02-21 14:52:59]

the proxy pac stuff will come in a separate CL

http://codereview.chromium.org/6541021/diff/3015/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/proxy/events/test.js (right):

http://codereview.chromium.org/6541021/diff/3015/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/proxy/events/test.js:9: if (error.fatal &&
error.details == "")
On 2011/02/18 19:51:49, Matt Perry wrote:
> use:
>   chrome.test.assertTrue(error.fatal)
>   chrome.test.assertEq("", error.details)
>   chrome.test.notifyPass()

Done.

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request....
net/url_request/url_request.cc:490: if (context_ &&
context_->network_delegate())
On 2011/02/18 21:02:45, willchan wrote:
> context_ is always non-NULL.

Done.

[jochen (gone - plz use gerrit), 2011-02-21 15:55:53]

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/6541021/diff/3015/net/url_request/url_request....
net/url_request/url_request.cc:490: if (context_ &&
context_->network_delegate())
On 2011/02/21 14:52:59, jochen wrote:
> On 2011/02/18 21:02:45, willchan wrote:
> > context_ is always non-NULL.
> 
> Done.

actually, it's NULL during some unit tests

[willchan no longer on Chromium, 2011-02-21 23:35:39]

You should probably update url_request_unittest.cc with some tests for these
cases. For the mock sockets needed to simulate the proxy errors, you can find
them in http_network_transaction_unittest.cc.

http://codereview.chromium.org/6541021/diff/5004/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/6541021/diff/5004/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:349: // TODO(willchan): Enable for proxy script
fetcher context.
What happened here? How does it break?

http://codereview.chromium.org/6541021/diff/5004/net/http/http_network_delega...
File net/http/http_network_delegate.h (right):

http://codereview.chromium.org/6541021/diff/5004/net/http/http_network_delega...
net/http/http_network_delegate.h:26: // Called when the Read of the response
body is completed.
You should say this corresponds to URLRequest::Delegate::Read(). Otherwise, you
need to note here that bytes_read is -1 when an error occurs, and the
URLRequestStatus will be updated.

[jochen (gone - plz use gerrit), 2011-02-21 23:37:32]

On 2011/02/21 23:35:39, willchan wrote:
> You should probably update url_request_unittest.cc with some tests for these
> cases. For the mock sockets needed to simulate the proxy errors, you can find
> them in http_network_transaction_unittest.cc.
> 
> http://codereview.chromium.org/6541021/diff/5004/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
http://codereview.chromium.org/6541021/diff/5004/chrome/browser/io_thread.cc#...
> chrome/browser/io_thread.cc:349: // TODO(willchan): Enable for proxy script
> fetcher context.
> What happened here? How does it break?

chrome network delegate includes a static cast to chrome url request context.

> 
>
http://codereview.chromium.org/6541021/diff/5004/net/http/http_network_delega...
> File net/http/http_network_delegate.h (right):
> 
>
http://codereview.chromium.org/6541021/diff/5004/net/http/http_network_delega...
> net/http/http_network_delegate.h:26: // Called when the Read of the response
> body is completed.
> You should say this corresponds to URLRequest::Delegate::Read(). Otherwise,
you
> need to note here that bytes_read is -1 when an error occurs, and the
> URLRequestStatus will be updated.

[jochen (gone - plz use gerrit), 2011-02-22 12:27:44]

I've pulled out the extension bits from this CL, as I'm only waiting for
feedback from Eric on the network/proxy part


 http://codereview.chromium.org/6549015

[eroman, 2011-02-22 23:21:58]

LGTM.

This takes the approach of identifying the "obvious" proxy errors, however it is
also possible that DIRECT connections are failing in other mysterious ways
(ERR_NAME_NOT_RESOLVED, ERR_CONNECTION_RESET, ERR_CONNECTION_TIMEOUT, etc..),
due to not using a proxy server. 

Not sure what you can do about those though, since there is no way to know from
the error code whether it was caused by misconfigured proxy.

http://codereview.chromium.org/6541021/diff/3017/net/url_request/url_request_...
File net/url_request/url_request_test_util.h (right):

http://codereview.chromium.org/6541021/diff/3017/net/url_request/url_request_...
net/url_request/url_request_test_util.h:193: TestHttpNetworkDelegate();
nit: indentation.