Automatically retry failed network requests.

Retry failed network requests.

BUG=143425

[mmenke, 2012-08-17 18:36:06]

I want to see how promising this looks before making it too complicated.

I included both the most common error types and those I thought most likely to
work on retry.  I'd be surprised if retrying on ERR_NAME_NOT_RESOLVED or
ERR_INTERNET_DISCONNECTED turns out to be useful, but they're quite common.  One
of the cache errors is quite uncommon (can't remember which), but if we get the
error before we read headers, I suspect retrying has a high probability of
working.

Potential modifications, if it proves useful:
- Only allow one retry per UrlRequest.  As it is, this can retry once per
redirect.
- Only retry main frames or certain resource types.
- Rate limit retries, possibly based on number of failed retries per domain.
- Add load flag, or some other method, to prevent retries.  favicons, for
instance, probably should not use retries.  Could imagine servers behaving in
all sorts of bad ways on missing files.

[willchan no longer on Chromium, 2012-08-21 21:01:10]

LGTM as a temporary experiment.

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:747: if (error_before_retry_ != OK) {
Why aren't we checking has_retried_? And if true, then assert
error_before_retry_ < ERR_IO_PENDING.

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:749: error_before_retry_ = OK;
Why is this needed?

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1385: if (request_info_.load_flags &
LOAD_MAIN_FRAME) {
Note that long-term this is definitely unacceptable. I don't want more
proliferation of LOAD_MAIN_FRAME since those don't belong at the net/ level.

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1386: if (result >= 0) {
Why is this >= 0? Shouldn't it be OK? Do we expect any other values?

[mmenke, 2012-08-21 21:25:29]

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:747: if (error_before_retry_ != OK) {
On 2012/08/21 21:01:10, willchan wrote:
> Why aren't we checking has_retried_? And if true, then assert
> error_before_retry_ < ERR_IO_PENDING.

See response below.  I've added a DCHECK(has_retried_);

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:749: error_before_retry_ = OK;
On 2012/08/21 21:01:10, willchan wrote:
> Why is this needed?

There are a bunch of ways to restart (ContinueWithCertificate,
ContinueDespiteLastError, RestartTransactionWithAuth).  If we retry a request as
a result of an error, and then retry for another reason, we don't want to add
two entries to the retry histogram.

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1385: if (request_info_.load_flags &
LOAD_MAIN_FRAME) {
On 2012/08/21 21:01:10, willchan wrote:
> Note that long-term this is definitely unacceptable. I don't want more
> proliferation of LOAD_MAIN_FRAME since those don't belong at the net/ level.

Yea, I think you're right about that.

You think the way to do this, if it ends up being useful, is with a
NetworkDelegate function?  ShouldRetryRequestOnFailure?  Could also do the
entire retry Chrome-side, but messing much with ResourceDispatcherHost just
seems a bad idea.

http://codereview.chromium.org/10854204/diff/5007/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1386: if (result >= 0) {
On 2012/08/21 21:01:10, willchan wrote:
> Why is this >= 0? Shouldn't it be OK? Do we expect any other values?

No, we don't, I was just being paranoid.  Done.

[commit-bot: I haz the power, 2012-08-23 15:04:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10854204/8004

[commit-bot: I haz the power, 2012-08-23 17:30:12]

Change committed as 153025