Issue 2487001: Request Throttler : Exponential back-off

Jói

http://codereview.chromium.org/2487001/diff/6001/7001 File net/url_request/network_safety.cc (right): http://codereview.chromium.org/2487001/diff/6001/7001#newcode1 net/url_request/network_safety.cc:1: // Copyright (c) 2006-2009 The Chromium Authors. All rights ...

4 years, 12 months ago (2010-06-02 18:48:16 UTC) #1

malavv

I have made adjustment you recommended and inserted some comments about what should we use ...

4 years, 12 months ago (2010-06-02 20:43:04 UTC) #2

I have made adjustment you recommended and inserted some comments about what
should we use as a key to map entries.

http://codereview.chromium.org/2487001/diff/6001/7001
File net/url_request/network_safety.cc (right):

http://codereview.chromium.org/2487001/diff/6001/7001#newcode1
net/url_request/network_safety.cc:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
On 2010/06/02 18:48:18, Jói wrote:
> 2010

Done.

http://codereview.chromium.org/2487001/diff/6001/7001#newcode28
net/url_request/network_safety.cc:28: NetworkSafetyEntry::~NetworkSafetyEntry()
{
On 2010/06/02 18:48:18, Jói wrote:
> use one blank line between function definitions

Done.

http://codereview.chromium.org/2487001/diff/6001/7001#newcode37
net/url_request/network_safety.cc:37: 
On 2010/06/02 18:48:18, Jói wrote:
> no blank lines before closing blocks

Done.

http://codereview.chromium.org/2487001/diff/6001/7001#newcode110
net/url_request/network_safety.cc:110: NetworkSafetyEntry*
NetworkSafetyManager::RegisterHost
On 2010/06/02 18:48:18, Jói wrote:
> I doubt that the exponential back-off registry should be per host; consider
how
> many services (and many separate server types) are behind e.g.
http://www.google.com
> 
> Probably, we should use either all of the URL (least chance of unwanted
> interactions between services, but maybe slower to "catch on" to the need to
do
> exponential back-off), or all of the URL except the query parameters
(everything
> after ?).
> 
> I'd suggest checking what Toolbar and/or Desktop are doing and match that.

After checking both implementation, they both take a CString url parameter and
use this to map the urlProtectionEntries.

Most of the time when those function are use in the code base, they are passed
"host + path" which is what you suggested.

I will in my correction make this member function take a GURL as a parameter and
normalise the url by keeping only the host and path and lowering its case.

As I tested the module I have seen some problem with this implementation.
For one, many new websites use web server to map their query in URL so we would
be saving query parameter which would cause a privacy problem.
Also, and on the same note, I when I tested I have seen safesearch appear a lot
and being register a couple of time. It seems to put a session token or
something in it's url path. If this token is different for every usage for
everyone, this module would not be able to protect safesearch from being DDoS
cause it would always be getting a new url_id.

Finally, if the user let chrome open for a while it would basically be an ever
growing map and end up using quite some space.

http://codereview.chromium.org/2487001/diff/6001/7001#newcode111
net/url_request/network_safety.cc:111: (const std::string &hostName) {
On 2010/06/02 18:48:18, Jói wrote:
> hostName -> host_name

Done.

http://codereview.chromium.org/2487001/diff/6001/7002
File net/url_request/network_safety.h (right):

http://codereview.chromium.org/2487001/diff/6001/7002#newcode1
net/url_request/network_safety.h:1: // Copyright (c) 2006-2009 The Chromium
Authors. All rights reserved.
On 2010/06/02 18:48:18, Jói wrote:
> 2006-2009 -> 2010

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode17
net/url_request/network_safety.h:17: class NetworkSafetyEntry {
On 2010/06/02 18:48:18, Jói wrote:
> Just FYI, I know this is prototypical, but in terms of documentation, classes
> need a sentence or two on their responsibilities, plus sometimes on how to use
> them.

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode28
net/url_request/network_safety.h:28: int64 UpdateEntryValues(EventType
event_type);
On 2010/06/02 18:48:18, Jói wrote:
> Again FYI, non-obvious methods need a comment explaining how to use, which
> inputs would be valid or invalid inputs (if any), what the return values mean,
> etc.

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode31
net/url_request/network_safety.h:31: typedef std::queue<base::TimeTicks>
RequestLog;
On 2010/06/02 18:48:18, Jói wrote:
> Again FYI, I like to see an invariant of data for each class member, or for
> groups of class members together when appropriate.

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode42
net/url_request/network_safety.h:42: void resetEntryValues();
On 2010/06/02 18:48:18, Jói wrote:
> resetEntryValues -> ResetEntryValues

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode44
net/url_request/network_safety.h:44: static EntryFct updateFct[3];
On 2010/06/02 18:48:18, Jói wrote:
> update_fct_

Done.

http://codereview.chromium.org/2487001/diff/6001/7002#newcode45
net/url_request/network_safety.h:45: static const int kSliding_window_period_ms;
On 2010/06/02 18:48:18, Jói wrote:
> our constant naming is very confusing; it is basically kCamelCaps so this
should
> be kSlidingWindowPeriodMs, same thing for the rest below.

Done.

http://codereview.chromium.org/2487001/diff/6001/7003
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/6001/7003#newcode94
net/url_request/url_request_http_job.cc:94:
safetyEntry_(Singleton<NetworkSafetyManager>::get()->
On 2010/06/02 18:48:18, Jói wrote:
> safetyEntry_ -> safety_entry_

Done.

http://codereview.chromium.org/2487001/diff/6001/7003#newcode567
net/url_request/url_request_http_job.cc:567: if ( !is_cached_content_ ) {
On 2010/06/02 18:48:18, Jói wrote:
> no space after ( or before )

Done.

http://codereview.chromium.org/2487001/diff/6001/7003#newcode623
net/url_request/url_request_http_job.cc:623: int64 backoff =
safetyEntry_->UpdateEntryValues(NetworkSafetyEntry::SEND);
On 2010/06/02 18:48:18, Jói wrote:
> this is never used?

True, for the moment I have been unable to find how to use is to delay the
request. In some higher class they have been using (thread)->postDelayedTask(
... ) but I am really not sure that I can use that here. This is the main reason
of the mail to Darin, but I have still no answer on that point.

Sign in to reply to this message.

Jói

http://codereview.chromium.org/2487001/diff/6001/7001 File net/url_request/network_safety.cc (right): http://codereview.chromium.org/2487001/diff/6001/7001#newcode110 net/url_request/network_safety.cc:110: NetworkSafetyEntry* NetworkSafetyManager::RegisterHost > I will in my correction make ...

4 years, 12 months ago (2010-06-02 22:18:48 UTC) #3

http://codereview.chromium.org/2487001/diff/6001/7001
File net/url_request/network_safety.cc (right):

http://codereview.chromium.org/2487001/diff/6001/7001#newcode110
net/url_request/network_safety.cc:110: NetworkSafetyEntry*
NetworkSafetyManager::RegisterHost
> I will in my correction make this member function take a GURL as a parameter
and
> normalise the url by keeping only the host and path and lowering its case.

Sounds good.

> As I tested the module I have seen some problem with this implementation.
> For one, many new websites use web server to map their query in URL so we
would
> be saving query parameter which would cause a privacy problem.

Agreed; there is not much we can do about this but when a service is truly down,
the problem should sort itself out in the end - if repeated requests are being
made they will usually have the same parameters so they would start backing off.

There is no privacy problem though, you are not sending the URLs anywhere or
storing them to disk, they just stick around in the profile until it is
destroyed, just as they stick around in the navigation (back/forward) history
even in incognito mode.

> Also, and on the same note, I when I tested I have seen safesearch appear a
lot
> and being register a couple of time. It seems to put a session token or
> something in it's url path. If this token is different for every usage for
> everyone, this module would not be able to protect safesearch from being DDoS
> cause it would always be getting a new url_id.

True, this is an edge case.  I'm not sure if we should try to handle it.

> Finally, if the user let chrome open for a while it would basically be an ever
> growing map and end up using quite some space.

You can remove entries whose timestamp-at-which-next-request-may-be-made is in
the past from the map in an opportunistic fashion (e.g. during other calls to
your code, and then perhaps only 20% of the time so you don't add overhead to
every invocation).

http://codereview.chromium.org/2487001/diff/6001/7003
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/6001/7003#newcode623
net/url_request/url_request_http_job.cc:623: int64 backoff =
safetyEntry_->UpdateEntryValues(NetworkSafetyEntry::SEND);
On 2010/06/02 20:43:04, malavv wrote:
> On 2010/06/02 18:48:18, Jói wrote:
> > this is never used?
> 
> True, for the moment I have been unable to find how to use is to delay the
> request. In some higher class they have been using (thread)->postDelayedTask(
> ... ) but I am really not sure that I can use that here. This is the main
reason
> of the mail to Darin, but I have still no answer on that point.
> 

The backoff mechanism shouldn't delay the request at all, it should simply deny
it if it is made too soon.  It is up to the client code to retry the request
later.  To the client code it should appear either as if the network connection
is down or the destination server is down.

Sign in to reply to this message.

Jói

First round of comments, will look again when you're finished making the changes you mentioned ...

4 years, 11 months ago (2010-06-08 20:28:04 UTC) #5

First round of comments, will look again when you're finished making the changes
you mentioned verbally.

http://codereview.chromium.org/2487001/diff/35001/36003
File net/url_request/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/35001/36003#newcode1
net/url_request/network_safety_unittest.cc:1: // Copyright (c) 2006-2010 The
Chromium Authors. All rights reserved.
just 2010 (I think)

http://codereview.chromium.org/2487001/diff/35001/36003#newcode23
net/url_request/network_safety_unittest.cc:23: void
AddEventToSendLog(base::TimeTicks time = base::TimeTicks::Now());
seems weird to me to default to concrete Now() implementation when in the tests
you are faking the time

http://codereview.chromium.org/2487001/diff/35001/36003#newcode27
net/url_request/network_safety_unittest.cc:27: virtual base::TimeTicks
time_now();
In the base class (and thus here) this shouldn't follow the (IMHO silly) naming
convention of naming accessors for the variable they are accessing since in the
base class it is not accessing a variable but rather making an out-call to the
TimeTicks code.  So the name should be GetCurrentTime() or some such

http://codereview.chromium.org/2487001/diff/35001/36003#newcode52
net/url_request/network_safety_unittest.cc:52: }
no indent within the namespace

can use blank lines after opening brace and before closing brace to delinate;
also it is customary to add a comment on the closing brace to indicate that you
are closing a namespace; in this case the line would be 

}  // namespace

whereas with a namespace named Xyz it would be

}  // namespace Xyz

http://codereview.chromium.org/2487001/diff/35001/36003#newcode53
net/url_request/network_safety_unittest.cc:53:
MockNetworkSafetyEntry::MockNetworkSafetyEntry()
use one blank line between top-level elements

http://codereview.chromium.org/2487001/diff/35001/36003#newcode55
net/url_request/network_safety_unittest.cc:55: {}
the opening brace should be at the end of the preceding line

not sure if lint would have caught this, but please run it or check the lint
results after doing the g4 upload (they are on the codereview.chromium.org issue
page)

http://codereview.chromium.org/2487001/diff/35001/36003#newcode121
net/url_request/network_safety_unittest.cc:121: EXPECT_EQ(false,
entry.UpdateEntryValues(test_value[0]));
seems like these 4 lines could be written as a for loop

http://codereview.chromium.org/2487001/diff/35001/36003#newcode163
net/url_request/network_safety_unittest.cc:163: for ( int i = 3; i < 6; ++i ) {
no space after ( or before )

http://codereview.chromium.org/2487001/diff/35001/36003#newcode235
net/url_request/network_safety_unittest.cc:235:
std::string("http://www.example.com/")),
suggest indenting these to match the opening brace, i.e.

std::make_pair(GURL(...),
               std::string(...)),
std::make_pair(GURL(...),
               std::string(...)),
etc.

http://codereview.chromium.org/2487001/diff/35001/36003#newcode236
net/url_request/network_safety_unittest.cc:236:
std::make_pair(GURL("http://www.Example.com"),
-4 spaces indent

http://codereview.chromium.org/2487001/diff/35001/36003#newcode247
net/url_request/network_safety_unittest.cc:247: const int max =
sizeof(test_values)/sizeof(test_values[0]);
instead, you can use either arraysize(test_values) (preferred) or
ARRAYSIZE(test_values) inline in the for statement

http://codereview.chromium.org/2487001/diff/35001/36003#newcode254
net/url_request/network_safety_unittest.cc:254: TEST(NetworkSafetyManager,
areEntriesBeingCollected ) {
use one blank line between top-level functions

http://codereview.chromium.org/2487001/diff/35001/36003#newcode268
net/url_request/network_safety_unittest.cc:268: EXPECT_EQ(3 ,
manager.GetNumberOfEntries());
no space before ,   (here and elsewhere)

Sign in to reply to this message.

malavv

I have fixed the comments I was talking about and have increase general readability of ...

4 years, 11 months ago (2010-06-08 21:51:03 UTC) #6

Jói

Some more quick comments based on the diff to the latest patch set. I will ...

4 years, 11 months ago (2010-06-09 17:57:08 UTC) #7

Jói

More comments. We might want to have a chat about some of them before I ...

4 years, 11 months ago (2010-06-09 18:33:55 UTC) #8

More comments. We might want to have a chat about some of them before I leave.

http://codereview.chromium.org/2487001/diff/42002/52002
File net/url_request/network_safety.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52002#newcode14
net/url_request/network_safety.cc:14: NetworkSafetyEntry::EntryFct
NetworkSafetyEntry::update_fct_[3] = {
I think you can remove this indirection, especially if you make the interface
change I am suggesting in the client file and move more of the logic into this
class

http://codereview.chromium.org/2487001/diff/42002/52002#newcode19
net/url_request/network_safety.cc:19: const int
NetworkSafetyEntry::kFailureThreshold = 5;
With this window, it would seem you might have a request that is expected to be
done only once per day, that starts being done 4.9 times every 2 minutes and
would stay under the radar forever with no exponential backoff.  This would be
bad if the server was provisioned for one request per day per client, rather
than 4.9*30*24 = 3528 requests per day per client.  Once you've fixed this,
please add a unit test that would catch regressions in a case like that.

Why not start backing off immediately after 1 failed request?  Our initial
backoff is only 700ms (per next line) and then doubles every time it fails.

In general we should tune these parameters based on comparison with the Toolbar
and Desktop implementations since they are proven to work.

http://codereview.chromium.org/2487001/diff/42002/52002#newcode145
net/url_request/network_safety.cc:145: if ( !(requests_counter_ %
kRequestBetweenCollecting) )
prefer == 0 to ! in this case

http://codereview.chromium.org/2487001/diff/42002/52002#newcode185
net/url_request/network_safety.cc:185: while ( i != host_entries_.end() ) {
no space after ( or before )

please make sure you fix all cases of this

http://codereview.chromium.org/2487001/diff/42002/52003
File net/url_request/network_safety.h (right):

http://codereview.chromium.org/2487001/diff/42002/52003#newcode19
net/url_request/network_safety.h:19: // Class to abstract an entry of the
Network Safety Manager
"Represents one entry in the NetworkSafetyManager."

("class" is redundant and so is "abstract")

http://codereview.chromium.org/2487001/diff/42002/52003#newcode20
net/url_request/network_safety.h:20: class NetworkSafetyEntry {
it would be good to minimize the public interface to just what clients need; per
my comments in other files it might be just:

IsRequestAllowed()
UpdateFromResponse(response)

// optional - this is if we want application-level
// detection of failures
BadResponse()

http://codereview.chromium.org/2487001/diff/42002/52003#newcode65
net/url_request/network_safety.h:65: static const int kAdditionalCstMs;
the "Cst" abbreviation is non-obvious, suggest not abbreviating

http://codereview.chromium.org/2487001/diff/42002/52003#newcode75
net/url_request/network_safety.h:75: // Queue to store times at which requests
to an host failed.
"an host" -> "a host"

http://codereview.chromium.org/2487001/diff/42002/52003#newcode107
net/url_request/network_safety.h:107: // Are we currently delaying request
isn't this redundant with release_time_ (I imagine if we are not delaying a
request, release_time_ is set to 0 or some such? - and that needs to be
documented in its invariant of data)

http://codereview.chromium.org/2487001/diff/42002/52003#newcode110
net/url_request/network_safety.h:110: static Lock lock_;
document whether this is needed before accessing any member variable, or only
specific variables

http://codereview.chromium.org/2487001/diff/42002/52004
File net/url_request/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52004#newcode107
net/url_request/network_safety_unittest.cc:107: TEST(NetworkSafetyEntry,
illegalEventType) {
illegalEventType -> IllegalEventType

fix capitalization of first letter on all other test names below

http://codereview.chromium.org/2487001/diff/42002/52005
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52005#newcode568
net/url_request/url_request_http_job.cc:568: if
(response_info_->headers->HasHeader(
The way this is currently written, each additional client (e.g. a new project)
using the network safety component would need a block of code similar to this
one.  This seems like one of the points where instead of the client having code
to check for the header, retrieve it and parse it, we should move that code into
the network safety component.  The client should need only to provide an object
that conforms to an interface, with something like the following methods:
- HasHeader
- GetNormalizedHeader

One way to accomplish this is to simply define (in the network safety component)
an abstract base interface that the client needs to implement.  To make it
simple for Chrome, we could have a simple adapter class that implements this
base interface, given a headers object (as returned by response_info_->headers).

This block of code could then possibly be transformed to something like:

if (!is_cached_content_) {
  safety_entry_->UpdateFromResponse(
      NetworkSafetyHeadersAdapter(
          response_info_->headers));
}

(Note that NetworkSafetyHeadersAdapter would be Chrome-specific code, outside
the network safety component).

Some of this refactoring can wait and be done in a larger move to place the
network safety code in its own component so that it can be more easily reused by
other projects.  If we do that, let's add TODOs to the code and start a document
somewhere about outstanding items for that work.  I'm definitely not asking that
you actually move the code to somewhere reusable at this point - we will need to
discuss the where and how of that.  I would however suggest introducing the
abstract base "headers access" interface now, so that you can move this block of
logic into the network safety code.

http://codereview.chromium.org/2487001/diff/42002/52005#newcode634
net/url_request/url_request_http_job.cc:634: // Return value
Just name it return_value or result instead of the comment, or skip the comment.

http://codereview.chromium.org/2487001/diff/42002/52005#newcode638
net/url_request/url_request_http_job.cc:638:
safety_entry_->UpdateEntryValues(NetworkSafetyEntry::SEND);
suggest adding a utility method so you can write:

if (safety_entry_->IsRequestAllowed())

http://codereview.chromium.org/2487001/diff/42002/52005#newcode640
net/url_request/url_request_http_job.cc:640: if ( is_request_allowed ) {
no space after ( or before )

Sign in to reply to this message.

Jói

Adding Siggi as a reviewer since it looks like this may need to land while ...

4 years, 11 months ago (2010-06-09 19:59:33 UTC) #9

malavv

Hello Joi, and now Siggi. I just finish applying all the changes that you proposed ...

4 years, 11 months ago (2010-06-10 03:29:53 UTC) #10

Hello Joi, and now Siggi.

I just finish applying all the changes that you proposed today/yesterday. The
test suite is partially updated but I focused more on refactoring the network
safety component for you to have a look before your holidays thus giving you
work until the very last day. As you see, I am only doing my part to make you
appreciate more your holidays.

Happy Holidays,
Maxime

http://codereview.chromium.org/2487001/diff/42002/52002
File net/url_request/network_safety.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52002#newcode39
net/url_request/network_safety.cc:39: // You should not be here.
On 2010/06/09 17:57:11, Jói wrote:
> Make this a DCHECK instead of a comment.

Done.

http://codereview.chromium.org/2487001/diff/42002/52002#newcode138
net/url_request/network_safety.cc:138: const unsigned int
NetworkSafetyManager::kRequestBetweenCollecting = 200;
On 2010/06/09 17:57:11, Jói wrote:
> Request -> Requests

Done.

http://codereview.chromium.org/2487001/diff/42002/52002#newcode145
net/url_request/network_safety.cc:145: if ( !(requests_counter_ %
kRequestBetweenCollecting) )
On 2010/06/09 18:33:55, Jói wrote:
> prefer == 0 to ! in this case

Done.

http://codereview.chromium.org/2487001/diff/42002/52002#newcode185
net/url_request/network_safety.cc:185: while ( i != host_entries_.end() ) {
On 2010/06/09 18:33:55, Jói wrote:
> no space after ( or before )
> 
> please make sure you fix all cases of this

Done.

http://codereview.chromium.org/2487001/diff/42002/52003
File net/url_request/network_safety.h (right):

http://codereview.chromium.org/2487001/diff/42002/52003#newcode19
net/url_request/network_safety.h:19: // Class to abstract an entry of the
Network Safety Manager
On 2010/06/09 18:33:55, Jói wrote:
> "Represents one entry in the NetworkSafetyManager."
> 
> ("class" is redundant and so is "abstract")

Done.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode27
net/url_request/network_safety.h:27: // Please remind to increase kEnumSize each
time a new entry is added
On 2010/06/09 17:57:11, Jói wrote:
> Instead of this, it is more maintainable if you add a final item to the enum
> e.g. ENUM_SIZE with a comment behind it "used for boundary checking, make sure
> this is the last item"

Done.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode65
net/url_request/network_safety.h:65: static const int kAdditionalCstMs;
On 2010/06/09 18:33:55, Jói wrote:
> the "Cst" abbreviation is non-obvious, suggest not abbreviating

Done.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode75
net/url_request/network_safety.h:75: // Queue to store times at which requests
to an host failed.
On 2010/06/09 18:33:55, Jói wrote:
> "an host" -> "a host"

Done.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode86
net/url_request/network_safety.h:86: //   Return value indicates if this request
can be made (or were delayed).
On 2010/06/09 17:57:11, Jói wrote:
> "can be made (or were delayed)"  -> "is allowed to be made at this time"

Done.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode107
net/url_request/network_safety.h:107: // Are we currently delaying request
On 2010/06/09 18:33:55, Jói wrote:
> isn't this redundant with release_time_ (I imagine if we are not delaying a
> request, release_time_ is set to 0 or some such? - and that needs to be
> documented in its invariant of data)

The value of release_time is often set to now in this class. For the redundancy,
the reason why I have chosen this is that we can be incrementing back-off to say
3rd level and stop making request until the delay passes and than retry if it
fails we want to go to the 4th level of back-off even though release_time_ is
less than now.

http://codereview.chromium.org/2487001/diff/42002/52003#newcode119
net/url_request/network_safety.h:119: // Manager class that keeps a map of hosts
with their corresponding entry.
On 2010/06/09 17:57:11, Jói wrote:
> Hosts? I thought we were indexing by URL (minus query parameters)?

Done.

http://codereview.chromium.org/2487001/diff/42002/52004
File net/url_request/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52004#newcode32
net/url_request/network_safety_unittest.cc:32: void release_time(base::TimeTicks
time) { release_time_ = time; }
On 2010/06/09 17:57:11, Jói wrote:
> set_release_time

Done.

http://codereview.chromium.org/2487001/diff/42002/52004#newcode107
net/url_request/network_safety_unittest.cc:107: TEST(NetworkSafetyEntry,
illegalEventType) {
On 2010/06/09 18:33:55, Jói wrote:
> illegalEventType -> IllegalEventType
> 
> fix capitalization of first letter on all other test names below

Done.

http://codereview.chromium.org/2487001/diff/42002/52005
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/42002/52005#newcode634
net/url_request/url_request_http_job.cc:634: // Return value
On 2010/06/09 18:33:55, Jói wrote:
> Just name it return_value or result instead of the comment, or skip the
comment.

Done.

http://codereview.chromium.org/2487001/diff/42002/52005#newcode640
net/url_request/url_request_http_job.cc:640: if ( is_request_allowed ) {
On 2010/06/09 18:33:55, Jói wrote:
> no space after ( or before )

Done.

Sign in to reply to this message.

malavv

Hi Siggi, I have completed rewriting my test for the new public interface and finished ...

4 years, 11 months ago (2010-06-10 20:02:56 UTC) #11

Jói

Looking better. Only had time to do a quick scan, will leave more details for ...

4 years, 11 months ago (2010-06-10 20:26:56 UTC) #12

Looking better.  Only had time to do a quick scan, will leave more details for
Siggi :)

Cheers,
Jói

http://codereview.chromium.org/2487001/diff/66002/45003
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45003#newcode31
net/network_safety/network_safety_entry.cc:31: int64 unused_since, lifespanMs;
lifespanMs -> lifespan_ms

also, I think a more conventional style for us would be to declare the type
right at the start of lines 32 and 33, where you assign the values; that way
they are also never unassigned (and thus arbitrary)

http://codereview.chromium.org/2487001/diff/66002/45003#newcode56
net/network_safety/network_safety_entry.cc:56: // We have taken ownership of
this object and so, we need to delete it.
a) I'm not sure taking ownership is the right approach here. It seems trivial to
have the caller own the object; that way the caller can use either an automatic
or a heap-allocated object at its discretion.

b) If you do take ownership, that needs to be documented at the interface level,
i.e. in the header file.

http://codereview.chromium.org/2487001/diff/66002/45004
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/66002/45004#newcode39
net/network_safety/network_safety_entry.h:39: static const int
kEntryLivingTimeSec;
hmm... it probably needs to live for some amount of time that is a ratio of the
exponential backoff time, _after_ the exponential backoff time has passed. 
Otherwise we could have an error, and then you retry it 1 ms after the
exponential backoff has passed and you have another error, at which point there
is no doubling of the exponential backoff timeout.

Sorry if my earlier comments were confusing on this point, I think I may have
said something like "it's outdated as soon as the backoff interval has passed"
which of course is wrong :), you need it quite a bit longer than that (maybe
e.g. the earlier of 3 more backoff intervals or when the request succeeds).

http://codereview.chromium.org/2487001/diff/66002/45005
File net/network_safety/network_safety_header_adapter.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45005#newcode8
net/network_safety/network_safety_header_adapter.cc:8: const
scoped_refptr<net::HttpResponseHeaders>& chrome_header)
+2 indent

Sign in to reply to this message.

siggi1

Some initial comments, let's have a chat on threading model and safety. http://codereview.chromium.org/2487001/diff/66002/45008 File net/network_safety/network_safety_manager.cc ...

4 years, 11 months ago (2010-06-11 13:06:09 UTC) #13

malavv

Fixed race conditions and initial code review stuff http://codereview.chromium.org/2487001/diff/66002/45003 File net/network_safety/network_safety_entry.cc (right): http://codereview.chromium.org/2487001/diff/66002/45003#newcode31 net/network_safety/network_safety_entry.cc:31: int64 ...

4 years, 11 months ago (2010-06-11 18:02:49 UTC) #14

Fixed race conditions and initial code review stuff

http://codereview.chromium.org/2487001/diff/66002/45003
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45003#newcode31
net/network_safety/network_safety_entry.cc:31: int64 unused_since, lifespanMs;
On 2010/06/10 20:26:57, Jói wrote:
> lifespanMs -> lifespan_ms
> 
> also, I think a more conventional style for us would be to declare the type
> right at the start of lines 32 and 33, where you assign the values; that way
> they are also never unassigned (and thus arbitrary)

Done.

http://codereview.chromium.org/2487001/diff/66002/45003#newcode56
net/network_safety/network_safety_entry.cc:56: // We have taken ownership of
this object and so, we need to delete it.
On 2010/06/10 20:26:57, Jói wrote:
> a) I'm not sure taking ownership is the right approach here. It seems trivial
to
> have the caller own the object; that way the caller can use either an
automatic
> or a heap-allocated object at its discretion.
> 
> b) If you do take ownership, that needs to be documented at the interface
level,
> i.e. in the header file.

Done.

http://codereview.chromium.org/2487001/diff/66002/45004
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/66002/45004#newcode39
net/network_safety/network_safety_entry.h:39: static const int
kEntryLivingTimeSec;
On 2010/06/10 20:26:57, Jói wrote:
> hmm... it probably needs to live for some amount of time that is a ratio of
the
> exponential backoff time, _after_ the exponential backoff time has passed. 
> Otherwise we could have an error, and then you retry it 1 ms after the
> exponential backoff has passed and you have another error, at which point
there
> is no doubling of the exponential backoff timeout.
> 
> Sorry if my earlier comments were confusing on this point, I think I may have
> said something like "it's outdated as soon as the backoff interval has passed"
> which of course is wrong :), you need it quite a bit longer than that (maybe
> e.g. the earlier of 3 more backoff intervals or when the request succeeds).

The main goal of using this constant is to have a pool of recent requests. If we
outdate those requests as soon as they succeed, normal successful request would
need to be regenerated every time there is a garbage collection occurring.
Currently, entries reset their release_time, to at least now, each time they are
used and therefore would not get collected if they request resources on a fairly
quit basis.

I think that the name may be confusing, this constant is the amount of time
needed after the release_time is passed for the request to be considered
outdated and not the plain maximum living time.

http://codereview.chromium.org/2487001/diff/66002/45005
File net/network_safety/network_safety_header_adapter.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45005#newcode8
net/network_safety/network_safety_header_adapter.cc:8: const
scoped_refptr<net::HttpResponseHeaders>& chrome_header)
On 2010/06/10 20:26:57, Jói wrote:
> +2 indent

Done.

http://codereview.chromium.org/2487001/diff/66002/45008
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45008#newcode18
net/network_safety/network_safety_manager.cc:18: AutoLock lock(lock_);
On 2010/06/11 13:06:09, siggi1 wrote:
> the presence of this lock suggests that this method can be entered on multiple
> threads, but I don't see how the following code is threadsafe, let's discuss.

Done.

http://codereview.chromium.org/2487001/diff/66002/45009
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/66002/45009#newcode18
net/network_safety/network_safety_manager.h:18: class NetworkSafetyManager {
On 2010/06/11 13:06:09, siggi1 wrote:
> I'm not clear on the overall purpose of this class, nor its threading model,
can
> you clarify in comments a bit, please?

Done.

http://codereview.chromium.org/2487001/diff/66002/45009#newcode23
net/network_safety/network_safety_manager.h:23: // matches with this Url id and
create it if it does not exist.
On 2010/06/11 13:06:09, siggi1 wrote:
> nit: create -> creates

Done.

http://codereview.chromium.org/2487001/diff/66002/45009#newcode26
net/network_safety/network_safety_manager.h:26: void
NotifyRequestBodyWasMalformed(const GURL& url);
On 2010/06/11 13:06:09, siggi1 wrote:
> comment on the purpose of this method, please?

Done.

http://codereview.chromium.org/2487001/diff/66002/45009#newcode39
net/network_safety/network_safety_manager.h:39: // Method that allow us to
standardize the representation of an Url in our
On 2010/06/11 13:06:09, siggi1 wrote:
> comment to 
> 
> Standardizes (or canonicalizes) the Url representation.

Done.

http://codereview.chromium.org/2487001/diff/66002/45009#newcode58
net/network_safety/network_safety_manager.h:58:
DISALLOW_EVIL_CONSTRUCTORS(NetworkSafetyManager);
On 2010/06/11 13:06:09, siggi1 wrote:
> DISALLOW_COPY_AND_ASSIGNMENT is the current way to specify this.

Done.

http://codereview.chromium.org/2487001/diff/66002/45010
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45010#newcode17
net/network_safety/network_safety_unittest.cc:17: class
MockNetworkSafetyHeaderAdapter : public NetworkSafetyHeaderInterface {
On 2010/06/11 13:06:09, siggi1 wrote:
> code inside the anonymous namespace is not indented, so indent -2

Done.

http://codereview.chromium.org/2487001/diff/66002/45010#newcode71
net/network_safety/network_safety_unittest.cc:71:
MockNetworkSafetyHeaderAdapter::MockNetworkSafetyHeaderAdapter()
On 2010/06/11 13:06:09, siggi1 wrote:
> move this into the anonymous namespace?

Done.

http://codereview.chromium.org/2487001/diff/66002/45010#newcode77
net/network_safety/network_safety_unittest.cc:77: :
fake_retry_value_(retry_value),
On 2010/06/11 13:06:09, siggi1 wrote:
> fix indent 

Done.

http://codereview.chromium.org/2487001/diff/66002/45011
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/66002/45011#newcode569
net/url_request/url_request_http_job.cc:569: safety_entry_->UpdateWithResponse(
On 2010/06/11 13:06:09, siggi1 wrote:
> I don't understand how this isn't racy - e.g. why couldn't this entry have
been
> garbage collected in the time since it was created?

Done.

http://codereview.chromium.org/2487001/diff/66002/45011#newcode639
net/url_request/url_request_http_job.cc:639: return_value =
net::ERR_CONNECTION_ABORTED;
On 2010/06/11 13:06:09, siggi1 wrote:
> would it make sense to have a more descriptive error value here?

Done.

http://codereview.chromium.org/2487001/diff/66002/45012
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/2487001/diff/66002/45012#newcode90
net/url_request/url_request_http_job.h:90: NetworkSafetyEntry* safety_entry_;
On 2010/06/11 13:06:09, siggi1 wrote:
> one line comment on the purpose of this variable.

Done.

Sign in to reply to this message.

malavv

Hi Siggi, I have resolved problems you pointed me and check for the real utility ...

4 years, 11 months ago (2010-06-14 17:17:50 UTC) #15

siggi1

Some additional comments, will keep going with a detailed review later this morning. http://codereview.chromium.org/2487001/diff/90003/109003 File ...

4 years, 11 months ago (2010-06-15 14:02:12 UTC) #16

Sigurður Ásgeirsson

looking good, still a few nits :). http://codereview.chromium.org/2487001/diff/117001/118003 File net/network_safety/network_safety_entry.cc (right): http://codereview.chromium.org/2487001/diff/117001/118003#newcode43 net/network_safety/network_safety_entry.cc:43: SaveState(); you ...

4 years, 11 months ago (2010-06-16 13:57:51 UTC) #17

looking good, still a few nits :).

http://codereview.chromium.org/2487001/diff/117001/118003
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118003#newcode43
net/network_safety/network_safety_entry.cc:43: SaveState();
you may be saving a state here where there is no backoff, and the
ReceivedContentWasMalformed may revert to that state - e.g. reduce the backoff. 
It would be a better idea to never allow failure of any sort to revise the
backoff down, as any failure condition that allows downward revision of the
backoff may lend itself to malicious exploitation. 
We discussed the case where malicious software can engineer to cause a downward
revision through replaying content from cache.

http://codereview.chromium.org/2487001/diff/117001/118008
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118008#newcode16
net/network_safety/network_safety_manager.cc:16: DCHECK_EQ(MessageLoop::TYPE_IO,
MessageLoop::current()->type()) <<
this check does not guarantee the single-threaded use of this class, but merely
that the class is only used from threads of type IO. I suggest capturing
MessageLoop::current() in the  constructor, then asserting on all accesses being
from the same message loop in this method.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode19
net/network_safety/network_safety_manager.cc:19: // Garbage collection if
necessary
Comments should be imperative, full sentences and end with a full stop:
// Periodically garbage collect old entries.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode21
net/network_safety/network_safety_manager.cc:21: if ((requests_counter_ %
kRequestsBetweenCollecting) == 0)
modulus is still a fairly expensive operation, I suggest resetting the counter,
or keeping a separate garbage collection counter if you want a running tally of
all requests.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode24
net/network_safety/network_safety_manager.cc:24: // Normalization of the url
-> // Normalize the url.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode27
net/network_safety/network_safety_manager.cc:27: // Creates a new entry to the
map. Std insert on a map will create a new
I suspect you leak the NetworkSafetyEntry here when the entry already exists in
the map. The std::pair's second element is going to be of type
NetworkSafetyEntry*.
Also doing a spurious allocation for every lookup is a little evil. I suggest
splitting this up into a find() and insert, or simply indexing into the map and
testing for a NULL  NetworkSafetyEntry, e.g. 
UrlEntryMap::value_type& entry = url_entries_[url_id];
if (entry.second... == NULL) 
  ...

http://codereview.chromium.org/2487001/diff/117001/118008#newcode52
net/network_safety/network_safety_manager.cc:52: UrlEntryMap::iterator i =
url_entries_.begin();
Assert on single-threadedness here too. While this is only called from
RegisterRequestUrl now, there's no guarantee that a maintainer won't change that
down the line.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode66
net/network_safety/network_safety_manager.cc:66: // Normalization of the url
// Normalization -> Normalize the url.
assert on threadedness here.

http://codereview.chromium.org/2487001/diff/117001/118009
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/117001/118009#newcode19
net/network_safety/network_safety_manager.h:19: // Class that register a safety
entry for each Url being access in order to
register->registers
being access->accessed

http://codereview.chromium.org/2487001/diff/117001/118009#newcode21
net/network_safety/network_safety_manager.h:21: // manager on each request. Must
permit simultaneous access.
I don't understand what "Must permit simultaneous access" means here, can you
please clarify or remove comment?

http://codereview.chromium.org/2487001/diff/117001/118009#newcode24
net/network_safety/network_safety_manager.h:24: // Must be call on each request.
Return value is the safety entry that is
Must be called for every request, returns ... 

I'm pretty sure this method ought to return a ref_pointer per chrome conventions
for using ref counted objects. 

It might be good to describe the caller's further responsibilities here, or
refer to documentation in NetworkSafetyEntry.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode47
net/network_safety/network_safety_manager.h:47: // Method that allow us to
transform an url into a id that is useful and
allow->allows

http://codereview.chromium.org/2487001/diff/117001/118009#newcode49
net/network_safety/network_safety_manager.h:49: // the query string and
fragment.
the host name should be normal cased, but is that justifiable for the request
path?

http://codereview.chromium.org/2487001/diff/117001/118009#newcode52
net/network_safety/network_safety_manager.h:52: // Method that ensure the map
gets cleaned from time to time. The period at
ensure->ensures

http://codereview.chromium.org/2487001/diff/117001/118009#newcode58
net/network_safety/network_safety_manager.h:58: 
remove blank line.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode59
net/network_safety/network_safety_manager.h:59: // This keeps track of how many
request has been made for used with
has been made for used -> have been made. Used

http://codereview.chromium.org/2487001/diff/117001/118010
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118010#newcode52
net/network_safety/network_safety_unittest.cc:52: release_time_ = release_time;
indent -1

Sign in to reply to this message.

malavv

- Did change the way that state and back-off value is kept in order to ...

4 years, 11 months ago (2010-06-16 17:25:34 UTC) #18

- Did change the way that state and back-off value is kept in order to implement
what we talked about.

- Corrected memory leak on insert and the scoped_refptr return value.

- + nits correction

http://codereview.chromium.org/2487001/diff/117001/118003
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118003#newcode43
net/network_safety/network_safety_entry.cc:43: SaveState();
On 2010/06/16 13:57:51, Ruðrugis wrote:
> you may be saving a state here where there is no backoff, and the
> ReceivedContentWasMalformed may revert to that state - e.g. reduce the
backoff. 
> It would be a better idea to never allow failure of any sort to revise the
> backoff down, as any failure condition that allows downward revision of the
> backoff may lend itself to malicious exploitation. 
> We discussed the case where malicious software can engineer to cause a
downward
> revision through replaying content from cache. 

I resolve this by only allowing bodyWasMalformed to regenerate state if past
state has more back-off than the current one. I also made sure that even though
we do not delay outgoing requests on success, we keep track of the back-off
value and exponentially diminish it in order to keep stability with multiple
malformed bodies.

http://codereview.chromium.org/2487001/diff/117001/118008
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118008#newcode16
net/network_safety/network_safety_manager.cc:16: DCHECK_EQ(MessageLoop::TYPE_IO,
MessageLoop::current()->type()) <<
On 2010/06/16 13:57:51, Ruðrugis wrote:
> this check does not guarantee the single-threaded use of this class, but
merely
> that the class is only used from threads of type IO. I suggest capturing
> MessageLoop::current() in the  constructor, then asserting on all accesses
being
> from the same message loop in this method.  

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode19
net/network_safety/network_safety_manager.cc:19: // Garbage collection if
necessary
On 2010/06/16 13:57:51, Ruðrugis wrote:
> Comments should be imperative, full sentences and end with a full stop:
> // Periodically garbage collect old entries.

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode21
net/network_safety/network_safety_manager.cc:21: if ((requests_counter_ %
kRequestsBetweenCollecting) == 0)
On 2010/06/16 13:57:51, Ruðrugis wrote:
> modulus is still a fairly expensive operation, I suggest resetting the
counter,
> or keeping a separate garbage collection counter if you want a running tally
of
> all requests.

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode24
net/network_safety/network_safety_manager.cc:24: // Normalization of the url
On 2010/06/16 13:57:51, Ruðrugis wrote:
> -> // Normalize the url.

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode27
net/network_safety/network_safety_manager.cc:27: // Creates a new entry to the
map. Std insert on a map will create a new
On 2010/06/16 13:57:51, Ruðrugis wrote:
> I suspect you leak the NetworkSafetyEntry here when the entry already exists
in
> the map. The std::pair's second element is going to be of type
> NetworkSafetyEntry*.
> Also doing a spurious allocation for every lookup is a little evil. I suggest
> splitting this up into a find() and insert, or simply indexing into the map
and
> testing for a NULL  NetworkSafetyEntry, e.g. 
> UrlEntryMap::value_type& entry = url_entries_[url_id];
> if (entry.second... == NULL) 
>   ... 

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode52
net/network_safety/network_safety_manager.cc:52: UrlEntryMap::iterator i =
url_entries_.begin();
On 2010/06/16 13:57:51, Ruðrugis wrote:
> Assert on single-threadedness here too. While this is only called from
> RegisterRequestUrl now, there's no guarantee that a maintainer won't change
that
> down the line.

Done.

http://codereview.chromium.org/2487001/diff/117001/118008#newcode66
net/network_safety/network_safety_manager.cc:66: // Normalization of the url
On 2010/06/16 13:57:51, Ruðrugis wrote:
> // Normalization -> Normalize the url.
> assert on threadedness here.

Done.

http://codereview.chromium.org/2487001/diff/117001/118009
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/117001/118009#newcode19
net/network_safety/network_safety_manager.h:19: // Class that register a safety
entry for each Url being access in order to
On 2010/06/16 13:57:51, Ruðrugis wrote:
> register->registers
> being access->accessed
> 

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode21
net/network_safety/network_safety_manager.h:21: // manager on each request. Must
permit simultaneous access.
On 2010/06/16 13:57:51, Ruðrugis wrote:
> I don't understand what "Must permit simultaneous access" means here, can you
> please clarify or remove comment?

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode24
net/network_safety/network_safety_manager.h:24: // Must be call on each request.
Return value is the safety entry that is
On 2010/06/16 13:57:51, Ruðrugis wrote:
> Must be called for every request, returns ... 
> 
> I'm pretty sure this method ought to return a ref_pointer per chrome
conventions
> for using ref counted objects. 
> 
> It might be good to describe the caller's further responsibilities here, or
> refer to documentation in NetworkSafetyEntry.

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode47
net/network_safety/network_safety_manager.h:47: // Method that allow us to
transform an url into a id that is useful and
On 2010/06/16 13:57:51, Ruðrugis wrote:
> allow->allows

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode49
net/network_safety/network_safety_manager.h:49: // the query string and
fragment.
On 2010/06/16 13:57:51, Ruðrugis wrote:
> the host name should be normal cased, but is that justifiable for the request
> path?

URLs on a Windows Web server are sometimes case-insensitive. Therefore an
attacker could bypass our protection by only changing the case of a letter in
the url each time he makes a request.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode52
net/network_safety/network_safety_manager.h:52: // Method that ensure the map
gets cleaned from time to time. The period at
On 2010/06/16 13:57:51, Ruðrugis wrote:
> ensure->ensures

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode58
net/network_safety/network_safety_manager.h:58: 
On 2010/06/16 13:57:51, Ruðrugis wrote:
> remove blank line.

Done.

http://codereview.chromium.org/2487001/diff/117001/118009#newcode59
net/network_safety/network_safety_manager.h:59: // This keeps track of how many
request has been made for used with
On 2010/06/16 13:57:51, Ruðrugis wrote:
> has been made for used -> have been made. Used

Done.

http://codereview.chromium.org/2487001/diff/117001/118010
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/117001/118010#newcode52
net/network_safety/network_safety_unittest.cc:52: release_time_ = release_time;
On 2010/06/16 13:57:51, Ruðrugis wrote:
> indent -1

Done.

Sign in to reply to this message.

Sigurður Ásgeirsson

A few more nits. http://codereview.chromium.org/2487001/diff/126001/127001 File net/base/net_error_list.h (right): http://codereview.chromium.org/2487001/diff/126001/127001#newcode160 net/base/net_error_list.h:160: // request on a server ...

4 years, 11 months ago (2010-06-17 19:54:38 UTC) #19

malavv

Resolved issues pointed out by your last code review. Also tighten the interface of networkSafetyEntry ...

4 years, 11 months ago (2010-06-18 19:45:59 UTC) #20

Resolved issues pointed out by your last code review. Also tighten the interface
of networkSafetyEntry by using an interface.

http://codereview.chromium.org/2487001/diff/126001/127001
File net/base/net_error_list.h (right):

http://codereview.chromium.org/2487001/diff/126001/127001#newcode160
net/base/net_error_list.h:160: // request on a server that is unavailable.
On 2010/06/17 19:54:38, Ruðrugis wrote:
> Might be better to name the module you're writing more explicitly:
> 
> // The network safety module cancelled this request ... 
> // requests to a server that is failing requests. 

Done.

http://codereview.chromium.org/2487001/diff/126001/127003
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/126001/127003#newcode14
net/network_safety/network_safety_entry.cc:14: const int
NetworkSafetyEntry::kEntryLivingTimeSec = 600;
On 2010/06/17 19:54:38, Ruðrugis wrote:
> So the cache is going to contain an entry for every request URL made in the
last
> 5 minute interval, whether it succeeded or failed?

Sorry, used to be 2 minutes but guess I changed it for debug reasons. Your
comment made me realize that currently managed request should not be allowed to
be collected unless they are older than the maximum allowed back-off.

http://codereview.chromium.org/2487001/diff/126001/127003#newcode49
net/network_safety/network_safety_entry.cc:49: // We slowly decay the number of
time delayed instead of resetting it to 0
On 2010/06/17 19:54:38, Ruðrugis wrote:
> number of time -> number of times

Done.

http://codereview.chromium.org/2487001/diff/126001/127003#newcode50
net/network_safety/network_safety_entry.cc:50: // in order to stay stable if we
received lots of malformed request at the
On 2010/06/17 19:54:38, Ruðrugis wrote:
> lots of requests with malformed bodies?

Done.

http://codereview.chromium.org/2487001/diff/126001/127003#newcode54
net/network_safety/network_safety_entry.cc:54: release_time_ =
std::max(TimeTicks::Now(), release_time_);
On 2010/06/17 19:54:38, Ruðrugis wrote:
> I'm confused here, we've just had a success reported and we don't want to
delay
> the next request. Why not simply set the release time to GetTimeNow() to let
the
> next request through. If there's an error, you've persisted the backoff
> magnitude in the num_of_time_delayed_, and so your error backoff time can
decay
> without retarding the success case?

We spoked about keeping request until the back-off horizon.

http://codereview.chromium.org/2487001/diff/126001/127003#newcode101
net/network_safety/network_safety_entry.cc:101: // malformed body in cache an
replay it to decrease delay.
On 2010/06/17 19:54:38, Ruðrugis wrote:
> an -> and

Done.

http://codereview.chromium.org/2487001/diff/126001/127004
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/126001/127004#newcode20
net/network_safety/network_safety_entry.h:20: // Represent an entry of the
Network Safety Manager
On 2010/06/17 19:54:38, Ruðrugis wrote:
> Represent->Represents
> end comments with a full stop.

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode30
net/network_safety/network_safety_entry.h:30: // To be call on each requests.
Informs us of whether we should stop it or not
On 2010/06/17 19:54:38, Ruðrugis wrote:
> call->called
> // Returns true iff the request is allowed.
> // If the request is not allowed, the caller should (???)

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode33
net/network_safety/network_safety_entry.h:33: // Each time a response is
received, we need to inform this module with it.
On 2010/06/17 19:54:38, Ruðrugis wrote:
> inform ... with -> inform ... of

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode40
net/network_safety/network_safety_entry.h:40: // bodies are registered as
success this is why we keep a memento to keep last
On 2010/06/17 19:54:38, Ruðrugis wrote:
> as success this -> as success, which

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode41
net/network_safety/network_safety_entry.h:41: // state in order to switch back
if we are inform that it were corrupted. This
On 2010/06/17 19:54:38, Ruðrugis wrote:
> inform -> informed
> it were -> it was

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode43
net/network_safety/network_safety_entry.h:43: // this will corrupt event the
memento by putting last state to success.
On 2010/06/17 19:54:38, Ruðrugis wrote:
> corrupt event->corrupt even
> 
> Is this still true?

Done.

http://codereview.chromium.org/2487001/diff/126001/127004#newcode50
net/network_safety/network_safety_entry.h:50: static const int
kEntryLivingTimeSec;
On 2010/06/17 19:54:38, Ruðrugis wrote:
> kEntryLifetimeSec?

Done.

http://codereview.chromium.org/2487001/diff/126001/127008
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/126001/127008#newcode15
net/network_safety/network_safety_manager.cc:15: (const GURL &url) {
On 2010/06/17 19:54:38, Ruðrugis wrote:
> -> ...::RegisterRequestUrl(
>    const GURL...

Done.

http://codereview.chromium.org/2487001/diff/126001/127008#newcode17
net/network_safety/network_safety_manager.cc:17: "The current MessageLoop must
be the same as the one it were created on.";
On 2010/06/17 19:54:38, Ruðrugis wrote:
> This assertion is common enough that you don't need a message.

Done.

http://codereview.chromium.org/2487001/diff/126001/127008#newcode21
net/network_safety/network_safety_manager.cc:21: if (requests_counter_ >=
kRequestsBetweenCollecting)
On 2010/06/17 19:54:38, Ruðrugis wrote:
> reset the counter after GC?

It was done within the GC function, but for clarity i will pu it there.

http://codereview.chromium.org/2487001/diff/126001/127008#newcode30
net/network_safety/network_safety_manager.cc:30: url_entries_[url_id] = new
NetworkSafetyEntry();
On 2010/06/17 19:54:38, Ruðrugis wrote:
> as entry is a reference to the map, you can assign straight to it.

Done.

http://codereview.chromium.org/2487001/diff/126001/127008#newcode31
net/network_safety/network_safety_manager.cc:31: entry = url_entries_[url_id];
On 2010/06/17 19:54:38, Ruðrugis wrote:
> this is a self-assignment, as the entry is a reference.

Done.

http://codereview.chromium.org/2487001/diff/126001/127009
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/126001/127009#newcode12
net/network_safety/network_safety_manager.h:12: #include "base/message_loop.h"
On 2010/06/17 19:54:38, Ruðrugis wrote:
> you can forward declare MessageLoop if you move the constructor to the .cc
file.

Done.

http://codereview.chromium.org/2487001/diff/126001/127009#newcode38
net/network_safety/network_safety_manager.h:38: NetworkSafetyManager() {
current_loop_ = MessageLoop::current(); }
On 2010/06/17 19:54:38, Ruðrugis wrote:
> move initializer to .cc file, use member initialization syntax.

Done.

Sign in to reply to this message.

Sigurður Ásgeirsson

looking better, getting close. http://codereview.chromium.org/2487001/diff/113002/96006 File net/network_safety/network_safety_entry.h (right): http://codereview.chromium.org/2487001/diff/113002/96006#newcode15 net/network_safety/network_safety_entry.h:15: using base::TimeTicks; It's not polite ...

4 years, 11 months ago (2010-06-22 14:43:47 UTC) #21

malavv

Thanks you Siggi for those code Review, I know that it is time consuming for ...

4 years, 11 months ago (2010-06-22 17:36:42 UTC) #22

Thanks you Siggi for those code Review, I know that it is time consuming for
you, I appreciate the comments.

For this iteration, I have reply to your comments in the unit test and removed
code that was not supposed to be there but were related to my other project.

I also written two comments in the entry interface.

Thanks!

http://codereview.chromium.org/2487001/diff/113002/96006
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/113002/96006#newcode15
net/network_safety/network_safety_entry.h:15: using base::TimeTicks;
On 2010/06/22 14:43:47, Ruðrugis wrote:
> It's not polite to have using clauses in a header, you'll pollute the global
> namespace for all your users. Suggest using qualified names in your code, and
> you can have a using declaration in the .cc file.

Done.

http://codereview.chromium.org/2487001/diff/113002/96007
File net/network_safety/network_safety_entry_interface.h (right):

http://codereview.chromium.org/2487001/diff/113002/96007#newcode16
net/network_safety/network_safety_entry_interface.h:16:
NetworkSafetyEntryInterface() { }
On 2010/06/22 14:43:47, Ruðrugis wrote:
> Remove the constructor, the compiler provides one implicitly.

I know that the compiler will offer a constructor if you don`t write one, but I
use Dissallow_Copy_and_assign which writes a constructor by copy in the private
section. And therefore the compiler will not write a default constructor by
itself since I already wrote one.

http://codereview.chromium.org/2487001/diff/113002/96007#newcode21
net/network_safety/network_safety_entry_interface.h:21: virtual bool
IsRequestAllowed() const =0;
On 2010/06/22 14:43:47, Ruðrugis wrote:
> = 0;

Done.

http://codereview.chromium.org/2487001/diff/113002/96007#newcode27
net/network_safety/network_safety_entry_interface.h:27: friend class
base::RefCountedThreadSafe<NetworkSafetyEntryInterface>;
I changed this to use RefCounted instead of thread safe, but the interface
requests that you friend this class.

http://codereview.chromium.org/2487001/diff/113002/96011
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/113002/96011#newcode33
net/network_safety/network_safety_manager.cc:33: entry= new
NetworkSafetyEntry();
On 2010/06/22 14:43:47, Ruðrugis wrote:
> wonky indent here?

Done

http://codereview.chromium.org/2487001/diff/113002/96011#newcode39
net/network_safety/network_safety_manager.cc:39:
:current_loop_(MessageLoop::current())
On 2010/06/22 14:43:47, Ruðrugis wrote:
> : current_loop_() ... {
> }

Done.

http://codereview.chromium.org/2487001/diff/113002/96013
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/113002/96013#newcode183
net/network_safety/network_safety_unittest.cc:183: TimeDelta living_time,
five_ms_unit;
On 2010/06/22 14:43:47, Ruðrugis wrote:
> living_time -> lifetime? 
> five_ms_unit -> const TimeDelta kFiveMs = ...?
> 

Done.

http://codereview.chromium.org/2487001/diff/113002/96013#newcode190
net/network_safety/network_safety_unittest.cc:190: std::make_pair(now_, false),
On 2010/06/22 14:43:47, Ruðrugis wrote:
> it is generally better to do this sort of thing in a sequence of EXPECT_EQ()
> assertions, as that way you get information about which case failed from the
> line number on failure. Same for below.

I understand the problematic and I tried to give a nicer way to debug than what
I did by printing the number of the test case which failed in the for loop. I
think the way it is done now prevent lots of code duplication and is less smelly
giving a way for user to easily see test cases parameters and expected values.

Nevertheless, if you still find that it would be better to format this as a
sequence of EXPECT_EQ(), I will change it.

http://codereview.chromium.org/2487001/diff/113002/96015
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/2487001/diff/113002/96015#newcode19
net/url_request/url_request_http_job.h:19: #include
"net/network_safety/network_tagging.h"
On 2010/06/22 14:43:47, Ruðrugis wrote:
> unrelated change.

Done.

http://codereview.chromium.org/2487001/diff/113002/96015#newcode96
net/url_request/url_request_http_job.h:96: NetworkTagging tagModule_;
On 2010/06/22 14:43:47, Ruðrugis wrote:
> ooops, unrelated change sneaking in here?

Done.

Sign in to reply to this message.

Sigurður Ásgeirsson

Couple of nits, then I think this is ready for review by someone more familiar ...

4 years, 11 months ago (2010-06-22 17:56:53 UTC) #23

Jói

Just some nits now that I finally had time to take a last quick look. ...

4 years, 11 months ago (2010-06-30 10:43:35 UTC) #24

eroman

Some high level comments before I dive into nits: (a) Nice design document! Can this ...

4 years, 10 months ago (2010-07-21 07:31:57 UTC) #25

Jói

Thanks for a very thorough review Eric. We could change the name to RequestThrottler, that ...

4 years, 10 months ago (2010-07-21 20:28:54 UTC) #26

Thanks for a very thorough review Eric.

We could change the name to RequestThrottler, that seems fairly
specific while still broad enough to capture that it does both
exponential back-off and back-off based on the custom header sites can
send back.

Sounds like a good idea to let users override the throttling without
restarting the browser, although note that similar mechanisms have
existed in Toolbar and Desktop for a long time now and we've never had
a problem - generally the timeout doesn't get very long before the
server recovers.  For extensions, we're planning to have Maxime look
at detecting user-initiated requests vs. machine-generated requests
(something like assuming all user-generated events occur within half a
second or so of an event handler firing into the extension JavaScript)
and it might be a possibility to make these and other explicitly
user-generated requests (like hitting the refresh button) clear the
back-off settings for the given URL or a whole website.

Cheers,
Jói


On Wed, Jul 21, 2010 at 3:31 AM,  <eroman@chromium.org> wrote:
> Some high level comments before I dive into nits:
>
> (a) Nice design document!
>
> Can this document be made public. That way you can link to it from the code
> review description, and from the header file.
>
> (b) Please add a description for this CL. To do this, click the "Edit Issue"
> link on the review webpage.
>
> (c) How are we going to measure the effectiveness of this change?
>
> The general mechanism we use in Chrome for answering such questions are
> "histograms".
> You can read up about them here:
>
https://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics#...
>
> Basically, I think we should have a way to see how often we are throttling
> requests.
> This will help catch if there are problems and lots of users are getting the
> errors, and also to evaluate the impact of tweaking the various backoff
> knobs.
>
> Note that currently we have the Net.ErrorCodesForMainFrame histogram which
> measures error codes seen by user initiated navigations, so we will be
> seeing
> some ERR_REQUEST_REFUSED distributions show up in here (but only for a
> limited
> subset of requests).
>
> At any rate, additional histograms are best added as a separate follow-up
> change
> so don't add it to this review, but I am interested to hear your thoughts on
> it.
>
> (d) Will users be able to override the throttling? For example what if
> something
> went wrong and the user has now fixed the problem (uninstalled problem
> extension). They now want to load the webpage again, but keep getting
> getting
> ERR_REQUEST_REFUSED on each request. Perhaps refresh should bypass the
> throttling layer? Otherwise they either need to wait, or restart the browser
> (which isn't really viable on chromeos).
>
> (e) Debugging and diagnostics: I recommend exposing the safety entry map on
> about:net-internals. This will help with debugging, and could also be a way
> for
> power-users to hit a button to reset the current entries. (This of course is
> a
> separate change).
>
> (f) Naming: I am not very fond of "network safety", since it sounds more
> like a
> security layer. I think the keywords for this feature are more along the
> lines
> of "throttling", "ddos", "backoff". So maybe something like
> RequestThrottler, or
> DDOSProtector would be more descriptive.
>
>
>
> http://codereview.chromium.org/2487001/diff/159001/160002
> File net/base/net_error_list.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160002#newcode161
> net/base/net_error_list.h:161: NET_ERROR(REQUEST_REFUSED, -127)
> It is important for the error name to be as descriptive as possible,
> since for the users that hit this the symbolic name is all they
> information they have to diagnose the problem.
>
> Some useful keywords for users will be "throttled", "ddos", "aborted",
> "temporary"
>
> How about something like:
>
> TEMPORARILY_THROTTLED_BY_DDOS
>
> http://codereview.chromium.org/2487001/diff/159001/160004
> File net/network_safety/network_safety_entry.cc (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode8
> net/network_safety/network_safety_entry.cc:8: #include <cmath>
> header orders here should be as follows:
>
> #include "net/network_safety/network_safety_entry.h"
>
> #include <cmath>
>
> #include "base/logging.h"
> #include "base/rand_util.h"
> #include "base/string_util.h"
> #include "net/network_safety/network_safety_header_interface.h"
>
>
> For more info see:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode37
> net/network_safety/network_safety_entry.cc:37:
> NetworkSafetyHeaderInterface* response) {
> Can this be a const pointer instead, or a const reference?
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode65
> net/network_safety/network_safety_entry.cc:65:
> base::TimeDelta(GetTimeNow() - release_time_).InMilliseconds();
> indent continued lines by 4 spaces.
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode86
> net/network_safety/network_safety_entry.cc:86:
> std::max(old_values_.number_of_failed_requests, num_of_time_delayed_);
> indent by 4 spaces.
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode90
> net/network_safety/network_safety_entry.cc:90:
> std::max(old_values_.release_time, release_time_));
> indentation.
>
> http://codereview.chromium.org/2487001/diff/159001/160004#newcode115
> net/network_safety/network_safety_entry.cc:115: DCHECK(conversion_is_ok)
> We need to be prepared to handle malformed header values.
>
> In chrome we use DCHECK as an assert for something that is IMPOSSIBLE to
> happen (if a DCHECK is triggered it results in a crash, and implies
> there was some incorrect logic in our code).
>
> So it is inappropriate to DCHECK() based on a user input, since it is
> possible for that input to be malformed.
>
> http://codereview.chromium.org/2487001/diff/159001/160005
> File net/network_safety/network_safety_entry.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode13
> net/network_safety/network_safety_entry.h:13: #include
> "base/ref_counted.h"
> nit: move ref_counted.h above time.h so they are in sorted order.
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode22
> net/network_safety/network_safety_entry.h:22: // This method needs to be
> called prior to every requests; if it returns
> nit: "requests" --> "request"
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode42
> net/network_safety/network_safety_entry.h:42: // of our packet was
> malformed.
> wording: "packet" --> "packets". That said, I don't think packets is the
> right thing here.
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode48
> net/network_safety/network_safety_entry.h:48: // Additional constant to
> further adjust back-off
> Please end with a period. Also, "additional" + "further" in the same
> sentence seems redundant.
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode54
> net/network_safety/network_safety_entry.h:54: // Fuzzing percentage. ex:
> 10% will spread request randomly
> "request" --> "requests"
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode69
> net/network_safety/network_safety_entry.h:69: static const std::string
> kRetryHeaderName;
> Our style guide prohibits using static classes (as it adds initializer
> calls to startup).
>
> Instead, please define this as a const char[].
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode71
> net/network_safety/network_safety_entry.h:71: // Calculate when should
> we start sending request again. Follows a failure
> "Calculates when we should start sending requests again".
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode89
> net/network_safety/network_safety_entry.h:89: // Number of time we were
> delayed
> "time" --> "times".
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode90
> net/network_safety/network_safety_entry.h:90: int num_of_time_delayed_;
> "num_of_time_delayed_"  -->  "num_times_deleayed_"
>
> http://codereview.chromium.org/2487001/diff/159001/160005#newcode97
> net/network_safety/network_safety_entry.h:97: virtual
> ~NetworkSafetyEntry();
> Please move this to the top of the protected section.
>
> For more info see:
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
>
> http://codereview.chromium.org/2487001/diff/159001/160006
> File net/network_safety/network_safety_entry_interface.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160006#newcode16
> net/network_safety/network_safety_entry_interface.h:16:
> NetworkSafetyEntryInterface() { }
> nit: collapse empty blocks as {}
>
> http://codereview.chromium.org/2487001/diff/159001/160006#newcode25
> net/network_safety/network_safety_entry_interface.h:25: virtual
> ~NetworkSafetyEntryInterface() { }
> {}
>
> http://codereview.chromium.org/2487001/diff/159001/160006#newcode28
> net/network_safety/network_safety_entry_interface.h:28:
> DISALLOW_COPY_AND_ASSIGN(NetworkSafetyEntryInterface);
> Please include "base/basictypes.h" when using this macro.
>
> (It is included indirectly from base/ref_counted.h, but generally nice
> to be explicit on the dependencies).
>
> http://codereview.chromium.org/2487001/diff/159001/160009
> File net/network_safety/network_safety_header_interface.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160009#newcode11
> net/network_safety/network_safety_header_interface.h:11: class
> NetworkSafetyHeaderInterface {
> I am not convinced this interface is worthwhile, since it is pretty much
> a direct mapping to HttpResponseHeaders.
>
> http://codereview.chromium.org/2487001/diff/159001/160010
> File net/network_safety/network_safety_manager.cc (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode24
> net/network_safety/network_safety_manager.cc:24: requests_counter_ = 0;
> perhaps this would be better called "requests_since_last_gc_"
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode39
> net/network_safety/network_safety_manager.cc:39: :
> current_loop_(MessageLoop::current()) {
> It is unfortunate that we can't simply extend NetworkSafetyManager from
> NonThreadSafe (base/non_thread_safe.h).
>
> I guess that wouldn't work though, since the destructor will get called
> from the UI thread.
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode47
> net/network_safety/network_safety_manager.cc:47: std::string
> NetworkSafetyManager::GetIdFromUrl(const GURL& url) {
> IMPORTANT: stripping down URLs is fairly complicated, I think we would
> be safer by building "up" the URL.
>
> In particular, one thing I think is not handled by GetWithEmptyPath() is
> embedded identities in the URL.
>
> For example:
>
> http://foo:bar@google.com/x
> http://foo1:bar@google.com/x
>
> Are conceptually the same URL, however they will result in a different
> "ID" I believe.
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode57
> net/network_safety/network_safety_manager.cc:57:
> DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO);
> Why not do DCHECK_EQ(current_loop_, MessageLoop::current()) ?
>
> Or could get the thread id and use that for comparisons.
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode60
> net/network_safety/network_safety_manager.cc:60: while (i !=
> url_entries_.end()) {
> It is probably a good idea to also have an upper bound on the size the
> map can grow to, to make sure we cant end up with hundreds of thousands
> of outtaded entries that can't be collected.
>
> http://codereview.chromium.org/2487001/diff/159001/160010#newcode62
> net/network_safety/network_safety_manager.cc:62: UrlEntryMap::iterator
> tmp = i++;
> I think you can just put the expression directly in place of tmp.
>
> http://codereview.chromium.org/2487001/diff/159001/160011
> File net/network_safety/network_safety_manager.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode1
> net/network_safety/network_safety_manager.h:1: // Copyright (c)
> 2006-2010 The Chromium Authors. All rights reserved.
> just use 2010 as the copyright date.
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode15
> net/network_safety/network_safety_manager.h:15: #include
> "base/scoped_ptr.h"
> keep in sorted order
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode37
> net/network_safety/network_safety_manager.h:37: void
> NotifyRequestBodyWasMalformed(const GURL& url);
> What is this used by?
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode43
> net/network_safety/network_safety_manager.h:43: // Allow us to map an
> URL ID with its entry.
> what is a "URL ID" ?
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode64
> net/network_safety/network_safety_manager.h:64: static unsigned int
> requests_counter_;
> why does this need to be static, i thought we were using a singleton?
>
> http://codereview.chromium.org/2487001/diff/159001/160011#newcode69
> net/network_safety/network_safety_manager.h:69: static const unsigned
> int kRequestsBetweenCollecting;
> Same question as above.
>
> http://codereview.chromium.org/2487001/diff/159001/160012
> File net/network_safety/network_safety_unittest.cc (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode37
> net/network_safety/network_safety_unittest.cc:37: static const int
> k_add_constant() { return kAdditionalConstantMs; }
> Instead of having public accessors to the constants, why not make the
> constants in base class public?
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode57
> net/network_safety/network_safety_unittest.cc:57: std::string
> retry_value = "0.0", int response_code = 0)
> our style guide does not allow default parameters.
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode65
> net/network_safety/network_safety_unittest.cc:65: else
> nit: our style is to not use "else" when the if returns.
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode72
> net/network_safety/network_safety_unittest.cc:72: if (key ==
> MockNetworkSafetyEntry::k_retry_name())
> simplify to return (key == ....);
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode121
> net/network_safety/network_safety_unittest.cc:121: TimeTicks time_;
> We only use the trailing underscore for private members. Since this is
> public, just write "time".
>
> http://codereview.chromium.org/2487001/diff/159001/160012#newcode132
> net/network_safety/network_safety_unittest.cc:132: GURL url_;
> see comment above.
>
> http://codereview.chromium.org/2487001/diff/159001/160013
> File net/url_request/url_request_http_job.cc (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160013#newcode35
> net/url_request/url_request_http_job.cc:35: #include
> "net/network_safety/network_safety_header_adapter.h"
> move this up.
>
> http://codereview.chromium.org/2487001/diff/159001/160014
> File net/url_request/url_request_http_job.h (right):
>
> http://codereview.chromium.org/2487001/diff/159001/160014#newcode18
> net/url_request/url_request_http_job.h:18: #include
> "net/network_safety/network_safety_manager.h"
> move this up to preserve sorted order.
>
> http://codereview.chromium.org/2487001/show
>

Sign in to reply to this message.

malavv

Hi Eric, I really appreciate the time you put into doing this code review, I ...

4 years, 10 months ago (2010-07-22 15:02:58 UTC) #27

Hi Eric,

I really appreciate the time you put into doing this code review, I know
that time is precious to you right now. I tried to apply all of your
suggestions yesterday and to push it on the try-bots before answering to
this mail, but I encountered problem with my client.

On the matter of making the design document public, I was of hoping that
once my CL is integrated to Chromium it could live in the design-docs
section of Chromium.org. For the moment, I switched it to a @gmail account
and to share with everyone.
Public design-doc: Link to the public version of my design
document<https://docs.google.com/document/edit?id=1oqOh6oRqnGP2yz0yETdaxKh5MhP5U4gXa-Jd0bAwLM8&hl=en#>

On another subject, the idea you suggested about histograms and metrics
sounds like a good idea. This module is definitely not something your want
to start behaving widely and refusing random requests. It would be a good
addition to have some way in which we could monitor this. A tab in
net-internal that contains a reset button also makes sense. I will look for
implementing those recommendation when the main module will be integrated.

As it seems to make consensus, I renamed the module from *NetworkSafety* to
*RequestThrottler*. The external documentation still needs to be adjusted
but I will do this shortly.

Thanks,
Maxime

On Wed, Jul 21, 2010 at 4:28 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Thanks for a very thorough review Eric.
>
> We could change the name to RequestThrottler, that seems fairly
> specific while still broad enough to capture that it does both
> exponential back-off and back-off based on the custom header sites can
> send back.
>
> Sounds like a good idea to let users override the throttling without
> restarting the browser, although note that similar mechanisms have
> existed in Toolbar and Desktop for a long time now and we've never had
> a problem - generally the timeout doesn't get very long before the
> server recovers.  For extensions, we're planning to have Maxime look
> at detecting user-initiated requests vs. machine-generated requests
> (something like assuming all user-generated events occur within half a
> second or so of an event handler firing into the extension JavaScript)
> and it might be a possibility to make these and other explicitly
> user-generated requests (like hitting the refresh button) clear the
> back-off settings for the given URL or a whole website.
>
> Cheers,
> Jói
>
>
> On Wed, Jul 21, 2010 at 3:31 AM,  <eroman@chromium.org> wrote:
> > Some high level comments before I dive into nits:
> >
> > (a) Nice design document!
> >
> > Can this document be made public. That way you can link to it from the
> code
> > review description, and from the header file.
> >
> > (b) Please add a description for this CL. To do this, click the "Edit
> Issue"
> > link on the review webpage.
> >
> > (c) How are we going to measure the effectiveness of this change?
> >
> > The general mechanism we use in Chrome for answering such questions are
> > "histograms".
> > You can read up about them here:
> >
>
https://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics#...
> >
> > Basically, I think we should have a way to see how often we are
> throttling
> > requests.
> > This will help catch if there are problems and lots of users are getting
> the
> > errors, and also to evaluate the impact of tweaking the various backoff
> > knobs.
> >
> > Note that currently we have the Net.ErrorCodesForMainFrame histogram
> which
> > measures error codes seen by user initiated navigations, so we will be
> > seeing
> > some ERR_REQUEST_REFUSED distributions show up in here (but only for a
> > limited
> > subset of requests).
> >
> > At any rate, additional histograms are best added as a separate follow-up
> > change
> > so don't add it to this review, but I am interested to hear your thoughts
> on
> > it.
> >
> > (d) Will users be able to override the throttling? For example what if
> > something
> > went wrong and the user has now fixed the problem (uninstalled problem
> > extension). They now want to load the webpage again, but keep getting
> > getting
> > ERR_REQUEST_REFUSED on each request. Perhaps refresh should bypass the
> > throttling layer? Otherwise they either need to wait, or restart the
> browser
> > (which isn't really viable on chromeos).
> >
> > (e) Debugging and diagnostics: I recommend exposing the safety entry map
> on
> > about:net-internals. This will help with debugging, and could also be a
> way
> > for
> > power-users to hit a button to reset the current entries. (This of course
> is
> > a
> > separate change).
> >
> > (f) Naming: I am not very fond of "network safety", since it sounds more
> > like a
> > security layer. I think the keywords for this feature are more along the
> > lines
> > of "throttling", "ddos", "backoff". So maybe something like
> > RequestThrottler, or
> > DDOSProtector would be more descriptive.
> >
> >
> >
> > http://codereview.chromium.org/2487001/diff/159001/160002
> > File net/base/net_error_list.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160002#newcode161
> > net/base/net_error_list.h:161: NET_ERROR(REQUEST_REFUSED, -127)
> > It is important for the error name to be as descriptive as possible,
> > since for the users that hit this the symbolic name is all they
> > information they have to diagnose the problem.
> >
> > Some useful keywords for users will be "throttled", "ddos", "aborted",
> > "temporary"
> >
> > How about something like:
> >
> > TEMPORARILY_THROTTLED_BY_DDOS
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004
> > File net/network_safety/network_safety_entry.cc (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode8
> > net/network_safety/network_safety_entry.cc:8: #include <cmath>
> > header orders here should be as follows:
> >
> > #include "net/network_safety/network_safety_entry.h"
> >
> > #include <cmath>
> >
> > #include "base/logging.h"
> > #include "base/rand_util.h"
> > #include "base/string_util.h"
> > #include "net/network_safety/network_safety_header_interface.h"
> >
> >
> > For more info see:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode37
> > net/network_safety/network_safety_entry.cc:37:
> > NetworkSafetyHeaderInterface* response) {
> > Can this be a const pointer instead, or a const reference?
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode65
> > net/network_safety/network_safety_entry.cc:65:
> > base::TimeDelta(GetTimeNow() - release_time_).InMilliseconds();
> > indent continued lines by 4 spaces.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode86
> > net/network_safety/network_safety_entry.cc:86:
> > std::max(old_values_.number_of_failed_requests, num_of_time_delayed_);
> > indent by 4 spaces.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode90
> > net/network_safety/network_safety_entry.cc:90:
> > std::max(old_values_.release_time, release_time_));
> > indentation.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode115
> > net/network_safety/network_safety_entry.cc:115: DCHECK(conversion_is_ok)
> > We need to be prepared to handle malformed header values.
> >
> > In chrome we use DCHECK as an assert for something that is IMPOSSIBLE to
> > happen (if a DCHECK is triggered it results in a crash, and implies
> > there was some incorrect logic in our code).
> >
> > So it is inappropriate to DCHECK() based on a user input, since it is
> > possible for that input to be malformed.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005
> > File net/network_safety/network_safety_entry.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode13
> > net/network_safety/network_safety_entry.h:13: #include
> > "base/ref_counted.h"
> > nit: move ref_counted.h above time.h so they are in sorted order.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode22
> > net/network_safety/network_safety_entry.h:22: // This method needs to be
> > called prior to every requests; if it returns
> > nit: "requests" --> "request"
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode42
> > net/network_safety/network_safety_entry.h:42: // of our packet was
> > malformed.
> > wording: "packet" --> "packets". That said, I don't think packets is the
> > right thing here.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode48
> > net/network_safety/network_safety_entry.h:48: // Additional constant to
> > further adjust back-off
> > Please end with a period. Also, "additional" + "further" in the same
> > sentence seems redundant.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode54
> > net/network_safety/network_safety_entry.h:54: // Fuzzing percentage. ex:
> > 10% will spread request randomly
> > "request" --> "requests"
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode69
> > net/network_safety/network_safety_entry.h:69: static const std::string
> > kRetryHeaderName;
> > Our style guide prohibits using static classes (as it adds initializer
> > calls to startup).
> >
> > Instead, please define this as a const char[].
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode71
> > net/network_safety/network_safety_entry.h:71: // Calculate when should
> > we start sending request again. Follows a failure
> > "Calculates when we should start sending requests again".
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode89
> > net/network_safety/network_safety_entry.h:89: // Number of time we were
> > delayed
> > "time" --> "times".
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode90
> > net/network_safety/network_safety_entry.h:90: int num_of_time_delayed_;
> > "num_of_time_delayed_"  -->  "num_times_deleayed_"
> >
> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode97
> > net/network_safety/network_safety_entry.h:97: virtual
> > ~NetworkSafetyEntry();
> > Please move this to the top of the protected section.
> >
> > For more info see:
> >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> >
> > http://codereview.chromium.org/2487001/diff/159001/160006
> > File net/network_safety/network_safety_entry_interface.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode16
> > net/network_safety/network_safety_entry_interface.h:16:
> > NetworkSafetyEntryInterface() { }
> > nit: collapse empty blocks as {}
> >
> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode25
> > net/network_safety/network_safety_entry_interface.h:25: virtual
> > ~NetworkSafetyEntryInterface() { }
> > {}
> >
> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode28
> > net/network_safety/network_safety_entry_interface.h:28:
> > DISALLOW_COPY_AND_ASSIGN(NetworkSafetyEntryInterface);
> > Please include "base/basictypes.h" when using this macro.
> >
> > (It is included indirectly from base/ref_counted.h, but generally nice
> > to be explicit on the dependencies).
> >
> > http://codereview.chromium.org/2487001/diff/159001/160009
> > File net/network_safety/network_safety_header_interface.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160009#newcode11
> > net/network_safety/network_safety_header_interface.h:11: class
> > NetworkSafetyHeaderInterface {
> > I am not convinced this interface is worthwhile, since it is pretty much
> > a direct mapping to HttpResponseHeaders.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010
> > File net/network_safety/network_safety_manager.cc (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode24
> > net/network_safety/network_safety_manager.cc:24: requests_counter_ = 0;
> > perhaps this would be better called "requests_since_last_gc_"
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode39
> > net/network_safety/network_safety_manager.cc:39: :
> > current_loop_(MessageLoop::current()) {
> > It is unfortunate that we can't simply extend NetworkSafetyManager from
> > NonThreadSafe (base/non_thread_safe.h).
> >
> > I guess that wouldn't work though, since the destructor will get called
> > from the UI thread.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode47
> > net/network_safety/network_safety_manager.cc:47: std::string
> > NetworkSafetyManager::GetIdFromUrl(const GURL& url) {
> > IMPORTANT: stripping down URLs is fairly complicated, I think we would
> > be safer by building "up" the URL.
> >
> > In particular, one thing I think is not handled by GetWithEmptyPath() is
> > embedded identities in the URL.
> >
> > For example:
> >
> > http://foo:bar@google.com/x
> > http://foo1:bar@google.com/x
> >
> > Are conceptually the same URL, however they will result in a different
> > "ID" I believe.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode57
> > net/network_safety/network_safety_manager.cc:57:
> > DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO);
> > Why not do DCHECK_EQ(current_loop_, MessageLoop::current()) ?
> >
> > Or could get the thread id and use that for comparisons.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode60
> > net/network_safety/network_safety_manager.cc:60: while (i !=
> > url_entries_.end()) {
> > It is probably a good idea to also have an upper bound on the size the
> > map can grow to, to make sure we cant end up with hundreds of thousands
> > of outtaded entries that can't be collected.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode62
> > net/network_safety/network_safety_manager.cc:62: UrlEntryMap::iterator
> > tmp = i++;
> > I think you can just put the expression directly in place of tmp.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011
> > File net/network_safety/network_safety_manager.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode1
> > net/network_safety/network_safety_manager.h:1: // Copyright (c)
> > 2006-2010 The Chromium Authors. All rights reserved.
> > just use 2010 as the copyright date.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode15
> > net/network_safety/network_safety_manager.h:15: #include
> > "base/scoped_ptr.h"
> > keep in sorted order
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode37
> > net/network_safety/network_safety_manager.h:37: void
> > NotifyRequestBodyWasMalformed(const GURL& url);
> > What is this used by?
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode43
> > net/network_safety/network_safety_manager.h:43: // Allow us to map an
> > URL ID with its entry.
> > what is a "URL ID" ?
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode64
> > net/network_safety/network_safety_manager.h:64: static unsigned int
> > requests_counter_;
> > why does this need to be static, i thought we were using a singleton?
> >
> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode69
> > net/network_safety/network_safety_manager.h:69: static const unsigned
> > int kRequestsBetweenCollecting;
> > Same question as above.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012
> > File net/network_safety/network_safety_unittest.cc (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode37
> > net/network_safety/network_safety_unittest.cc:37: static const int
> > k_add_constant() { return kAdditionalConstantMs; }
> > Instead of having public accessors to the constants, why not make the
> > constants in base class public?
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode57
> > net/network_safety/network_safety_unittest.cc:57: std::string
> > retry_value = "0.0", int response_code = 0)
> > our style guide does not allow default parameters.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode65
> > net/network_safety/network_safety_unittest.cc:65: else
> > nit: our style is to not use "else" when the if returns.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode72
> > net/network_safety/network_safety_unittest.cc:72: if (key ==
> > MockNetworkSafetyEntry::k_retry_name())
> > simplify to return (key == ....);
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode121
> > net/network_safety/network_safety_unittest.cc:121: TimeTicks time_;
> > We only use the trailing underscore for private members. Since this is
> > public, just write "time".
> >
> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode132
> > net/network_safety/network_safety_unittest.cc:132: GURL url_;
> > see comment above.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160013
> > File net/url_request/url_request_http_job.cc (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160013#newcode35
> > net/url_request/url_request_http_job.cc:35: #include
> > "net/network_safety/network_safety_header_adapter.h"
> > move this up.
> >
> > http://codereview.chromium.org/2487001/diff/159001/160014
> > File net/url_request/url_request_http_job.h (right):
> >
> > http://codereview.chromium.org/2487001/diff/159001/160014#newcode18
> > net/url_request/url_request_http_job.h:18: #include
> > "net/network_safety/network_safety_manager.h"
> > move this up to preserve sorted order.
> >
> > http://codereview.chromium.org/2487001/show
> >
>

Sign in to reply to this message.

malavv

Fixed issues pointed by Eroman. Also made document public: https://docs.google.com/document/edit?id=1oqOh6oRqnGP2yz0yETdaxKh5MhP5U4gXa-Jd0bAwLM8&hl=en# http://codereview.chromium.org/2487001/diff/138002/122011 File net/network_safety/network_safety_manager.cc (right): http://codereview.chromium.org/2487001/diff/138002/122011#newcode33 ...

4 years, 10 months ago (2010-07-22 15:35:58 UTC) #28

Fixed issues pointed by Eroman.

Also made document public:
https://docs.google.com/document/edit?id=1oqOh6oRqnGP2yz0yETdaxKh5MhP5U4gXa-J...

http://codereview.chromium.org/2487001/diff/138002/122011
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/138002/122011#newcode33
net/network_safety/network_safety_manager.cc:33: entry= new
NetworkSafetyEntry();
On 2010/06/22 17:56:53, Ruðrugis wrote:
> entry = 

Done.

http://codereview.chromium.org/2487001/diff/138002/122012
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/138002/122012#newcode52
net/network_safety/network_safety_manager.h:52: // can be used in our map.
Resulting i`d will be lowercase and be missing both
On 2010/06/22 17:56:53, Ruðrugis wrote:
> oops?

Done.

http://codereview.chromium.org/2487001/diff/145002/125005
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/145002/125005#newcode101
net/network_safety/network_safety_entry.cc:101: delay_int = (delay_int <
kMaximumBackoffMs) ? delay_int : kMaximumBackoffMs;
On 2010/06/30 10:43:35, Jói wrote:
> Why not std::min() ?

Done.

http://codereview.chromium.org/2487001/diff/145002/125006
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/145002/125006#newcode41
net/network_safety/network_safety_entry.h:41: struct OldValues {
On 2010/06/30 10:43:35, Jói wrote:
> The name and documentation here are not that enlightening when you read only
the
> header.  The release_time is easy to see has some relation to release time
> below, but what meaning does num_of_time have?

Done.

http://codereview.chromium.org/2487001/diff/145002/125006#newcode93
net/network_safety/network_safety_entry.h:93: OldValues old_values_;
On 2010/06/30 10:43:35, Jói wrote:
> Would be good to document this as well ("memento to retrieve state" isn't very
> enlightening - what state? when it is memorized?)

Done.

http://codereview.chromium.org/2487001/diff/145002/125010
File net/network_safety/network_safety_header_interface.h (right):

http://codereview.chromium.org/2487001/diff/145002/125010#newcode15
net/network_safety/network_safety_header_interface.h:15: virtual std::string
GetNormalizedValue(const std::string& key) const = 0;
On 2010/06/30 10:43:35, Jói wrote:
> These methods should be documented here since this is the interface;
documenting
> them where they're overridden is the wrong place.

Done.

http://codereview.chromium.org/2487001/diff/145002/125011
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/145002/125011#newcode16
net/network_safety/network_safety_manager.cc:16:
NetworkSafetyManager::RegisterRequestUrl(
On 2010/06/30 10:43:35, Jói wrote:
> here, this indenting is strange again, but probably the best you can do given
> you probably don't want to break on the ::

Done.

http://codereview.chromium.org/2487001/diff/145002/125012
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/145002/125012#newcode30
net/network_safety/network_safety_manager.h:30: RegisterRequestUrl(const GURL&
url);
On 2010/06/30 10:43:35, Jói wrote:
> this is strange indentation; normally you would take the "const GURL..." part
> and indent it to where this line starts, so that the function name can be in
the
> normally expected place

Done.

http://codereview.chromium.org/2487001/diff/145002/125012#newcode32
net/network_safety/network_safety_manager.h:32: // This method is used by higher
level modules who can notify this class if
On 2010/06/30 10:43:35, Jói wrote:
> "who" -> "which" or "that"   (handy tip: "that" is usually safe if you're not
> sure whether to use "who" or "which")

Sorry, French habits.

http://codereview.chromium.org/2487001/diff/145002/125012#newcode51
net/network_safety/network_safety_manager.h:51: // Method that allows us to
transform an url into a id that is useful and
On 2010/06/30 10:43:35, Jói wrote:
> "a id" -> "an id"
> 
> Also, in comments I'd personally prefer "URL" and "ID" instead of "url" and
"id"
> since both are acronyms, but this is a very minor nit and you don't need to
> change it.

Done.

http://codereview.chromium.org/2487001/diff/145002/125012#newcode58
net/network_safety/network_safety_manager.h:58: // kRequestBetweenCollecting
constant.
On 2010/06/30 10:43:35, Jói wrote:
> The name is actually kRequestsBetweenCollecting (note the plural Requests).

Done.

http://codereview.chromium.org/2487001/diff/145002/125013
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/145002/125013#newcode36
net/network_safety/network_safety_unittest.cc:36: static const int
k_add_constant() { return kAdditionalConstantMs; }
On 2010/06/30 10:43:35, Jói wrote:
> What are these functions for?  Maybe a one-line comment as to why they're
> needed.

Done.

http://codereview.chromium.org/2487001/diff/145002/125013#newcode124
net/network_safety/network_safety_unittest.cc:124: struct GurlAndString {
On 2010/06/30 10:43:35, Jói wrote:
> generally our style is to have one blank line between top-level elements,
except
> perhaps a block of one-line definitions (which these are not)

Done.

http://codereview.chromium.org/2487001/diff/159001/160004
File net/network_safety/network_safety_entry.cc (right):

http://codereview.chromium.org/2487001/diff/159001/160004#newcode8
net/network_safety/network_safety_entry.cc:8: #include <cmath>
On 2010/07/21 07:31:57, eroman wrote:
> header orders here should be as follows:
> 
> #include "net/network_safety/network_safety_entry.h"
> 
> #include <cmath>
> 
> #include "base/logging.h"
> #include "base/rand_util.h"
> #include "base/string_util.h"
> #include "net/network_safety/network_safety_header_interface.h"
> 
> 
> For more info see:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

Done.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode37
net/network_safety/network_safety_entry.cc:37: NetworkSafetyHeaderInterface*
response) {
On 2010/07/21 07:31:57, eroman wrote:
> Can this be a const pointer instead, or a const reference?

Done.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode65
net/network_safety/network_safety_entry.cc:65: base::TimeDelta(GetTimeNow() -
release_time_).InMilliseconds();
On 2010/07/21 07:31:57, eroman wrote:
> indent continued lines by 4 spaces.

Done.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode86
net/network_safety/network_safety_entry.cc:86:
std::max(old_values_.number_of_failed_requests, num_of_time_delayed_);
On 2010/07/21 07:31:57, eroman wrote:
> indent by 4 spaces.

Done.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode90
net/network_safety/network_safety_entry.cc:90:
std::max(old_values_.release_time, release_time_));
On 2010/07/21 07:31:57, eroman wrote:
> indentation.

Done.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode115
net/network_safety/network_safety_entry.cc:115: DCHECK(conversion_is_ok)
On 2010/07/21 07:31:57, eroman wrote:
> We need to be prepared to handle malformed header values.
> 
> In chrome we use DCHECK as an assert for something that is IMPOSSIBLE to
happen
> (if a DCHECK is triggered it results in a crash, and implies there was some
> incorrect logic in our code).
> 
> So it is inappropriate to DCHECK() based on a user input, since it is possible
> for that input to be malformed.

I have modified it to its original state where it returns if t was not able to
do the conversion but I am wondering if it would be better to LOG this.

http://codereview.chromium.org/2487001/diff/159001/160004#newcode115
net/network_safety/network_safety_entry.cc:115: DCHECK(conversion_is_ok)
On 2010/07/21 07:31:57, eroman wrote:
> We need to be prepared to handle malformed header values.
> 
> In chrome we use DCHECK as an assert for something that is IMPOSSIBLE to
happen
> (if a DCHECK is triggered it results in a crash, and implies there was some
> incorrect logic in our code).
> 
> So it is inappropriate to DCHECK() based on a user input, since it is possible
> for that input to be malformed.

I have modified it to its original state where it returns if t was not able to
do the conversion but I am wondering if it would be better to LOG this.

http://codereview.chromium.org/2487001/diff/159001/160005
File net/network_safety/network_safety_entry.h (right):

http://codereview.chromium.org/2487001/diff/159001/160005#newcode13
net/network_safety/network_safety_entry.h:13: #include "base/ref_counted.h"
On 2010/07/21 07:31:57, eroman wrote:
> nit: move ref_counted.h above time.h so they are in sorted order.

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode22
net/network_safety/network_safety_entry.h:22: // This method needs to be called
prior to every requests; if it returns
On 2010/07/21 07:31:57, eroman wrote:
> nit: "requests" --> "request"

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode42
net/network_safety/network_safety_entry.h:42: // of our packet was malformed.
On 2010/07/21 07:31:57, eroman wrote:
> wording: "packet" --> "packets". That said, I don't think packets is the right
> thing here.
Changed for "bodies"

http://codereview.chromium.org/2487001/diff/159001/160005#newcode42
net/network_safety/network_safety_entry.h:42: // of our packet was malformed.
On 2010/07/21 07:31:57, eroman wrote:
> wording: "packet" --> "packets". That said, I don't think packets is the right
> thing here.
Changed for "bodies"

http://codereview.chromium.org/2487001/diff/159001/160005#newcode48
net/network_safety/network_safety_entry.h:48: // Additional constant to further
adjust back-off
On 2010/07/21 07:31:57, eroman wrote:
> Please end with a period. Also, "additional" + "further" in the same sentence
> seems redundant.

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode54
net/network_safety/network_safety_entry.h:54: // Fuzzing percentage. ex: 10%
will spread request randomly
On 2010/07/21 07:31:57, eroman wrote:
> "request" --> "requests"

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode69
net/network_safety/network_safety_entry.h:69: static const std::string
kRetryHeaderName;
On 2010/07/21 07:31:57, eroman wrote:
> Our style guide prohibits using static classes (as it adds initializer calls
to
> startup).
> 
> Instead, please define this as a const char[].

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode71
net/network_safety/network_safety_entry.h:71: // Calculate when should we start
sending request again. Follows a failure
On 2010/07/21 07:31:57, eroman wrote:
> "Calculates when we should start sending requests again".

Done.

http://codereview.chromium.org/2487001/diff/159001/160005#newcode90
net/network_safety/network_safety_entry.h:90: int num_of_time_delayed_;
On 2010/07/21 07:31:57, eroman wrote:
> "num_of_time_delayed_"  -->  "num_times_deleayed_"

I considered that "deleayed" was a typo for delayed so it is num_times_delayed_

http://codereview.chromium.org/2487001/diff/159001/160005#newcode97
net/network_safety/network_safety_entry.h:97: virtual ~NetworkSafetyEntry();
On 2010/07/21 07:31:57, eroman wrote:
> Please move this to the top of the protected section.
> 
> For more info see:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/2487001/diff/159001/160006
File net/network_safety/network_safety_entry_interface.h (right):

http://codereview.chromium.org/2487001/diff/159001/160006#newcode16
net/network_safety/network_safety_entry_interface.h:16:
NetworkSafetyEntryInterface() { }
On 2010/07/21 07:31:57, eroman wrote:
> nit: collapse empty blocks as {}

Done.

http://codereview.chromium.org/2487001/diff/159001/160006#newcode25
net/network_safety/network_safety_entry_interface.h:25: virtual
~NetworkSafetyEntryInterface() { }
On 2010/07/21 07:31:57, eroman wrote:
> {}

Done.

http://codereview.chromium.org/2487001/diff/159001/160006#newcode28
net/network_safety/network_safety_entry_interface.h:28:
DISALLOW_COPY_AND_ASSIGN(NetworkSafetyEntryInterface);
On 2010/07/21 07:31:57, eroman wrote:
> Please include "base/basictypes.h" when using this macro.
> 
> (It is included indirectly from base/ref_counted.h, but generally nice to be
> explicit on the dependencies).

Done.

http://codereview.chromium.org/2487001/diff/159001/160009
File net/network_safety/network_safety_header_interface.h (right):

http://codereview.chromium.org/2487001/diff/159001/160009#newcode11
net/network_safety/network_safety_header_interface.h:11: class
NetworkSafetyHeaderInterface {
On 2010/07/21 07:31:57, eroman wrote:
> I am not convinced this interface is worthwhile, since it is pretty much a
> direct mapping to HttpResponseHeaders.

One of the main concern of my host was to make this module as portable as
possible. Since lots of project use an exponential back-off module and are
reinventing the wheel this implementation could be pretty much plug and play.
Therefore, we were careful not to directly map to any chrome component.

http://codereview.chromium.org/2487001/diff/159001/160010
File net/network_safety/network_safety_manager.cc (right):

http://codereview.chromium.org/2487001/diff/159001/160010#newcode24
net/network_safety/network_safety_manager.cc:24: requests_counter_ = 0;
On 2010/07/21 07:31:57, eroman wrote:
> perhaps this would be better called "requests_since_last_gc_"

Done.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode39
net/network_safety/network_safety_manager.cc:39: :
current_loop_(MessageLoop::current()) {
On 2010/07/21 07:31:57, eroman wrote:
> It is unfortunate that we can't simply extend NetworkSafetyManager from
> NonThreadSafe (base/non_thread_safe.h).
> 
> I guess that wouldn't work though, since the destructor will get called from
the
> UI thread.

Done.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode47
net/network_safety/network_safety_manager.cc:47: std::string
NetworkSafetyManager::GetIdFromUrl(const GURL& url) {
On 2010/07/21 07:31:57, eroman wrote:
> IMPORTANT: stripping down URLs is fairly complicated, I think we would be
safer
> by building "up" the URL.
> 
> In particular, one thing I think is not handled by GetWithEmptyPath() is
> embedded identities in the URL.
> 
> For example:
> 
> http://foo:bar%40google.com/x
> http://foo1:bar%40google.com/x
> 
> Are conceptually the same URL, however they will result in a different "ID" I
> believe.

Done.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode57
net/network_safety/network_safety_manager.cc:57:
DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO);
On 2010/07/21 07:31:57, eroman wrote:
> Why not do DCHECK_EQ(current_loop_, MessageLoop::current()) ?
> 
> Or could get the thread id and use that for comparisons.

This was done to make unit_test pass. Lots of unit tests in the network stuff
implicitly created a manager and then did thread testing creating an IO thread
on every test. Since the manager was not reset, it failed when CHECKing
current_loop against current message loop.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode57
net/network_safety/network_safety_manager.cc:57:
DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO);
On 2010/07/21 07:31:57, eroman wrote:
> Why not do DCHECK_EQ(current_loop_, MessageLoop::current()) ?
> 
> Or could get the thread id and use that for comparisons.

This was done to make unit_test pass. Lots of unit tests in the network stuff
implicitly created a manager and then did thread testing creating an IO thread
on every test. Since the manager was not reset, it failed when CHECKing
current_loop against current message loop.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode60
net/network_safety/network_safety_manager.cc:60: while (i != url_entries_.end())
{
On 2010/07/21 07:31:57, eroman wrote:
> It is probably a good idea to also have an upper bound on the size the map can
> grow to, to make sure we cant end up with hundreds of thousands of outtaded
> entries that can't be collected.

Done.

http://codereview.chromium.org/2487001/diff/159001/160010#newcode62
net/network_safety/network_safety_manager.cc:62: UrlEntryMap::iterator tmp =
i++;
On 2010/07/21 07:31:57, eroman wrote:
> I think you can just put the expression directly in place of tmp.

Done.

http://codereview.chromium.org/2487001/diff/159001/160011
File net/network_safety/network_safety_manager.h (right):

http://codereview.chromium.org/2487001/diff/159001/160011#newcode1
net/network_safety/network_safety_manager.h:1: // Copyright (c) 2006-2010 The
Chromium Authors. All rights reserved.
On 2010/07/21 07:31:57, eroman wrote:
> just use 2010 as the copyright date.

Done.

http://codereview.chromium.org/2487001/diff/159001/160011#newcode15
net/network_safety/network_safety_manager.h:15: #include "base/scoped_ptr.h"
On 2010/07/21 07:31:57, eroman wrote:
> keep in sorted order

Done.

http://codereview.chromium.org/2487001/diff/159001/160011#newcode37
net/network_safety/network_safety_manager.h:37: void
NotifyRequestBodyWasMalformed(const GURL& url);
On 2010/07/21 07:31:57, eroman wrote:
> What is this used by?
Currently nobody, but if for example the SpellChecker LoadDictionary function
received a payload that is incoherent it could notify us and we would give the
server time to readjust by marking it as a failure.

http://codereview.chromium.org/2487001/diff/159001/160011#newcode37
net/network_safety/network_safety_manager.h:37: void
NotifyRequestBodyWasMalformed(const GURL& url);
On 2010/07/21 07:31:57, eroman wrote:
> What is this used by?
Currently nobody, but if for example the SpellChecker LoadDictionary function
received a payload that is incoherent it could notify us and we would give the
server time to readjust by marking it as a failure.

http://codereview.chromium.org/2487001/diff/159001/160011#newcode43
net/network_safety/network_safety_manager.h:43: // Allow us to map an URL ID
with its entry.
On 2010/07/21 07:31:57, eroman wrote:
> what is a "URL ID" ?

I made the comment clearer.
From each URL we generate an ID composed of the host and query parameters that
allows us to unically map an entry to it.

http://codereview.chromium.org/2487001/diff/159001/160011#newcode64
net/network_safety/network_safety_manager.h:64: static unsigned int
requests_counter_;
On 2010/07/21 07:31:57, eroman wrote:
> why does this need to be static, i thought we were using a singleton?

Good catch, no reason at all

http://codereview.chromium.org/2487001/diff/159001/160011#newcode64
net/network_safety/network_safety_manager.h:64: static unsigned int
requests_counter_;
On 2010/07/21 07:31:57, eroman wrote:
> why does this need to be static, i thought we were using a singleton?

Good catch, no reason at all

http://codereview.chromium.org/2487001/diff/159001/160011#newcode69
net/network_safety/network_safety_manager.h:69: static const unsigned int
kRequestsBetweenCollecting;
On 2010/07/21 07:31:57, eroman wrote:
> Same question as above.

Same answer

http://codereview.chromium.org/2487001/diff/159001/160011#newcode69
net/network_safety/network_safety_manager.h:69: static const unsigned int
kRequestsBetweenCollecting;
On 2010/07/21 07:31:57, eroman wrote:
> Same question as above.

Same answer

http://codereview.chromium.org/2487001/diff/159001/160012
File net/network_safety/network_safety_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/159001/160012#newcode37
net/network_safety/network_safety_unittest.cc:37: static const int
k_add_constant() { return kAdditionalConstantMs; }
On 2010/07/21 07:31:57, eroman wrote:
> Instead of having public accessors to the constants, why not make the
constants
> in base class public?

Done.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode57
net/network_safety/network_safety_unittest.cc:57: std::string retry_value =
"0.0", int response_code = 0)
On 2010/07/21 07:31:57, eroman wrote:
> our style guide does not allow default parameters.

Done.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode65
net/network_safety/network_safety_unittest.cc:65: else
On 2010/07/21 07:31:57, eroman wrote:
> nit: our style is to not use "else" when the if returns.

Done.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode72
net/network_safety/network_safety_unittest.cc:72: if (key ==
MockNetworkSafetyEntry::k_retry_name())
On 2010/07/21 07:31:57, eroman wrote:
> simplify to return (key == ....);

Can't believe I did this sorry.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode72
net/network_safety/network_safety_unittest.cc:72: if (key ==
MockNetworkSafetyEntry::k_retry_name())
On 2010/07/21 07:31:57, eroman wrote:
> simplify to return (key == ....);

Can't believe I did this sorry.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode121
net/network_safety/network_safety_unittest.cc:121: TimeTicks time_;
On 2010/07/21 07:31:57, eroman wrote:
> We only use the trailing underscore for private members. Since this is public,
> just write "time".

Done.

http://codereview.chromium.org/2487001/diff/159001/160012#newcode132
net/network_safety/network_safety_unittest.cc:132: GURL url_;
On 2010/07/21 07:31:57, eroman wrote:
> see comment above.

Done.

http://codereview.chromium.org/2487001/diff/159001/160013
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/2487001/diff/159001/160013#newcode35
net/url_request/url_request_http_job.cc:35: #include
"net/network_safety/network_safety_header_adapter.h"
On 2010/07/21 07:31:57, eroman wrote:
> move this up.

Done.

http://codereview.chromium.org/2487001/diff/159001/160014
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/2487001/diff/159001/160014#newcode18
net/url_request/url_request_http_job.h:18: #include
"net/network_safety/network_safety_manager.h"
On 2010/07/21 07:31:57, eroman wrote:
> move this up to preserve sorted order.

Done.

Sign in to reply to this message.

malavv

Hi Eric, Thanks again for the very thorough code review you gave me in the ...

4 years, 10 months ago (2010-07-27 16:14:43 UTC) #29

Hi Eric,

Thanks again for the very thorough code review you gave me in the middle of
last week. I have made changes you requested, can you give it another look
to see if it would need further changes?

Thanks,
Maxime

On Thu, Jul 22, 2010 at 11:02 AM, Maxime Lavigne <malavv@google.com> wrote:

> Hi Eric,
>
> I really appreciate the time you put into doing this code review, I know
> that time is precious to you right now. I tried to apply all of your
> suggestions yesterday and to push it on the try-bots before answering to
> this mail, but I encountered problem with my client.
>
> On the matter of making the design document public, I was of hoping that
> once my CL is integrated to Chromium it could live in the design-docs
> section of Chromium.org. For the moment, I switched it to a @gmail account
> and to share with everyone.
> Public design-doc: Link to the public version of my design
document<https://docs.google.com/document/edit?id=1oqOh6oRqnGP2yz0yETdaxKh5MhP5U4gXa-Jd0bAwLM8&hl=en#>
>
> On another subject, the idea you suggested about histograms and metrics
> sounds like a good idea. This module is definitely not something your want
> to start behaving widely and refusing random requests. It would be a good
> addition to have some way in which we could monitor this. A tab in
> net-internal that contains a reset button also makes sense. I will look for
> implementing those recommendation when the main module will be integrated.
>
> As it seems to make consensus, I renamed the module from *NetworkSafety* to
> *RequestThrottler*. The external documentation still needs to be adjusted
> but I will do this shortly.
>
> Thanks,
> Maxime
>
> On Wed, Jul 21, 2010 at 4:28 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>
>> Thanks for a very thorough review Eric.
>>
>> We could change the name to RequestThrottler, that seems fairly
>> specific while still broad enough to capture that it does both
>> exponential back-off and back-off based on the custom header sites can
>> send back.
>>
>> Sounds like a good idea to let users override the throttling without
>> restarting the browser, although note that similar mechanisms have
>> existed in Toolbar and Desktop for a long time now and we've never had
>> a problem - generally the timeout doesn't get very long before the
>> server recovers.  For extensions, we're planning to have Maxime look
>> at detecting user-initiated requests vs. machine-generated requests
>> (something like assuming all user-generated events occur within half a
>> second or so of an event handler firing into the extension JavaScript)
>> and it might be a possibility to make these and other explicitly
>> user-generated requests (like hitting the refresh button) clear the
>> back-off settings for the given URL or a whole website.
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Jul 21, 2010 at 3:31 AM,  <eroman@chromium.org> wrote:
>> > Some high level comments before I dive into nits:
>> >
>> > (a) Nice design document!
>> >
>> > Can this document be made public. That way you can link to it from the
>> code
>> > review description, and from the header file.
>> >
>> > (b) Please add a description for this CL. To do this, click the "Edit
>> Issue"
>> > link on the review webpage.
>> >
>> > (c) How are we going to measure the effectiveness of this change?
>> >
>> > The general mechanism we use in Chrome for answering such questions are
>> > "histograms".
>> > You can read up about them here:
>> >
>>
https://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics#...
>> >
>> > Basically, I think we should have a way to see how often we are
>> throttling
>> > requests.
>> > This will help catch if there are problems and lots of users are getting
>> the
>> > errors, and also to evaluate the impact of tweaking the various backoff
>> > knobs.
>> >
>> > Note that currently we have the Net.ErrorCodesForMainFrame histogram
>> which
>> > measures error codes seen by user initiated navigations, so we will be
>> > seeing
>> > some ERR_REQUEST_REFUSED distributions show up in here (but only for a
>> > limited
>> > subset of requests).
>> >
>> > At any rate, additional histograms are best added as a separate
>> follow-up
>> > change
>> > so don't add it to this review, but I am interested to hear your
>> thoughts on
>> > it.
>> >
>> > (d) Will users be able to override the throttling? For example what if
>> > something
>> > went wrong and the user has now fixed the problem (uninstalled problem
>> > extension). They now want to load the webpage again, but keep getting
>> > getting
>> > ERR_REQUEST_REFUSED on each request. Perhaps refresh should bypass the
>> > throttling layer? Otherwise they either need to wait, or restart the
>> browser
>> > (which isn't really viable on chromeos).
>> >
>> > (e) Debugging and diagnostics: I recommend exposing the safety entry map
>> on
>> > about:net-internals. This will help with debugging, and could also be a
>> way
>> > for
>> > power-users to hit a button to reset the current entries. (This of
>> course is
>> > a
>> > separate change).
>> >
>> > (f) Naming: I am not very fond of "network safety", since it sounds more
>> > like a
>> > security layer. I think the keywords for this feature are more along the
>> > lines
>> > of "throttling", "ddos", "backoff". So maybe something like
>> > RequestThrottler, or
>> > DDOSProtector would be more descriptive.
>> >
>> >
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160002
>> > File net/base/net_error_list.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160002#newcode161
>> > net/base/net_error_list.h:161: NET_ERROR(REQUEST_REFUSED, -127)
>> > It is important for the error name to be as descriptive as possible,
>> > since for the users that hit this the symbolic name is all they
>> > information they have to diagnose the problem.
>> >
>> > Some useful keywords for users will be "throttled", "ddos", "aborted",
>> > "temporary"
>> >
>> > How about something like:
>> >
>> > TEMPORARILY_THROTTLED_BY_DDOS
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004
>> > File net/network_safety/network_safety_entry.cc (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode8
>> > net/network_safety/network_safety_entry.cc:8: #include <cmath>
>> > header orders here should be as follows:
>> >
>> > #include "net/network_safety/network_safety_entry.h"
>> >
>> > #include <cmath>
>> >
>> > #include "base/logging.h"
>> > #include "base/rand_util.h"
>> > #include "base/string_util.h"
>> > #include "net/network_safety/network_safety_header_interface.h"
>> >
>> >
>> > For more info see:
>> >
>>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode37
>> > net/network_safety/network_safety_entry.cc:37:
>> > NetworkSafetyHeaderInterface* response) {
>> > Can this be a const pointer instead, or a const reference?
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode65
>> > net/network_safety/network_safety_entry.cc:65:
>> > base::TimeDelta(GetTimeNow() - release_time_).InMilliseconds();
>> > indent continued lines by 4 spaces.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode86
>> > net/network_safety/network_safety_entry.cc:86:
>> > std::max(old_values_.number_of_failed_requests, num_of_time_delayed_);
>> > indent by 4 spaces.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode90
>> > net/network_safety/network_safety_entry.cc:90:
>> > std::max(old_values_.release_time, release_time_));
>> > indentation.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160004#newcode115
>> > net/network_safety/network_safety_entry.cc:115: DCHECK(conversion_is_ok)
>> > We need to be prepared to handle malformed header values.
>> >
>> > In chrome we use DCHECK as an assert for something that is IMPOSSIBLE to
>> > happen (if a DCHECK is triggered it results in a crash, and implies
>> > there was some incorrect logic in our code).
>> >
>> > So it is inappropriate to DCHECK() based on a user input, since it is
>> > possible for that input to be malformed.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005
>> > File net/network_safety/network_safety_entry.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode13
>> > net/network_safety/network_safety_entry.h:13: #include
>> > "base/ref_counted.h"
>> > nit: move ref_counted.h above time.h so they are in sorted order.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode22
>> > net/network_safety/network_safety_entry.h:22: // This method needs to be
>> > called prior to every requests; if it returns
>> > nit: "requests" --> "request"
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode42
>> > net/network_safety/network_safety_entry.h:42: // of our packet was
>> > malformed.
>> > wording: "packet" --> "packets". That said, I don't think packets is the
>> > right thing here.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode48
>> > net/network_safety/network_safety_entry.h:48: // Additional constant to
>> > further adjust back-off
>> > Please end with a period. Also, "additional" + "further" in the same
>> > sentence seems redundant.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode54
>> > net/network_safety/network_safety_entry.h:54: // Fuzzing percentage. ex:
>> > 10% will spread request randomly
>> > "request" --> "requests"
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode69
>> > net/network_safety/network_safety_entry.h:69: static const std::string
>> > kRetryHeaderName;
>> > Our style guide prohibits using static classes (as it adds initializer
>> > calls to startup).
>> >
>> > Instead, please define this as a const char[].
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode71
>> > net/network_safety/network_safety_entry.h:71: // Calculate when should
>> > we start sending request again. Follows a failure
>> > "Calculates when we should start sending requests again".
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode89
>> > net/network_safety/network_safety_entry.h:89: // Number of time we were
>> > delayed
>> > "time" --> "times".
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode90
>> > net/network_safety/network_safety_entry.h:90: int num_of_time_delayed_;
>> > "num_of_time_delayed_"  -->  "num_times_deleayed_"
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160005#newcode97
>> > net/network_safety/network_safety_entry.h:97: virtual
>> > ~NetworkSafetyEntry();
>> > Please move this to the top of the protected section.
>> >
>> > For more info see:
>> >
>> >
>>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160006
>> > File net/network_safety/network_safety_entry_interface.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode16
>> > net/network_safety/network_safety_entry_interface.h:16:
>> > NetworkSafetyEntryInterface() { }
>> > nit: collapse empty blocks as {}
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode25
>> > net/network_safety/network_safety_entry_interface.h:25: virtual
>> > ~NetworkSafetyEntryInterface() { }
>> > {}
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160006#newcode28
>> > net/network_safety/network_safety_entry_interface.h:28:
>> > DISALLOW_COPY_AND_ASSIGN(NetworkSafetyEntryInterface);
>> > Please include "base/basictypes.h" when using this macro.
>> >
>> > (It is included indirectly from base/ref_counted.h, but generally nice
>> > to be explicit on the dependencies).
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160009
>> > File net/network_safety/network_safety_header_interface.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160009#newcode11
>> > net/network_safety/network_safety_header_interface.h:11: class
>> > NetworkSafetyHeaderInterface {
>> > I am not convinced this interface is worthwhile, since it is pretty much
>> > a direct mapping to HttpResponseHeaders.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010
>> > File net/network_safety/network_safety_manager.cc (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode24
>> > net/network_safety/network_safety_manager.cc:24: requests_counter_ = 0;
>> > perhaps this would be better called "requests_since_last_gc_"
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode39
>> > net/network_safety/network_safety_manager.cc:39: :
>> > current_loop_(MessageLoop::current()) {
>> > It is unfortunate that we can't simply extend NetworkSafetyManager from
>> > NonThreadSafe (base/non_thread_safe.h).
>> >
>> > I guess that wouldn't work though, since the destructor will get called
>> > from the UI thread.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode47
>> > net/network_safety/network_safety_manager.cc:47: std::string
>> > NetworkSafetyManager::GetIdFromUrl(const GURL& url) {
>> > IMPORTANT: stripping down URLs is fairly complicated, I think we would
>> > be safer by building "up" the URL.
>> >
>> > In particular, one thing I think is not handled by GetWithEmptyPath() is
>> > embedded identities in the URL.
>> >
>> > For example:
>> >
>> > http://foo:bar@google.com/x
>> > http://foo1:bar@google.com/x
>> >
>> > Are conceptually the same URL, however they will result in a different
>> > "ID" I believe.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode57
>> > net/network_safety/network_safety_manager.cc:57:
>> > DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_IO);
>> > Why not do DCHECK_EQ(current_loop_, MessageLoop::current()) ?
>> >
>> > Or could get the thread id and use that for comparisons.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode60
>> > net/network_safety/network_safety_manager.cc:60: while (i !=
>> > url_entries_.end()) {
>> > It is probably a good idea to also have an upper bound on the size the
>> > map can grow to, to make sure we cant end up with hundreds of thousands
>> > of outtaded entries that can't be collected.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160010#newcode62
>> > net/network_safety/network_safety_manager.cc:62: UrlEntryMap::iterator
>> > tmp = i++;
>> > I think you can just put the expression directly in place of tmp.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011
>> > File net/network_safety/network_safety_manager.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode1
>> > net/network_safety/network_safety_manager.h:1: // Copyright (c)
>> > 2006-2010 The Chromium Authors. All rights reserved.
>> > just use 2010 as the copyright date.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode15
>> > net/network_safety/network_safety_manager.h:15: #include
>> > "base/scoped_ptr.h"
>> > keep in sorted order
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode37
>> > net/network_safety/network_safety_manager.h:37: void
>> > NotifyRequestBodyWasMalformed(const GURL& url);
>> > What is this used by?
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode43
>> > net/network_safety/network_safety_manager.h:43: // Allow us to map an
>> > URL ID with its entry.
>> > what is a "URL ID" ?
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode64
>> > net/network_safety/network_safety_manager.h:64: static unsigned int
>> > requests_counter_;
>> > why does this need to be static, i thought we were using a singleton?
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160011#newcode69
>> > net/network_safety/network_safety_manager.h:69: static const unsigned
>> > int kRequestsBetweenCollecting;
>> > Same question as above.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012
>> > File net/network_safety/network_safety_unittest.cc (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode37
>> > net/network_safety/network_safety_unittest.cc:37: static const int
>> > k_add_constant() { return kAdditionalConstantMs; }
>> > Instead of having public accessors to the constants, why not make the
>> > constants in base class public?
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode57
>> > net/network_safety/network_safety_unittest.cc:57: std::string
>> > retry_value = "0.0", int response_code = 0)
>> > our style guide does not allow default parameters.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode65
>> > net/network_safety/network_safety_unittest.cc:65: else
>> > nit: our style is to not use "else" when the if returns.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode72
>> > net/network_safety/network_safety_unittest.cc:72: if (key ==
>> > MockNetworkSafetyEntry::k_retry_name())
>> > simplify to return (key == ....);
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode121
>> > net/network_safety/network_safety_unittest.cc:121: TimeTicks time_;
>> > We only use the trailing underscore for private members. Since this is
>> > public, just write "time".
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160012#newcode132
>> > net/network_safety/network_safety_unittest.cc:132: GURL url_;
>> > see comment above.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160013
>> > File net/url_request/url_request_http_job.cc (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160013#newcode35
>> > net/url_request/url_request_http_job.cc:35: #include
>> > "net/network_safety/network_safety_header_adapter.h"
>> > move this up.
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160014
>> > File net/url_request/url_request_http_job.h (right):
>> >
>> > http://codereview.chromium.org/2487001/diff/159001/160014#newcode18
>> > net/url_request/url_request_http_job.h:18: #include
>> > "net/network_safety/network_safety_manager.h"
>> > move this up to preserve sorted order.
>> >
>> > http://codereview.chromium.org/2487001/show
>> >
>>
>
>

Sign in to reply to this message.

eroman

Thanks for making those changes. I think we are getting close now. (This is a ...

4 years, 10 months ago (2010-07-28 01:00:15 UTC) #30

Jói

Thanks again Eric for such a thorough review. One response to one of your comments, ...

4 years, 10 months ago (2010-07-28 15:08:17 UTC) #31

malavv

Thanks Eric for this code review. I sent you a mail this morning about the ...

4 years, 10 months ago (2010-07-28 15:58:53 UTC) #32

eroman

LGTM. My main remaining high level comment is about the singleton RequestThrottlingManager. We try very ...

4 years, 10 months ago (2010-07-29 23:03:50 UTC) #33

LGTM.

My main remaining high level comment is about the singleton
RequestThrottlingManager.

We try very hard to avoid use of globals, as it causes problem when
multithreading, and also makes the dependencies less obvious.

Instead, a good place to put RequestThrottlingManager is as an instance variable
in URLRequestContext. 

URLRequestContext is effectively the "network stack globals", and allows
swapping in different dependencies for different requests (and isolates all the
dependencies for use on a single thread).

Of course moving from a singleton to an instance variable of the context would
result in a subtle behavior change -- instead of being global accross *all*
requests, it would be throttling across requests per context.

In practice this ends up being much the same thing, since Chrome issues almost
all its requests through a "primary" URLRequestContext. Chrome uses a separate
context for "incognito" windows (which is how they end up having a different
cookie store), so it would mean throttling in incognito windows would not use
the information from what had happened in regular windows.

I would like for the removal of singleton, but it doesn't need to block this
check-in

http://codereview.chromium.org/2487001/diff/205002/207007
File net/request_throttler/request_throttler_header_adapter.cc (right):

http://codereview.chromium.org/2487001/diff/205002/207007#newcode9
net/request_throttler/request_throttler_header_adapter.cc:9: :
response_header_(headers) {
nit: indent only 4 spaces total on this one.

http://codereview.chromium.org/2487001/diff/205002/207011
File net/request_throttler/request_throttler_manager.h (right):

http://codereview.chromium.org/2487001/diff/205002/207011#newcode37
net/request_throttler/request_throttler_manager.h:37: void
NotifyRequestBodyWasMalformed(const GURL& url);
This is unused. It should probably be removed.

http://codereview.chromium.org/2487001/diff/205002/207011#newcode43
net/request_throttler/request_throttler_manager.h:43: // From each URL we
generate an ID composed of the host and query parameters
this comment talks of "query parameters", however those are not being included
in the ID (only the path is).

http://codereview.chromium.org/2487001/diff/205002/207011#newcode69
net/request_throttler/request_throttler_manager.h:69: // Number of request that
will be made between garbage collection.
request --> requests.

http://codereview.chromium.org/2487001/diff/205002/207011#newcode70
net/request_throttler/request_throttler_manager.h:70: const unsigned int
kRequestsBetweenCollecting;
style nit: For these, I suggest either making them static, or else naming them
like_regular_member_variables_;

http://codereview.chromium.org/2487001/diff/205002/207012
File net/request_throttler/request_throttler_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/205002/207012#newcode53
net/request_throttler/request_throttler_unittest.cc:53:
MockRequestThrottlerHeaderAdapter(const std::string retry_value,
nit: please use "const std::string&"

http://codereview.chromium.org/2487001/diff/205002/207012#newcode78
net/request_throttler/request_throttler_unittest.cc:78: // Method to process the
url using RequestThrottlerManager protected method
minor nit: please end all of these comments with a period (.)

http://codereview.chromium.org/2487001/diff/205002/207012#newcode119
net/request_throttler/request_throttler_unittest.cc:119: GurlAndString(GURL
url_value, std::string expected, int line_num) {
nit: "const std::string& expected".

http://codereview.chromium.org/2487001/diff/205002/207012#newcode149
net/request_throttler/request_throttler_unittest.cc:149:
TEST_F(RequestThrottlerEntryTest, interfaceRequestNotAllowed) {
nit: We generally start our test names with a capital letter. Please change
"interface" --> "Interface"

http://codereview.chromium.org/2487001/diff/205002/207012#newcode169
net/request_throttler/request_throttler_unittest.cc:169: "When the server put a
positive value in retry-after we should" <<
nit: add a space at the end of the string to join properly with next line.

Also, can remove the << as the string literal will be auto-joined.

Sign in to reply to this message.

malavv

I corrected the changes you had requested. http://codereview.chromium.org/2487001/diff/205002/207007 File net/request_throttler/request_throttler_header_adapter.cc (right): http://codereview.chromium.org/2487001/diff/205002/207007#newcode9 net/request_throttler/request_throttler_header_adapter.cc:9: : response_header_(headers) ...

4 years, 10 months ago (2010-07-30 20:49:34 UTC) #34

I corrected the changes you had requested.

http://codereview.chromium.org/2487001/diff/205002/207007
File net/request_throttler/request_throttler_header_adapter.cc (right):

http://codereview.chromium.org/2487001/diff/205002/207007#newcode9
net/request_throttler/request_throttler_header_adapter.cc:9: :
response_header_(headers) {
On 2010/07/29 23:03:50, eroman wrote:
> nit: indent only 4 spaces total on this one.

Done.

http://codereview.chromium.org/2487001/diff/205002/207011
File net/request_throttler/request_throttler_manager.h (right):

http://codereview.chromium.org/2487001/diff/205002/207011#newcode37
net/request_throttler/request_throttler_manager.h:37: void
NotifyRequestBodyWasMalformed(const GURL& url);
On 2010/07/29 23:03:50, eroman wrote:
> This is unused. It should probably be removed.

It is unused for the moment because we did not add this to any upper level logic
but when it will be there developers will be able to use it. We unit test this
component by directly using its entry instead of using the manager's method.

http://codereview.chromium.org/2487001/diff/205002/207011#newcode43
net/request_throttler/request_throttler_manager.h:43: // From each URL we
generate an ID composed of the host and query parameters
On 2010/07/29 23:03:50, eroman wrote:
> this comment talks of "query parameters", however those are not being included
> in the ID (only the path is).

Done.

http://codereview.chromium.org/2487001/diff/205002/207011#newcode69
net/request_throttler/request_throttler_manager.h:69: // Number of request that
will be made between garbage collection.
On 2010/07/29 23:03:50, eroman wrote:
> request --> requests.

Done.

http://codereview.chromium.org/2487001/diff/205002/207011#newcode70
net/request_throttler/request_throttler_manager.h:70: const unsigned int
kRequestsBetweenCollecting;
On 2010/07/29 23:03:50, eroman wrote:
> style nit: For these, I suggest either making them static, or else naming them
> like_regular_member_variables_;

Done.

http://codereview.chromium.org/2487001/diff/205002/207012
File net/request_throttler/request_throttler_unittest.cc (right):

http://codereview.chromium.org/2487001/diff/205002/207012#newcode53
net/request_throttler/request_throttler_unittest.cc:53:
MockRequestThrottlerHeaderAdapter(const std::string retry_value,
On 2010/07/29 23:03:50, eroman wrote:
> nit: please use "const std::string&"

Done.

http://codereview.chromium.org/2487001/diff/205002/207012#newcode78
net/request_throttler/request_throttler_unittest.cc:78: // Method to process the
url using RequestThrottlerManager protected method
On 2010/07/29 23:03:50, eroman wrote:
> minor nit: please end all of these comments with a period (.)

Done.

http://codereview.chromium.org/2487001/diff/205002/207012#newcode119
net/request_throttler/request_throttler_unittest.cc:119: GurlAndString(GURL
url_value, std::string expected, int line_num) {
On 2010/07/29 23:03:50, eroman wrote:
> nit: "const std::string& expected".

Done.

http://codereview.chromium.org/2487001/diff/205002/207012#newcode149
net/request_throttler/request_throttler_unittest.cc:149:
TEST_F(RequestThrottlerEntryTest, interfaceRequestNotAllowed) {
On 2010/07/29 23:03:50, eroman wrote:
> nit: We generally start our test names with a capital letter. Please change
> "interface" --> "Interface"

Done.

http://codereview.chromium.org/2487001/diff/205002/207012#newcode169
net/request_throttler/request_throttler_unittest.cc:169: "When the server put a
positive value in retry-after we should" <<
On 2010/07/29 23:03:50, eroman wrote:
> nit: add a space at the end of the string to join properly with next line.
> 
> Also, can remove the << as the string literal will be auto-joined.

Done.

Sign in to reply to this message.

eroman

Do you have anyone that can commit this for you? (if not I can land ...

4 years, 10 months ago (2010-08-02 23:44:18 UTC) #35

Jói

I can land this for Maxime. Cheers, Jói On Mon, Aug 2, 2010 at 7:44 ...

4 years, 10 months ago (2010-08-02 23:48:02 UTC) #36

Jói

(and I believe Maxime is planning to switch from singleton to a member of URLRequestContext ...

4 years, 10 months ago (2010-08-03 00:15:02 UTC) #37

eroman

4 years, 10 months ago (2010-08-03 00:35:44 UTC) #38

> and I believe Maxime is planning to switch from singleton to 
> a member of URLRequestContext as per your comment, in a 
> follow-up change)

Great!

So the follow-ups to this CL will be (a) Make sure we have a histogram to see
how often this gets hit (b) Switch from singleton to per-URLRequestContext
state.

LGTM.

Sign in to reply to this message.

Issue 2487001: Request Throttler : Exponential back-off (Closed)

Description

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : Garbage collecting entries + url_id include path #

Patch Set 6 : Support for X-Retry-After + fix race conditions #

Patch Set 7 : '' #

Patch Set 8 : Modification of net.gyp file in order to include my changes. #

Patch Set 9 : Fixed smelly code + some code review tips #

Patch Set 10 : Many changes that were propose 6 of june #

Patch Set 11 : '' #

Patch Set 12 : Fixed code review stuff and race conditions. #

Patch Set 13 : Remove apparently unnecessairy Autolock() and added DCHECK for that #

Patch Set 14 : Same but that passes LINT #

Patch Set 15 : Upload without locks and with thread check. #

Patch Set 16 : Correction for this morning code review. 6/15/2010 #

Patch Set 17 : '' #

Patch Set 18 : '' #

Patch Set 19 : '' #

Patch Set 20 : Corrected two nits #

Patch Set 21 : Correction of details pointed by Joi #

Patch Set 22 : Added explicit conversion #

Patch Set 23 : Fixed problem with non-creation of transaction on failure + corrected compile & unittest #

Patch Set 24 : Changed name to be more relevant and corrected errors pointed out by Eric #

Patch Set 25 : It's been so long now I was 1k revision behind I updated the client and resolved conflict. #

Patch Set 26 : missing piece #

Patch Set 27 : Had problem with my client sync #

Patch Set 28 : Fixed unit test to pass try bots #

Patch Set 29 : Fixed changes suggested by Eric Roman, adjusted nits around code. #

Patch Set 30 : Corrected final nits #

Patch Set 31 : Sync with head #

Patch Set 32 : Corrected a flaky test due to having 2 implementation of request throttling #

Messages