Add an opt-out header for HTTP throttling. Never throttle for localhost.

Add an opt-out header for HTTP throttling. Never throttle for localhost.

Added net::IsLocalhost() function to net/base/net_utils.h

Unit tests for the above.  Also fix flakiness in the ReceivedContentMalformed test that was caused by non-zero jitter.

Modify back-off policy to ignore first 4 errors to help avoid back-off from erroneously kicking in on flaky connections.  Make maximum back-off period 15 minutes instead of 60.  Added documentation of results of analyzing behavior this new policy will give.

Add a simple server for manual testing of the throttling feature.

BUG=66062
TEST=net_unittests

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

[Jói, 2011-03-18 19:12:24]

Gentlemen, Happy Friday.

yzshen: Main reviewer, please review everything.

wtc: Please take a quick look to see if IsLocalhost() is in the appropriate
place (net/base/net_util.h)

FYI, next steps after this will be to log to JavaScript console when XHR
requests fail due to throttling, log diagnostic information to net log, and
probably clear throttling history for a given host when the user force-refreshes
a page.  At that point it will be ready to ask web developers who believed there
was a problem with the feature to try again.  Steps beyond that will be some
more refactoring e.g. moving to below the cache layer and making a member of
URLRequestContext.

Have a nice weekend, and see you in MTV next week :)

Cheers,
Jói

[yzshen1, 2011-03-18 22:49:50]

Hi, Joi.

Thanks!

http://codereview.chromium.org/6711046/diff/1001/net/tools/backoff_server/bac...
File net/tools/backoff_server/backoff_server.py (right):

http://codereview.chromium.org/6711046/diff/1001/net/tools/backoff_server/bac...
net/tools/backoff_server/backoff_server.py:40: httpd =
BaseHTTPServer.HTTPServer(('', int(sys.argv[1])), RequestHandler)
It is good if some more comments are added for this file: command line
parameters, special path and query parameters, etc.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:162:
throttling_entry_->UpdateWithResponse(request_info_.url.host(),
A throttling entry is used for a specific URL (and therefore, a specific host).
I think we could avoid passing in the host for each UpdateWithResponse call.
Instead, we can do it when the throttling entry is initialized.
What do you think?

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:115: if (is_backoff_disabled_)
[optional]
We have a enforce_throttling member in the manager to control system-wide
behavior, shall we also test it here?

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:159: return GetTimeNow();
[I don't have a strong opinion here, just to make sure it is intended.]
Please note that this will affect the retry behavior of URLFetcher.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:70: // The manager object's
lifetime must enclose the lifetime of this object.
It is unlikely that the entries will outlive the manager. However, since the
entries are ref-counted, it is not easy to prove this, especially after we
change the manager to be a member of URLRequestContext.

For example, to come to this conclusion, we need to do the following reasoning:
1) Only URLRequestHTTPJob or URLFetcher will hold refs to the entries.
2) They won't outlive the corresponding URLRequestContext.
3) The manager lives as long as the URLRequestContext that contains it.
4) As a result, the manager's lifetime will enclose the lifetime of the entries.

There are a lot of implicit dependencies in the middle. I think it indicates
design flaws.

