Anti-DDoS enhancements: Log to net log, UMA stats, improved policy.

Anti-DDoS enhancements: Log to net log, UMA stats, improved policy.

Count only certain error codes as failures. Don't count malformed body
for codes already counted failures. Start with fresh entry when
outdated, which makes policy slightly less aggressive.

Add registration/unregistration to NetworkChangeNotifier, and reset
throttling state on network changes, to avoid degenerate corner cases.

Add UMA stats for throttling, and unit tests for them.

Add hash-location-only URL to unit test.

Enable testing of AJAX behavior via backoff_server.

BUG=83775
TEST=net_unittests

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

[Jói, 2011-05-24 20:12:16]

Yuzhu: Please look at the whole change.

Sanjeev: Please look at the changes under chrome/ and provide owners approval
for the changes under chrome/service.

Cheers,
Jói

[Jói, 2011-05-24 20:12:59]

Forgot to say:  I will add a net/OWNERS reviewer once we've done an initial
pass; probably wtc@ since eroman@ is away.

Cheers,
Jói


On 2011/05/24 20:12:16, Jói wrote:
> Yuzhu: Please look at the whole change.
> 
> Sanjeev: Please look at the changes under chrome/ and provide owners approval
> for the changes under chrome/service.
> 
> Cheers,
> Jói

[sanjeevr, 2011-05-24 22:16:19]

LGTM for chrome/ and chrome/service/ changes.

[yzshen1, 2011-05-25 03:49:06]

Hi, Joi.

The change of counting only certain 5xx codes as failures will potentially
affect the behavior of URLFetcher. URLFetcher regards all 5xx codes as failures
and can be set to automatically retry when it encounters 5xx. As a result, if
URLFetcher keeps receiving, say, 502, it will retry without exponential back-off
(it still respects the sliding window constraint, though).
I am not sure whether it is important, just want to make sure you have
considered it.

Thanks! :)

http://codereview.chromium.org/6966038/diff/4001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6966038/diff/4001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:209: }
Please add "// namespace net"

http://codereview.chromium.org/6966038/diff/4001/chrome/common/net/url_fetche...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6966038/diff/4001/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.cc:845: if (status_code == -1) {
Might be better to use kInvalidHttpResponseCode.

http://codereview.chromium.org/6966038/diff/4001/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/base/backoff_entry.cc#ne...
net/base/backoff_entry.cc:20: // Can't use GetTimeNow() as it's virtual.
Please change the comment to ImplGetTimeNow().
(Line 45 also has the same issue.)

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1030: //     "host":         <The hostname
back-off was disabled for>
Please fix the spaces after ':'.

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1038: //     "release_after_ms":  <Number of
milliseconds until URL will be unblocked>
[minor] It should be one space after ':'. Other lines could align with it.

http://codereview.chromium.org/6966038/diff/4001/net/tools/testserver/backoff...
File net/tools/testserver/backoff_server.py (right):

