Adds backoff and Min Wait Duration compliance to Protocolmanager pver4 handlers.

Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867
Committed: https://crrev.com/9078b8a7df0bf5ecff5042c4a50377bea4fba407
Cr-Commit-Position: refs/heads/master@{#369089}

[kcarattini, 2016-01-04 05:39:06]

Description was changed from

==========
Adds backoff and Min Wait Duration compliance to Protocolmanager pver4 handlers.

BUG=561867
==========

to

==========
Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867
==========

[kcarattini, 2016-01-04 05:40:35]

kcarattini@chromium.org changed reviewers:
+ awoz@google.com, nparker@chromium.org, vakh@google.com

[Nathan Parker, 2016-01-07 06:06:58]

Looks reasonable to me. Can we test that wait_duration is parsed properly, and
used?

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:733: base::TimeDelta
SafeBrowsingProtocolManager::GetNextV4BackOffInterval(
This could be static, if you remove the CalledOnValidThread.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:740: *multiplier *= 2;
What happens on integer overflow?  Probably want to keep it at the clipped 24hrs
value rather than having multiplier go to zero.  I guess it'd be after ~64 days,
but still.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:270:
EXPECT_TRUE(pm->next_gethash_v4_time_ >= now + TimeDelta::FromMinutes(15) &&
These should probably use EXPECT_GE and EXPECT_LE, so that if they fail it'll
give the real values rather than just "not true"

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:319:
EXPECT_TRUE(pm->next_gethash_v4_time_ == now + TimeDelta::FromHours(24));
EXPECT_EQ

[kcarattini, 2016-01-07 06:48:54]

Thanks again, Nathan. PTAL.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:733: base::TimeDelta
SafeBrowsingProtocolManager::GetNextV4BackOffInterval(
On 2016/01/07 06:06:58, nparker wrote:
> This could be static, if you remove the CalledOnValidThread.

Done.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:740: *multiplier *= 2;
On 2016/01/07 06:06:58, nparker wrote:
> What happens on integer overflow?  Probably want to keep it at the clipped
24hrs
> value rather than having multiplier go to zero.  I guess it'd be after ~64
days,
> but still.

Done.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:270:
EXPECT_TRUE(pm->next_gethash_v4_time_ >= now + TimeDelta::FromMinutes(15) &&
On 2016/01/07 06:06:58, nparker wrote:
> These should probably use EXPECT_GE and EXPECT_LE, so that if they fail it'll
> give the real values rather than just "not true"

Done.

https://codereview.chromium.org/1555953002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:319:
EXPECT_TRUE(pm->next_gethash_v4_time_ == now + TimeDelta::FromHours(24));
On 2016/01/07 06:06:58, nparker wrote:
> EXPECT_EQ

Done.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:431: EXPECT_LE(now +
base::TimeDelta::FromSeconds(400), pm->next_gethash_v4_time_);
Added a test for minimum_wait_duration here.

[vakh (old account. dont use), 2016-01-08 00:17:41]

lgtm

[awoz, 2016-01-11 23:00:00]

lgtm

[kcarattini, 2016-01-12 01:16:55]

I think I'm still waiting for an lgtm from Nathan.

Thanks,
Kendra

[Nathan Parker, 2016-01-12 04:14:38]

lgtm

We should certainly run some manual e2e tests of the backoff code, once you've
got it wired up.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:137: // Backoff interval is
MIN(((2^(n-1))*15 minutes) * (RAND + 1), 24 hours) where
Is there a public doc we can ref with this spec? I think it's not public yet...
but awoz might have one in the works.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:176:
next_gethash_v4_time_(Time::Now()),
How about just zero?  Then if the clock drifts backwards slightly, we don't have
an issue.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:476: gethash_v4_error_count_ =
0;
Might be tidier to put this in a ResetGetHashV4Errors() function so these values
are only set in a few small functions.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:371: // The number of HTTP
response errors, used for request backoff timing.
number of HTTP response error since the last successful HTTP response (ya?)

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:411: base::Time
next_gethash_v4_time_;
Is this the max of the prescribed min-wait-time and the backoff time?

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:431: EXPECT_LE(now +
base::TimeDelta::FromSeconds(400), pm->next_gethash_v4_time_);
On 2016/01/07 06:48:53, kcarattini wrote:
> Added a test for minimum_wait_duration here.

Shouldn't this be eq, since you set min_wait_duration to 400 above?

[kcarattini, 2016-01-12 05:18:07]

Thanks for getting back to me!

Kendra

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:137: // Backoff interval is
MIN(((2^(n-1))*15 minutes) * (RAND + 1), 24 hours) where
On 2016/01/12 04:14:38, nparker wrote:
> Is there a public doc we can ref with this spec? I think it's not public
yet...
> but awoz might have one in the works.

I don't know of one either.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:176:
next_gethash_v4_time_(Time::Now()),
On 2016/01/12 04:14:38, nparker wrote:
> How about just zero?  Then if the clock drifts backwards slightly, we don't
have
> an issue.

Done.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:476: gethash_v4_error_count_ =
0;
On 2016/01/12 04:14:38, nparker wrote:
> Might be tidier to put this in a ResetGetHashV4Errors() function so these
values
> are only set in a few small functions.

Done.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.h (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:371: // The number of HTTP
response errors, used for request backoff timing.
On 2016/01/12 04:14:38, nparker wrote:
> number of HTTP response error since the last successful HTTP response (ya?)

Done.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.h:411: base::Time
next_gethash_v4_time_;
On 2016/01/12 04:14:38, nparker wrote:
> Is this the max of the prescribed min-wait-time and the backoff time?

No, it should be one or the other. Added a comment.

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1555953002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:431: EXPECT_LE(now +
base::TimeDelta::FromSeconds(400), pm->next_gethash_v4_time_);
On 2016/01/12 04:14:38, nparker wrote:
> On 2016/01/07 06:48:53, kcarattini wrote:
> > Added a test for minimum_wait_duration here.
> 
> Shouldn't this be eq, since you set min_wait_duration to 400 above?

They're different because the min_wait_duration set above is parsed to a
TimeDelta object, and next_gethash_v4_time is a Time object, calculated by
adding Now() to the delta. Becuase Now() in the Parse function is slightly later
than Now() in the test, it is an LE check.

[kcarattini, 2016-01-13 02:52:29]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-01-13 02:52:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, awoz@google.com, vakh@google.com
Link to the patchset: https://codereview.chromium.org/1555953002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-13 02:54:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1555953002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1555953002/120001

[commit-bot: I haz the power, 2016-01-13 03:40:58]

Description was changed from

==========
Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867
==========

to

==========
Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867
==========

[commit-bot: I haz the power, 2016-01-13 03:40:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-13 03:42:38]

Description was changed from

==========
Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867
==========

to

==========
Adds backoff and Min Wait Duration compliance to ProtocolManager pver4 handlers.

BUG=561867

Committed: https://crrev.com/9078b8a7df0bf5ecff5042c4a50377bea4fba407
Cr-Commit-Position: refs/heads/master@{#369089}
==========

[commit-bot: I haz the power, 2016-01-13 03:42:39]

Patchset 7 (id:??) landed as
https://crrev.com/9078b8a7df0bf5ecff5042c4a50377bea4fba407
Cr-Commit-Position: refs/heads/master@{#369089}