William suggested that we need to make URLRequestThrottlerEntry *not*
ref-counted. I think that is probably a good idea.
What do you think?

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:71:
URLRequestThrottlerEntry(URLRequestThrottlerManager* manager);
Please use explicit.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:36: if
(opt_out_hosts_.find(host) != opt_out_hosts_.end() || IsLocalhost(host)) {
[Don't have a strong opinion. Just to make sure you have considered it.]
If we disable throttling for localhost, a web developer, who always tests
his/her website on localhost, may have different experience comparing with his
users. He may be unaware of the throttling feature and doesn't do anything about
it. He may not be able to test opt-out headers. Will that cause any problem?

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:40: // not keep entries in
url_entries_ for opted-out sites).
We might still need to keep track of back-off period, because the retry logic in
URLFetcher will need to use the back-off period.

But this is not very important. We may consider to remove the retry logic
entirely as we discussed last time, or simply retry with a fixed interval. The
server won't mind how frequently we retry, because it asks us to disable
throttling at our side.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:44:
MockURLRequestThrottlerEntry(net::URLRequestThrottlerManager* manager)
Make it explicit, please.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:140:
!fake_retry_value_.empty()) {
[minor]
You don't need to test !fake_retry_value_.empty(), because even if it is empty,
it returns the same value as line 147.

[wtc, 2011-03-23 00:06:58]

joi: I only reviewed the three net/base/net_util* files.

net_util.h is the right home for the IsLocalhost function.

I think I found a bug in IsLocalhost.  Please see my comment
marked with "BUG" below.

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2115: if (host == "localhost")
We should also check for "localhost.localdomain",
"localhost6", and "localhost6.localdomain6".

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2121: if (size == 4) {
Nit: we usually use a switch statement for this kind of
if-else-if statement.

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2129: if (!ip_number[15] == 1)
BUG: this should be
  ip_number[15] != 1

(You should add the test case "::2" to the unit test.)

But, can you use the IN6_IS_ADDR_LOOPBACK macro?
This file already uses that macro.

[Jói, 2011-03-23 23:38:24]

Gentlemen, please take another look.  I have responded to your review comments,
and fixed a couple of external tests that were failing.

Cheers,
Jói

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2115: if (host == "localhost")
On 2011/03/23 00:06:58, wtc wrote:
> We should also check for "localhost.localdomain",
> "localhost6", and "localhost6.localdomain6".

Thanks, I was unaware of these.

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2121: if (size == 4) {
On 2011/03/23 00:06:58, wtc wrote:
> Nit: we usually use a switch statement for this kind of
> if-else-if statement.

Done.

http://codereview.chromium.org/6711046/diff/1001/net/base/net_util.cc#newcode...
net/base/net_util.cc:2129: if (!ip_number[15] == 1)
On 2011/03/23 00:06:58, wtc wrote:
> BUG: this should be
>   ip_number[15] != 1
> 
> (You should add the test case "::2" to the unit test.)
> 
> But, can you use the IN6_IS_ADDR_LOOPBACK macro?
> This file already uses that macro.

Thanks, fixed that and added the test case.

I could use IN6_IS_ADDR_LOOPBACK but it would involve rearranging the bytes into
an IN6_ADDR (8 words instead of the 16 bytes of our IPAddressNumber) because a
straight cast would not be endian-safe (I'm not sure we target any big-endian
architectures right now, but with some of the mobile platforms we might?).

Let me know if you still prefer that I use the macro.

http://codereview.chromium.org/6711046/diff/1001/net/tools/backoff_server/bac...
File net/tools/backoff_server/backoff_server.py (right):

http://codereview.chromium.org/6711046/diff/1001/net/tools/backoff_server/bac...
net/tools/backoff_server/backoff_server.py:40: httpd =
BaseHTTPServer.HTTPServer(('', int(sys.argv[1])), RequestHandler)
On 2011/03/18 22:49:51, yzshen1 wrote:
> It is good if some more comments are added for this file: command line
> parameters, special path and query parameters, etc.

Done.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:162:
throttling_entry_->UpdateWithResponse(request_info_.url.host(),
On 2011/03/18 22:49:51, yzshen1 wrote:
> A throttling entry is used for a specific URL (and therefore, a specific
host).
> I think we could avoid passing in the host for each UpdateWithResponse call.
> Instead, we can do it when the throttling entry is initialized.
> What do you think?

Storing the host would increase the required storage, since it is not what we
use as a key for url_entries_.  On the other hand, just creating a GURL object
parses the spec of the URL, and this is already done when the HttpRequestInfo
structure is initialized, so passing the host as a parameter should be very
cheap.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:115: if (is_backoff_disabled_)
On 2011/03/18 22:49:51, yzshen1 wrote:
> [optional]
> We have a enforce_throttling member in the manager to control system-wide
> behavior, shall we also test it here?

I don't think so; it's enforced in URLRequestHttpJob and I'd prefer for now that
IsDuringExponentialBackoff and GetExponentialBackoffReleaseTime always return
the "truth" (which for opted-out domains is "not backing off").

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.cc:159: return GetTimeNow();
On 2011/03/18 22:49:51, yzshen1 wrote:
> [I don't have a strong opinion here, just to make sure it is intended.]
> Please note that this will affect the retry behavior of URLFetcher.

I believe if a site opts out, it's likely because they have problems that
trigger the back-off mechanism inappropriately, in which case returning the
calculated back-off release time would probably be the wrong thing to do (i.e.
it would likely be too long).

I've added a comment explaining this behavior as intended.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:70: // The manager object's
lifetime must enclose the lifetime of this object.
On 2011/03/18 22:49:51, yzshen1 wrote:
> It is unlikely that the entries will outlive the manager. However, since the
> entries are ref-counted, it is not easy to prove this, especially after we
> change the manager to be a member of URLRequestContext.
> 
> For example, to come to this conclusion, we need to do the following
reasoning:
> 1) Only URLRequestHTTPJob or URLFetcher will hold refs to the entries.
> 2) They won't outlive the corresponding URLRequestContext.
> 3) The manager lives as long as the URLRequestContext that contains it.
> 4) As a result, the manager's lifetime will enclose the lifetime of the
entries.
> 
> There are a lot of implicit dependencies in the middle. I think it indicates
> design flaws.
> 
> William suggested that we need to make URLRequestThrottlerEntry *not*
> ref-counted. I think that is probably a good idea.
> What do you think?

I'm not sure about making the entries not ref-counted.  It seems we have two
choices:

a) Leave them ref-counted, which lets us have an interface where you do a lookup
for an entry once (via RegisterRequestUrl) and then use that entry for the
period you need it, which normally is short.

