Implement URLRequestBackoffManager for managing Backoff headers

Implement URLRequestBackoffManager for managing Backoff headers

This CL implemented a URLRequestBackoffManager to manage
Backoff response headers. This new feature is off by default
in this CL.

Design doc:
https://docs.google.com/document/d/1aAxwXK7Vw3VigFd6MmrItbAIgMdKAf-XxXXbhWXdID0/edit?usp=sharing

BUG=486891
Committed: https://crrev.com/3bb781a314aa895fd1eb576689ec4f452fbe0c4b
Cr-Commit-Position: refs/heads/master@{#339961}

[xunjieli, 2015-06-26 13:52:23]

Patchset #1 (id:1) has been deleted

[xunjieli, 2015-06-26 13:52:30]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2015-06-26 13:52:37]

Patchset #1 (id:40001) has been deleted

[xunjieli, 2015-06-26 13:59:49]

Patchset #1 (id:60001) has been deleted

[xunjieli, 2015-06-26 14:23:51]

Patchset #1 (id:80001) has been deleted

[xunjieli, 2015-06-26 14:24:41]

Patchset #1 (id:80001) has been deleted

[xunjieli, 2015-06-26 15:05:11]

Patchset #1 (id:100001) has been deleted

[xunjieli, 2015-06-26 15:19:59]

Patchset #2 (id:140001) has been deleted

[xunjieli, 2015-07-06 13:55:44]

Patchset #1 (id:120001) has been deleted

[xunjieli, 2015-07-06 13:58:39]

xunjieli@chromium.org changed reviewers:
+ ttuttle@chromium.org

[xunjieli, 2015-07-06 13:58:40]

Hi ttuttle, this is the CL that I talked about earlier. Thanks for the review in
advance! Please let me know if you have any question.

[mmenke, 2015-07-07 21:33:09]

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

[mmenke, 2015-07-07 21:33:10]

A couple drive by comments - didn't look over the entire CL (And don't plan to).

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_lis...
net/base/net_error_list.h:808: NET_ERROR(TEMPORARY_BACKOFF, -807)
Should add an error page for this, since this is an expected error that can be
triggered by the server.  See localized_error.cc and generated_resources.grd.

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_lis...
net/base/net_error_list.h:808: NET_ERROR(TEMPORARY_BACKOFF, -807)
The 8xx errors are used for DNS errors.  The -3xx error codes seem a better fit.

https://codereview.chromium.org/1153093002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1153093002/diff/160001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:207: sdch_enabled_(false) {
Need to initialize backoff_enabled_

[xunjieli, 2015-07-08 16:10:50]

Thanks for the feedback! Uploaded a new patch to address the comments. ttuttle@,
could you take a look when you get time? Thanks!

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_lis...
net/base/net_error_list.h:808: NET_ERROR(TEMPORARY_BACKOFF, -807)
On 2015/07/07 21:33:10, mmenke wrote:
> The 8xx errors are used for DNS errors.  The -3xx error codes seem a better
fit.

Done. I see! I missed the comments at the top.

https://codereview.chromium.org/1153093002/diff/160001/net/base/net_error_lis...
net/base/net_error_list.h:808: NET_ERROR(TEMPORARY_BACKOFF, -807)
On 2015/07/07 21:33:10, mmenke wrote:
> Should add an error page for this, since this is an expected error that can be
> triggered by the server.  See localized_error.cc and generated_resources.grd.

Done.

https://codereview.chromium.org/1153093002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/1153093002/diff/160001/net/url_request/url_re...
net/url_request/url_request_context_builder.cc:207: sdch_enabled_(false) {
On 2015/07/07 21:33:10, mmenke wrote:
> Need to initialize backoff_enabled_

Done.

[mmenke, 2015-07-09 18:52:30]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[xunjieli, 2015-07-13 20:00:47]

Hey ttuttle, it's been a week, just a friendly ping to make sure this is still
on your radar!

[Deprecated (see juliatuttle), 2015-07-13 20:30:41]

Looking good; just want a couple comments.

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.cc (right):

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.cc:66: // Allow one request between
throttled_time and release_time.
I'd like a comment (here or elsewhere) noting that this does not implement the
uniform distribution part of the "retry between time and 1.1 * time from now"
requirement.

Also, have you considered how a caller might implement that? Right now, the
caller would have to re-parse the backoff header and manually calculate a good
time.

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.h (right):

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:22: // Class that manages
information on Backoff headers. URL requests for HTTP
A link to the spec that you're implementing here (if a public copy exists) would
be cool.

[xunjieli, 2015-07-13 20:47:23]

Patchset #3 (id:200001) has been deleted

[xunjieli, 2015-07-13 20:53:38]

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.cc (right):

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.cc:66: // Allow one request between
throttled_time and release_time.
On 2015/07/13 20:30:41, ttuttle wrote:
> I'd like a comment (here or elsewhere) noting that this does not implement the
> uniform distribution part of the "retry between time and 1.1 * time from now"
> requirement.
> 
Done.
> Also, have you considered how a caller might implement that? Right now, the
> caller would have to re-parse the backoff header and manually calculate a good
> time.

I imagine I'd implement a GetReleaseTime in the backoff manager for the consumer
to get the release time(s) for a particular URL.

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.h (right):

https://codereview.chromium.org/1153093002/diff/180001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:22: // Class that manages
information on Backoff headers. URL requests for HTTP
On 2015/07/13 20:30:41, ttuttle wrote:
> A link to the spec that you're implementing here (if a public copy exists)
would
> be cool.

Done.

[Deprecated (see juliatuttle), 2015-07-13 21:21:26]

Thanks, lgtm!

[xunjieli, 2015-07-14 13:48:46]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-07-14 13:48:51]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2015-07-14 13:53:28]