http://codereview.chromium.org/6966038/diff/4001/net/tools/testserver/backoff...
net/tools/testserver/backoff_server.py:27: function Fetch() {
I think it should be 'f'etch?

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:82: int retry_after_ms)
Wrong indent.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:191:
NetLog::TYPE_THROTTLING_REJECTED_REQUEST,
Wrong indent.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:311:
last_successful_response_time_ = base::TimeTicks::Now();
It would be good to comment about why ImplGetTimeNow is not used here.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:408: base::TimeTicks now =
base::TimeTicks::Now();
Is it intended to not use ImplGetTimeNow here?

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:85: void OnIPAddressChanged()
OVERRIDE;
Might be good to add 'virtual'.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:88: void
OnOnlineStateChanged(bool online) OVERRIDE;
ditto.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_unittest.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:37: TimeTicks ImplGetTimeNow()
const OVERRIDE {
It might be good to add 'virtual'.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:326: << "When given a negative
value, it should not change the release_time";
Please also update the string.

[Jói, 2011-05-25 14:30:35]

Wan-Teh, I'm adding you for net/OWNERS approval.  Sanjeev and Yuzhu have already
done one round of reviews.

Yuzhu, thanks for the feedback.  I've responded to your review comments and
uploaded a new version, see below.

I looked at the all 5xx codes vs. specific 5xx codes issue you mentioned with
URLFetcher (Sanjeev had mentioned it in a previous review).  The change seems
very low risk, since before this, there would be no back-off delay anyway for
the first several errors, and there is no user of URLFetcher's retry mechanism
that retries more than 4 or 5 times.  I believe adding to back-off time for the
remaining 5xx codes would probably introduce a high risk of false positives, so
I think it's better we skip them.

I also considered whether to make URLFetcher retry only on the same status codes
that we count for exponential back-off, but that change seems unnecessary.  You
might get e.g. a 502 or 504 once or twice because e.g. one of the 10 servers
behind a load-balancing reverse proxy is down, but in this case retrying would
make perfect sense (and the load-balancing reverse proxy would likely stop
directing traffic to the bad server soon anyway).

Cheers,
Jói

http://codereview.chromium.org/6966038/diff/4001/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6966038/diff/4001/chrome/browser/browser_main....
chrome/browser/browser_main.cc:209: }
On 2011/05/25 03:49:06, yzshen1 wrote:
> Please add "// namespace net"

Done.