b) Remove ref-counting and force a look-up for every usage.  This would get rid
of ref-counting but would force two calculations per request/response of the
url_id, two lookups in the entry map, etc.

For now, I will leave it refcounted but am adding a TODO to revisit.  I've made
the back-pointer to the manager weak, and the manager now asks the entries to
detach the pointer during its destruction.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:71:
URLRequestThrottlerEntry(URLRequestThrottlerManager* manager);
On 2011/03/18 22:49:51, yzshen1 wrote:
> Please use explicit.

Done.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:36: if
(opt_out_hosts_.find(host) != opt_out_hosts_.end() || IsLocalhost(host)) {
On 2011/03/18 22:49:51, yzshen1 wrote:
> [Don't have a strong opinion. Just to make sure you have considered it.]
> If we disable throttling for localhost, a web developer, who always tests
> his/her website on localhost, may have different experience comparing with his
> users. He may be unaware of the throttling feature and doesn't do anything
about
> it. He may not be able to test opt-out headers. Will that cause any problem?

I'm doing this on request of the web developers in crbug.com/66062.  They can
still test exponential back-off locally by using a non-loopback address to point
to their own machine.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_manager.cc:40: // not keep entries in
url_entries_ for opted-out sites).
On 2011/03/18 22:49:51, yzshen1 wrote:
> We might still need to keep track of back-off period, because the retry logic
in
> URLFetcher will need to use the back-off period.
> 
> But this is not very important. We may consider to remove the retry logic
> entirely as we discussed last time, or simply retry with a fixed interval. The
> server won't mind how frequently we retry, because it asks us to disable
> throttling at our side.

Right, I think as per the comment I added on
URLRequestThrottlerEntry::GetExponentialBackoffReleaseTime, we should not return
the calculated back-off time for hosts that have opted out (instead we should
just return "now").

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:44:
MockURLRequestThrottlerEntry(net::URLRequestThrottlerManager* manager)
On 2011/03/18 22:49:51, yzshen1 wrote:
> Make it explicit, please.

Done.

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:140:
!fake_retry_value_.empty()) {
On 2011/03/18 22:49:51, yzshen1 wrote:
> [minor]
> You don't need to test !fake_retry_value_.empty(), because even if it is
empty,
> it returns the same value as line 147.

Agreed, I just preferred to be explicit rather than rely on that implicit
behavior.  Will leave as is if you don't object.

[yzshen1, 2011-03-24 20:06:37]

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:162:
throttling_entry_->UpdateWithResponse(request_info_.url.host(),
Okay.

On 2011/03/23 23:38:24, Jói wrote:
> On 2011/03/18 22:49:51, yzshen1 wrote:
> > A throttling entry is used for a specific URL (and therefore, a specific
> host).
> > I think we could avoid passing in the host for each UpdateWithResponse call.
> > Instead, we can do it when the throttling entry is initialized.
> > What do you think?
> 
> Storing the host would increase the required storage, since it is not what we
> use as a key for url_entries_.  On the other hand, just creating a GURL object
> parses the spec of the URL, and this is already done when the HttpRequestInfo
> structure is initialized, so passing the host as a parameter should be very
> cheap.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:70: // The manager object's
lifetime must enclose the lifetime of this object.
On 2011/03/23 23:38:24, Jói wrote:
> On 2011/03/18 22:49:51, yzshen1 wrote:
> > It is unlikely that the entries will outlive the manager. However, since the
> > entries are ref-counted, it is not easy to prove this, especially after we
> > change the manager to be a member of URLRequestContext.
> > 
> > For example, to come to this conclusion, we need to do the following
> reasoning:
> > 1) Only URLRequestHTTPJob or URLFetcher will hold refs to the entries.
> > 2) They won't outlive the corresponding URLRequestContext.
> > 3) The manager lives as long as the URLRequestContext that contains it.
> > 4) As a result, the manager's lifetime will enclose the lifetime of the
> entries.
> > 
> > There are a lot of implicit dependencies in the middle. I think it indicates
> > design flaws.
> > 
> > William suggested that we need to make URLRequestThrottlerEntry *not*
> > ref-counted. I think that is probably a good idea.
> > What do you think?
> 
> I'm not sure about making the entries not ref-counted.  It seems we have two
> choices:
> 
> a) Leave them ref-counted, which lets us have an interface where you do a
lookup
> for an entry once (via RegisterRequestUrl) and then use that entry for the
> period you need it, which normally is short.
> 
> b) Remove ref-counting and force a look-up for every usage.  This would get
rid
> of ref-counting but would force two calculations per request/response of the
> url_id, two lookups in the entry map, etc.
> 
> For now, I will leave it refcounted but am adding a TODO to revisit.  I've
made
> the back-pointer to the manager weak, and the manager now asks the entries to
> detach the pointer during its destruction.