xunjieli@chromium.org changed reviewers:
+ cpu@chromium.org

[xunjieli, 2015-07-14 13:56:03]

+ cpu@,

Hi Carlos, could you take a look at chrome/common/localized_error.cc? You are
welcomed to review the rest as well if you'd like to do so. A follow-up CL will
enable the feature for experiments.

[cpu_(ooo_6.6-7.5), 2015-07-17 19:34:45]

looking at it right now...

[cpu_(ooo_6.6-7.5), 2015-07-17 19:56:01]

this looks good. Here is my first pass.

https://codereview.chromium.org/1153093002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/base/net_error_lis...
net/base/net_error_list.h:645: // Request is throttled because of a Backoff
header.
maybe add link to the chromium-dev document?

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:22: // Class that manages
information on Backoff headers. URL requests for HTTP
"url requests for HTTP" --> HTTPS ?

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:26: //
https://docs.google.com/document/d/1aAxwXK7Vw3VigFd6MmrItbAIgMdKAf-XxXXbhWXdI...
Getting error trying to open that.

com.google.apps.docs.error.DocumentNotFoundException: Invalid or missing
required AppKey
	at
com.google.apps.docs.security.DocumentRequiredInterceptor.execute(DocumentRequiredInterceptor.java:36)
	at
com.google.apps.framework.request.impl.InterceptorInvocation$Http.beforeProceed(InterceptorInvocation.java:135)
	at
com.google.apps.framework.request.impl.InterceptorInvocation.proceed(InterceptorInvocation.java:166)
	at
com.google.apps.framework.request.ActionServlet$Dispatcher.dispatch(ActionServlet.java:81)
	at
com.google.apps.framework.request.ActionServlet.service(ActionServlet.java:47)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_context.cc:34: backoff_manager_(NULL),
nullptr

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_context.h:187: // May be NULL.
this comment should be the same style as 179 or 193

[xunjieli, 2015-07-17 21:24:16]

Thanks for the review! I just realize that I forgot about the part where we only
accept backoff headers from secure connection with no cert error. Although
that's a couple lines change, but I need to rework the tests. Will send out an
update and upload a new patch on Monday.

[xunjieli, 2015-07-20 14:22:27]

Patchset #4 (id:240001) has been deleted

[xunjieli, 2015-07-20 19:52:49]

Patchset #5 (id:280001) has been deleted

[xunjieli, 2015-07-20 19:58:35]

Patchset #5 (id:300001) has been deleted

[xunjieli, 2015-07-20 20:02:13]

Added check for accepting headers on secure connection with no ssl errors, and
fixed tests. PTAL.

https://codereview.chromium.org/1153093002/diff/220001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/base/net_error_lis...
net/base/net_error_list.h:645: // Request is throttled because of a Backoff
header.
On 2015/07/17 19:56:01, cpu wrote:
> maybe add link to the chromium-dev document?

Done. Linked to the bug tracker, which has the chromium-dev document as well as
a link to the chromium-dev discussion.

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_backoff_manager.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:22: // Class that manages
information on Backoff headers. URL requests for HTTP
On 2015/07/17 19:56:01, cpu wrote:
> "url requests for HTTP" --> HTTPS ?

Done.

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_backoff_manager.h:26: //
https://docs.google.com/document/d/1aAxwXK7Vw3VigFd6MmrItbAIgMdKAf-XxXXbhWXdI...
On 2015/07/17 19:56:01, cpu wrote:
> Getting error trying to open that.
> 
> com.google.apps.docs.error.DocumentNotFoundException: Invalid or missing
> required AppKey
> 	at
>
com.google.apps.docs.security.DocumentRequiredInterceptor.execute(DocumentRequiredInterceptor.java:36)
> 	at
>
com.google.apps.framework.request.impl.InterceptorInvocation$Http.beforeProceed(InterceptorInvocation.java:135)
> 	at
>
com.google.apps.framework.request.impl.InterceptorInvocation.proceed(InterceptorInvocation.java:166)
> 	at
>
com.google.apps.framework.request.ActionServlet$Dispatcher.dispatch(ActionServlet.java:81)
> 	at
> com.google.apps.framework.request.ActionServlet.service(ActionServlet.java:47)
> 	at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)

Done. Changed permission to no signin required. Could you try again?

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_context.cc:34: backoff_manager_(NULL),
On 2015/07/17 19:56:01, cpu wrote:
> nullptr

Done.

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/1153093002/diff/220001/net/url_request/url_re...
net/url_request/url_request_context.h:187: // May be NULL.
On 2015/07/17 19:56:01, cpu wrote:
> this comment should be the same style as 179 or 193

Done.

[cpu_(ooo_6.6-7.5), 2015-07-22 20:03:27]

lgtm

[xunjieli, 2015-07-22 20:06:27]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2015-07-22 20:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ttuttle@chromium.org
Link to the patchset: https://codereview.chromium.org/1153093002/#ps320001
(title: "Fixed tests")

[commit-bot: I haz the power, 2015-07-22 20:06:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1153093002/320001

[commit-bot: I haz the power, 2015-07-22 22:40:44]

Committed patchset #5 (id:320001)

[commit-bot: I haz the power, 2015-07-22 22:41:34]

Patchset 5 (id:??) landed as
https://crrev.com/3bb781a314aa895fd1eb576689ec4f452fbe0c4b
Cr-Commit-Position: refs/heads/master@{#339961}