http://codereview.chromium.org/6966038/diff/4001/chrome/common/net/url_fetche...
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6966038/diff/4001/chrome/common/net/url_fetche...
chrome/common/net/url_fetcher.cc:845: if (status_code == -1) {
On 2011/05/25 03:49:06, yzshen1 wrote:
> Might be better to use kInvalidHttpResponseCode.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/base/backoff_entry.cc#ne...
net/base/backoff_entry.cc:20: // Can't use GetTimeNow() as it's virtual.
On 2011/05/25 03:49:06, yzshen1 wrote:
> Please change the comment to ImplGetTimeNow().
> (Line 45 also has the same issue.)

Done.

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1030: //     "host":         <The hostname
back-off was disabled for>
On 2011/05/25 03:49:06, yzshen1 wrote:
> Please fix the spaces after ':'.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/base/net_log_event_type_...
net/base/net_log_event_type_list.h:1038: //     "release_after_ms":  <Number of
milliseconds until URL will be unblocked>
On 2011/05/25 03:49:06, yzshen1 wrote:
> [minor] It should be one space after ':'. Other lines could align with it.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/tools/testserver/backoff...
File net/tools/testserver/backoff_server.py (right):

http://codereview.chromium.org/6966038/diff/4001/net/tools/testserver/backoff...
net/tools/testserver/backoff_server.py:27: function Fetch() {
On 2011/05/25 03:49:06, yzshen1 wrote:
> I think it should be 'f'etch?

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:82: int retry_after_ms)
On 2011/05/25 03:49:06, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:191:
NetLog::TYPE_THROTTLING_REJECTED_REQUEST,
On 2011/05/25 03:49:06, yzshen1 wrote:
> Wrong indent.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:311:
last_successful_response_time_ = base::TimeTicks::Now();
On 2011/05/25 03:49:06, yzshen1 wrote:
> It would be good to comment about why ImplGetTimeNow is not used here.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:408: base::TimeTicks now =
base::TimeTicks::Now();
On 2011/05/25 03:49:06, yzshen1 wrote:
> Is it intended to not use ImplGetTimeNow here?

No, thanks for catching that.  I am writing tests in my next change that will
assert on perceived downtime in a simulated scenario - you may have saved me
from scratching my head over why they were failing :)

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.h (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:85: void OnIPAddressChanged()
OVERRIDE;
On 2011/05/25 03:49:06, yzshen1 wrote:
> Might be good to add 'virtual'.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:88: void
OnOnlineStateChanged(bool online) OVERRIDE;
On 2011/05/25 03:49:06, yzshen1 wrote:
> ditto.

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
File net/url_request/url_request_throttler_unittest.cc (right):

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:37: TimeTicks ImplGetTimeNow()
const OVERRIDE {
On 2011/05/25 03:49:06, yzshen1 wrote:
> It might be good to add 'virtual'. 

Done.

http://codereview.chromium.org/6966038/diff/4001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:326: << "When given a negative
value, it should not change the release_time";
On 2011/05/25 03:49:06, yzshen1 wrote:
> Please also update the string.

This was incorrect, I've fixed it now (the call to UpdateWithResponse with a
negative time header was missing).

[yzshen1, 2011-05-25 18:45:39]

LGTM

Thanks!

[wtc, 2011-05-25 23:12:04]

LGTM.

I only reviewed the changes to src/net.

I didn't read the histogram code and unit tests carefully.

I marked my important comments with "IMPORTANT".  You can
ignore the "Nit" comments.

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.cc#ne...
net/base/backoff_entry.cc:20: // Can't use ImplGetTimeNow() as it's virtual.
IMPORTANT: I wonder if the constructor should just initialize
exponential_backoff_release_time_ to 0 or some dummy value,
and we use Reset() to initialize exponential_backoff_release_time_
properly.

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h
File net/base/backoff_entry.h (right):

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h#new...
net/base/backoff_entry.h:76: int failure_count() const;
Nit: define this getter inline?

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h#new...
net/base/backoff_entry.h:80: virtual base::TimeTicks ImplGetTimeNow() const;
Nit: we usually put "Impl" at the end, so "GetTimeNowImpl"?

http://codereview.chromium.org/6966038/diff/8002/net/tools/testserver/backoff...
File net/tools/testserver/backoff_server.py (right):

http://codereview.chromium.org/6966038/diff/8002/net/tools/testserver/backoff...
net/tools/testserver/backoff_server.py:53: '''
IMPORTANT: AJAX_TEST_PAGE is missing the closing </html> tag.

Is that intentional?

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:14: #include "base/time.h"
Nit: url_request_throttler_entry.h already includes
"base/time.h"

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.h (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:119: virtual base::TimeTicks
ImplGetTimeNow() const;
Nit: ImplGetTimeNow => GetTimeNowImpl ?

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:130: void
HandleMetricsTracking(int status_code);
Nit: use 'response_code' or 'status_code' as the input
parameter name consistently.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:13: #include
"net/base/network_change_notifier.h"
Nit: This header is already included by url_request_throttler_manager.h.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.h (left):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:36: class NET_API
URLRequestThrottlerManager
IMPORTANT: this is a merging error.  Please add back the
NET_API.

[Jói, 2011-05-26 14:53:42]

Thanks for the review Wan-Teh. The changes are very small, so I plan to commit
based on the LGTMs I've received, after trybots pass.

Cheers,
Jói

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.cc
File net/base/backoff_entry.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.cc#ne...
net/base/backoff_entry.cc:20: // Can't use ImplGetTimeNow() as it's virtual.
On 2011/05/25 23:12:06, wtc wrote:
> IMPORTANT: I wonder if the constructor should just initialize
> exponential_backoff_release_time_ to 0 or some dummy value,
> and we use Reset() to initialize exponential_backoff_release_time_
> properly.

Good point, I fixed this so that the constructor calls Reset() and I modified
Reset() to not use the virtual function; it now sets the backoff release time as
follows, with this comment:

  // We leave exponential_backoff_release_time_ unset, meaning 0. We could
  // initialize to ImplGetTimeNow() but because it's a virtual method it's
  // not safe to call in the constructor (and the constructor calls Reset()).
  // The effects are the same, i.e. ShouldRejectRequest() will return false
  // right after Reset().
  exponential_backoff_release_time_ = base::TimeTicks();

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h
File net/base/backoff_entry.h (right):

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h#new...
net/base/backoff_entry.h:76: int failure_count() const;
On 2011/05/25 23:12:06, wtc wrote:
> Nit: define this getter inline?

Done.

http://codereview.chromium.org/6966038/diff/8002/net/base/backoff_entry.h#new...
net/base/backoff_entry.h:80: virtual base::TimeTicks ImplGetTimeNow() const;
On 2011/05/25 23:12:06, wtc wrote:
> Nit: we usually put "Impl" at the end, so "GetTimeNowImpl"?

I've been using Impl at the front for methods intended for unit tests to
override. Will leave as-is in this change, but if you think it's best, I can go
through (these files and several others) and rename these methods so that Impl
is at the end.

http://codereview.chromium.org/6966038/diff/8002/net/tools/testserver/backoff...
File net/tools/testserver/backoff_server.py (right):

http://codereview.chromium.org/6966038/diff/8002/net/tools/testserver/backoff...
net/tools/testserver/backoff_server.py:53: '''
On 2011/05/25 23:12:06, wtc wrote:
> IMPORTANT: AJAX_TEST_PAGE is missing the closing </html> tag.
> 
> Is that intentional?

Not intentional. Fixed.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:14: #include "base/time.h"
On 2011/05/25 23:12:06, wtc wrote:
> Nit: url_request_throttler_entry.h already includes
> "base/time.h"

Done.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_entry.h (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:130: void
HandleMetricsTracking(int status_code);
On 2011/05/25 23:12:06, wtc wrote:
> Nit: use 'response_code' or 'status_code' as the input
> parameter name consistently.

Switched to response_code.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.cc (right):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:13: #include
"net/base/network_change_notifier.h"
On 2011/05/25 23:12:06, wtc wrote:
> Nit: This header is already included by url_request_throttler_manager.h.

Done.

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
File net/url_request/url_request_throttler_manager.h (left):

http://codereview.chromium.org/6966038/diff/8002/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.h:36: class NET_API
URLRequestThrottlerManager
On 2011/05/25 23:12:06, wtc wrote:
> IMPORTANT: this is a merging error.  Please add back the
> NET_API.

Fixed. Thanks for catching that.

[Jói, 2011-05-26 17:04:49]

Adding jar@ for chrome/browser/net/OWNERS approval.

Cheers,
Jói

[Jói, 2011-05-26 23:38:57]

Wan-Teh, could you also look at the two files under chrome/browser/net
files?  Jim said you would be an appropriate reviewer for it.

Cheers,
Jói


On Thu, May 26, 2011 at 1:04 PM,  <joi@chromium.org> wrote:
> Adding jar@ for chrome/browser/net/OWNERS approval.
>
> Cheers,
> Jói
>
>
> http://codereview.chromium.org/6966038/
>

[wtc, 2011-05-27 17:35:18]

LGTM.

jar: I am not an OWNER of chrome/browser/net, so you need
to review those two files.

http://codereview.chromium.org/6966038/diff/16001/net/url_request/url_request...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/6966038/diff/16001/net/url_request/url_request...
net/url_request/url_request_throttler_entry_interface.h:55: virtual void
ReceivedContentWasMalformed(int status_code) = 0;
Nit: status_code => response_code ?

[jar (doing other things), 2011-05-27 17:42:19]

Per wtc's review, I'll LGTM (approve) this assuming you fix the nit he
identified.

(I've also put in a CL to make wtc an owner going forward)

[Jói, 2011-05-27 18:49:10]

Thanks Wan-Teh and Jim.

http://codereview.chromium.org/6966038/diff/16001/net/url_request/url_request...
File net/url_request/url_request_throttler_entry_interface.h (right):

http://codereview.chromium.org/6966038/diff/16001/net/url_request/url_request...
net/url_request/url_request_throttler_entry_interface.h:55: virtual void
ReceivedContentWasMalformed(int status_code) = 0;
On 2011/05/27 17:35:19, wtc wrote:
> Nit: status_code => response_code ?

Done, sorry I thought I had caught them all before.