Sounds good.

Another thing that might worth considering is to explicitly use WeakPtr<>.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_unittest.cc:140:
!fake_retry_value_.empty()) {
Okay. :)
On 2011/03/23 23:38:24, Jói wrote:
> On 2011/03/18 22:49:51, yzshen1 wrote:
> > [minor]
> > You don't need to test !fake_retry_value_.empty(), because even if it is
> empty,
> > it returns the same value as line 147.
> 
> Agreed, I just preferred to be explicit rather than rely on that implicit
> behavior.  Will leave as is if you don't object.

http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc#newcod...
net/base/net_util.cc:2118: host == "localhost6.localdomain6")
[minor, optional] might be good to change the comment in .h file to reflect this
change.

[Jói, 2011-03-24 22:03:45]

Thanks, please take a final look.

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

http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
net/url_request/url_request_throttler_entry.h:70: // The manager object's
lifetime must enclose the lifetime of this object.
On 2011/03/24 20:06:37, yzshen1 wrote:
> On 2011/03/23 23:38:24, Jói wrote:
> > On 2011/03/18 22:49:51, yzshen1 wrote:
> > > It is unlikely that the entries will outlive the manager. However, since
the
> > > entries are ref-counted, it is not easy to prove this, especially after we
> > > change the manager to be a member of URLRequestContext.
> > > 
> > > For example, to come to this conclusion, we need to do the following
> > reasoning:
> > > 1) Only URLRequestHTTPJob or URLFetcher will hold refs to the entries.
> > > 2) They won't outlive the corresponding URLRequestContext.
> > > 3) The manager lives as long as the URLRequestContext that contains it.
> > > 4) As a result, the manager's lifetime will enclose the lifetime of the
> > entries.
> > > 
> > > There are a lot of implicit dependencies in the middle. I think it
indicates
> > > design flaws.
> > > 
> > > William suggested that we need to make URLRequestThrottlerEntry *not*
> > > ref-counted. I think that is probably a good idea.
> > > What do you think?
> > 
> > I'm not sure about making the entries not ref-counted.  It seems we have two
> > choices:
> > 
> > a) Leave them ref-counted, which lets us have an interface where you do a
> lookup
> > for an entry once (via RegisterRequestUrl) and then use that entry for the
> > period you need it, which normally is short.
> > 
> > b) Remove ref-counting and force a look-up for every usage.  This would get
> rid
> > of ref-counting but would force two calculations per request/response of the
> > url_id, two lookups in the entry map, etc.
> > 
> > For now, I will leave it refcounted but am adding a TODO to revisit.  I've
> made
> > the back-pointer to the manager weak, and the manager now asks the entries
to
> > detach the pointer during its destruction.
> 
> Sounds good.
> 
> Another thing that might worth considering is to explicitly use WeakPtr<>.

Thanks, that's a good suggestion, but seeing as the manager already maintains a
list of all referrers, there was no additional overhead of doing it this way
(normally, the WeakPtr pattern allows you to skip maintaining the list of all
objects that have a reference to you).

http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc#newcod...
net/base/net_util.cc:2118: host == "localhost6.localdomain6")
On 2011/03/24 20:06:37, yzshen1 wrote:
> [minor, optional] might be good to change the comment in .h file to reflect
this
> change.

Done.

[yzshen1, 2011-03-24 22:12:34]

LGTM

Thanks.

On 2011/03/24 22:03:45, Jói wrote:
> Thanks, please take a final look.
> 
>
http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
> File net/url_request/url_request_throttler_entry.h (right):
> 
>
http://codereview.chromium.org/6711046/diff/1001/net/url_request/url_request_...
> net/url_request/url_request_throttler_entry.h:70: // The manager object's
> lifetime must enclose the lifetime of this object.
> On 2011/03/24 20:06:37, yzshen1 wrote:
> > On 2011/03/23 23:38:24, Jói wrote:
> > > On 2011/03/18 22:49:51, yzshen1 wrote:
> > > > It is unlikely that the entries will outlive the manager. However, since
> the
> > > > entries are ref-counted, it is not easy to prove this, especially after
we
> > > > change the manager to be a member of URLRequestContext.
> > > > 
> > > > For example, to come to this conclusion, we need to do the following
> > > reasoning:
> > > > 1) Only URLRequestHTTPJob or URLFetcher will hold refs to the entries.
> > > > 2) They won't outlive the corresponding URLRequestContext.
> > > > 3) The manager lives as long as the URLRequestContext that contains it.
> > > > 4) As a result, the manager's lifetime will enclose the lifetime of the
> > > entries.
> > > > 
> > > > There are a lot of implicit dependencies in the middle. I think it
> indicates
> > > > design flaws.
> > > > 
> > > > William suggested that we need to make URLRequestThrottlerEntry *not*
> > > > ref-counted. I think that is probably a good idea.
> > > > What do you think?
> > > 
> > > I'm not sure about making the entries not ref-counted.  It seems we have
two
> > > choices:
> > > 
> > > a) Leave them ref-counted, which lets us have an interface where you do a
> > lookup
> > > for an entry once (via RegisterRequestUrl) and then use that entry for the
> > > period you need it, which normally is short.
> > > 
> > > b) Remove ref-counting and force a look-up for every usage.  This would
get
> > rid
> > > of ref-counting but would force two calculations per request/response of
the
> > > url_id, two lookups in the entry map, etc.
> > > 
> > > For now, I will leave it refcounted but am adding a TODO to revisit.  I've
> > made
> > > the back-pointer to the manager weak, and the manager now asks the entries
> to
> > > detach the pointer during its destruction.
> > 
> > Sounds good.
> > 
> > Another thing that might worth considering is to explicitly use WeakPtr<>.
> 
> Thanks, that's a good suggestion, but seeing as the manager already maintains
a
> list of all referrers, there was no additional overhead of doing it this way
> (normally, the WeakPtr pattern allows you to skip maintaining the list of all
> objects that have a reference to you).
> 
> http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc
> File net/base/net_util.cc (right):
> 
>
http://codereview.chromium.org/6711046/diff/13001/net/base/net_util.cc#newcod...
> net/base/net_util.cc:2118: host == "localhost6.localdomain6")
> On 2011/03/24 20:06:37, yzshen1 wrote:
> > [minor, optional] might be good to change the comment in .h file to reflect
> this
> > change.
> 
> Done.

[wtc, 2011-03-25 19:06:24]

LGTM.  Note that I didn't review any of the
url_request_throttler* files or the Python
script.  I'm counting on yzshen to review them.

I have some questions and suggested changes below.

Thank you for working on this.

http://codereview.chromium.org/6711046/diff/15002/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/6711046/diff/15002/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc:289:
EXPECT_TRUE(Time::Now() - start_time_ >= TimeDelta::FromMilliseconds(200));
Just curious: is one second too long?  How do we come up with
the expected elapsed time (one second or 200 ms)?

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.cc#newcod...
net/base/net_util.cc:2134: case kIPv6AddressSize: {
Thank you for considering the byte order issue.  We do need
to support both big endian and little endian machines.

IPAddressNumber is defined to be in network byte order.
struct in6_addr is also in network byte order.
(The IN6_ADDR macro is only available on Windows.)
So we can do a straight memcpy:
  struct in6_addr sin6_addr;
  memcpy(&sin6_addr, &ip_number[0], kIPv6AddressSize);
  return IN6_IS_ADDR_LOOPBACK(&sin6_addr);

Note: the copying to a struct in6_addr is necessary to
deal with alignment issue.

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.h#newcode434
net/base/net_util.h:434: // the above that have been mapped back to the local
machine, i.e.
Nit: "mapped back to the local machine" is a little confusing.
Do you mean IP addresses of the local machine?  Or do you
mean /etc/hosts can map an IP address to 127.0.0.1?

http://codereview.chromium.org/6711046/diff/15002/net/tools/backoff_server/ba...
File net/tools/backoff_server/backoff_server.py (right):

http://codereview.chromium.org/6711046/diff/15002/net/tools/backoff_server/ba...
net/tools/backoff_server/backoff_server.py:1: #!/usr/bin/python2.4
Should we add backoff_server.py to src/net/tools/testserver
instead?  I see other types of servers (for Sync and XMPP)
in that directory.

[Jói, 2011-03-25 21:26:51]

Thanks for the review.

I've addressed your comments and uploaded a new version.  I will submit once
trybots look good.

Cheers,
Jói

http://codereview.chromium.org/6711046/diff/15002/chrome/service/cloud_print/...
File chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/6711046/diff/15002/chrome/service/cloud_print/...
chrome/service/cloud_print/cloud_print_url_fetcher_unittest.cc:289:
EXPECT_TRUE(Time::Now() - start_time_ >= TimeDelta::FromMilliseconds(200));
On 2011/03/25 19:06:24, wtc wrote:
> Just curious: is one second too long?  How do we come up with
> the expected elapsed time (one second or 200 ms)?

It's dependent on the default exponential back-off parameters, which I am
modifying in this change to be more lenient, in particular we now ignore the
first few errors.

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.cc#newcod...
net/base/net_util.cc:2134: case kIPv6AddressSize: {
On 2011/03/25 19:06:24, wtc wrote:
> Thank you for considering the byte order issue.  We do need
> to support both big endian and little endian machines.
> 
> IPAddressNumber is defined to be in network byte order.
> struct in6_addr is also in network byte order.
> (The IN6_ADDR macro is only available on Windows.)
> So we can do a straight memcpy:
>   struct in6_addr sin6_addr;
>   memcpy(&sin6_addr, &ip_number[0], kIPv6AddressSize);
>   return IN6_IS_ADDR_LOOPBACK(&sin6_addr);
> 
> Note: the copying to a struct in6_addr is necessary to
> deal with alignment issue.

Done.

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.h
File net/base/net_util.h (right):

http://codereview.chromium.org/6711046/diff/15002/net/base/net_util.h#newcode434
net/base/net_util.h:434: // the above that have been mapped back to the local
machine, i.e.
On 2011/03/25 19:06:24, wtc wrote:
> Nit: "mapped back to the local machine" is a little confusing.
> Do you mean IP addresses of the local machine?  Or do you
> mean /etc/hosts can map an IP address to 127.0.0.1?

Yes, I mean via /etc/hosts or by the host admin making other IPv6 addresses into
loopback addresses.  I've simplified the comment to simply state:


// Note that this function does not check for IP addresses other than
// the above, although other IP addresses may point to the local
// machine.

http://codereview.chromium.org/6711046/diff/15002/net/tools/backoff_server/ba...
File net/tools/backoff_server/backoff_server.py (right):

http://codereview.chromium.org/6711046/diff/15002/net/tools/backoff_server/ba...
net/tools/backoff_server/backoff_server.py:1: #!/usr/bin/python2.4
On 2011/03/25 19:06:24, wtc wrote:
> Should we add backoff_server.py to src/net/tools/testserver
> instead?  I see other types of servers (for Sync and XMPP)
> in that directory.

Done.

[wtc, 2011-03-28 22:08:19]

http://codereview.chromium.org/6711046/diff/19007/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/6711046/diff/19007/net/base/net_util.cc#newcod...
net/base/net_util.cc:2137: return IN6_IS_ADDR_LOOPBACK(&sin6_addr) != FALSE;
The != FALSE part should be unnecessary.  I know why you
added it.  I've seen Chromium code that uses !! for this
purpose:
    return !!IN6_IS_ADDR_LOOPBACK(&sin6_addr);
which we should use only as a last resort.

Also, FALSE may not be defined on non-Windows platforms.

[Jói, 2011-03-29 14:46:15]

Ah, OK, will fix that today.  The Windows compiler complains of forced
cast to bool without this, so I probably will need to use the !!
trick.

Cheers,
Jói


On Mon, Mar 28, 2011 at 6:08 PM,  <wtc@chromium.org> wrote:
>
> http://codereview.chromium.org/6711046/diff/19007/net/base/net_util.cc
> File net/base/net_util.cc (right):
>
>
http://codereview.chromium.org/6711046/diff/19007/net/base/net_util.cc#newcod...
> net/base/net_util.cc:2137: return IN6_IS_ADDR_LOOPBACK(&sin6_addr) !=
> FALSE;
> The != FALSE part should be unnecessary.  I know why you
> added it.  I've seen Chromium code that uses !! for this
> purpose:
>    return !!IN6_IS_ADDR_LOOPBACK(&sin6_addr);
> which we should use only as a last resort.
>
> Also, FALSE may not be defined on non-Windows platforms.
>
> http://codereview.chromium.org/6711046/
>