Implement throttling logic for fetching affiliation information.

components/password_manager/core/browser/affiliation_fetch_throttler.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/password_manager/core/browser/affiliation_fetch_throttler.h"
+
+#include "base/logging.h"
+#include "base/rand_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+
+namespace password_manager {
+
+namespace {
+
+net::BackoffEntry::Policy kAffiliationBackoffParameters = {

[mmenke, 2015/01/14 19:25:07]

const

[engedy, 2015/01/15 19:16:42]

Done.

+    // Number of initial errors (in sequence) to ignore before going into
+    // exponential backoff.
+    0,
+
+    // Initial delay (in ms) once backoff starts.
+    10 * 1000,  // 10 seconds
+
+    // Factor by which the delay will be multiplied on each subsequent failure.
+    4,
+
+    // Fuzzing percentage: 50% will spread delays randomly between 50%--100% of
+    // the nominal time.
+    .5,  // 50%
+
+    // Maximum delay (in ms) during exponential backoff.
+    6 * 3600 * 1000,  // 6 hours
+
+    // Time to keep an entry from being discarded even when it has no
+    // significant state, -1 to never discard. (Not applicable.)
+    -1,
+
+    // False means that initial_delay_ms is the first delay once we start
+    // exponential backoff, i.e., there is no delay after subsequent successful
+    // requests.
+    false,
+};
+
+// Grace period before the first request is sent after network connectivity is
+// restored. The fuzzing factor from above applies.
+const int64 kGracePeriodAfterNetworkRestoredInMs = 10 * 1000;  // 10 seconds

[mmenke, 2015/01/14 19:25:07]

Should include basictypes.h for int64

[engedy, 2015/01/15 19:16:42]

Ahh, it looks like that is already deprecated. I have included <stdint.h> and
changed this to int64_t instead.

+
+// Implementation of net::BackoffEntry that allows mocking its tick source.
+class BackoffEntryImpl : public net::BackoffEntry {
+ public:
+  BackoffEntryImpl(base::TickClock* tick_clock)

[mmenke, 2015/01/14 19:25:07]

explicit

[engedy, 2015/01/15 19:16:42]

Done.

+      : BackoffEntry(&kAffiliationBackoffParameters), tick_clock_(tick_clock) {}
+  ~BackoffEntryImpl() override{};

[mmenke, 2015/01/14 19:25:07]

nit:  space after "override"

[engedy, 2015/01/15 19:16:42]

Ahh, actually, there is also a superfluous semi-colon in here. The lack of space
was, in fact, a clang-format bug.

+
+ protected:
+  // net::BackoffEntry:
+  base::TimeTicks ImplGetTimeNow() const override {

[mmenke, 2015/01/14 19:25:06]

optional:  This can be private (parent classes can call their own virtual
methods, even if private in subclasses).  Generally feel if something doesn't
need to be protected, may as well just make it private, but if you prefer to
keep protected, I can certainly live with that.

[engedy, 2015/01/15 19:16:42]

Done.

+    return tick_clock_->NowTicks();
+  }
+
+ private:
+  // Must outlive this instance.
+  base::TickClock* tick_clock_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackoffEntryImpl);
+};
+
+}  // namespace
+
+AffiliationFetchThrottler::AffiliationFetchThrottler(
+    AffiliationFetchThrottlerDelegate* delegate,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    scoped_ptr<base::TickClock> tick_clock)
+    : delegate_(delegate),
+      task_runner_(task_runner),
+      state_(IDLE),
+      has_network_connectivity_(false),
+      is_fetch_scheduled_(false),
+      tick_clock_(tick_clock.Pass()),
+      exponential_backoff_(new BackoffEntryImpl(tick_clock_.get())),
+      weak_ptr_factory_(this) {
+  DCHECK(delegate);
+  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);

[mmenke, 2015/01/14 19:25:06]

Putting this in a constructor seems like a bad idea - could end up dereferencing
a pointer this |this|'s vtable on another thread before construction completes. 
Since we don't subclass this, that works, but, if we did subclass this, that
would be a problem.

Think it's best not to depend on such things, so I'd suggest only start watching
when a network request is needed, and stop when we return to idle state (Or add
some other method to start watching the NCN).

[engedy, 2015/01/15 19:16:42]

I think that is a bit over-cautious. The contract of ObserverList explicitly
states that notifications will be delivered to |this| on the thread where it has
registered from. It also states that it will always PostTask(), so we cannot get
a synchronous callback either.

In fact, 10 out of 12 users of NetworkChangeNotifier register themselves in
their constructors, so if this ever changes without proper refactorings, it will
break lots of users.

+  has_network_connectivity_ = !net::NetworkChangeNotifier::IsOffline();
+}
+
+AffiliationFetchThrottler::~AffiliationFetchThrottler() {
+  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+}
+
+void AffiliationFetchThrottler::SignalNetworkRequestNeeded() {
+  if (state_ != IDLE)
+    return;
+
+  state_ = FETCH_NEEDED;
+  if (has_network_connectivity_)
+    EnsureCallbackIsScheduled();
+}
+
+void AffiliationFetchThrottler::InformOfNetworkRequestComplete(bool success) {
+  DCHECK_EQ(state_, FETCH_IN_FLIGHT);
+  state_ = IDLE;
+  exponential_backoff_->InformOfRequest(success);

[mmenke, 2015/01/14 20:43:43]

Oops...Got so sidetracked thinking about the NCN stuff, that I somehow missed
this.  I'd still suggest we start without the NCN stuff for now, because smaller
CLs make life easier, and reviewing 600 line CLs is a pain, but I'm less
concerned that we're missing stuff.

[engedy, 2015/01/15 19:16:42]

Please see my other comment.

+}
+
+void AffiliationFetchThrottler::EnsureCallbackIsScheduled() {
+  DCHECK_EQ(state_, FETCH_NEEDED);
+  if (is_fetch_scheduled_)
+    return;
+
+  is_fetch_scheduled_ = true;
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&AffiliationFetchThrottler::OnBackoffDelayExpiredCallback,
+                 weak_ptr_factory_.GetWeakPtr()),
+      exponential_backoff_->GetTimeUntilRelease());
+}
+
+void AffiliationFetchThrottler::OnBackoffDelayExpiredCallback() {
+  DCHECK_EQ(state_, FETCH_NEEDED);
+  DCHECK(is_fetch_scheduled_);
+  is_fetch_scheduled_ = false;
+
+  // Do nothing if network connectivity was lost while this callback was in the
+  // task queue. The callback will be posted in the OnConnectionTypeChanged
+  // handler once again.
+  if (!has_network_connectivity_)
+    return;
+
+  // The release time might have been increased if network connectivity was lost
+  // and restored while this callback was in the task queue. If so, reschedule.
+  if (exponential_backoff_->ShouldRejectRequest())
+    EnsureCallbackIsScheduled();
+  else
+    state_ = delegate_->OnCanSendNetworkRequest() ? FETCH_IN_FLIGHT : IDLE;
+}
+
+void AffiliationFetchThrottler::OnConnectionTypeChanged(
+    net::NetworkChangeNotifier::ConnectionType type) {
+  if (has_network_connectivity_ == !net::NetworkChangeNotifier::IsOffline())

[mmenke, 2015/01/14 19:25:06]

What if we're switching from WIFI to cellular?

[mmenke, 2015/01/14 19:25:07]

Shouldn't call NetworkChangeNotifier::IsOffline twice - it can be expensive on
windows.  Actually, we're already given a ConnectionType, let's just use "type
== net::NetworkChangeNotifier::CONNECTION_NONE" instead.

[engedy, 2015/01/15 19:16:42]

Done.

[engedy, 2015/01/15 19:16:42]

Hmm. So far I did not consider the kind of the connection to be relevant: during
normal operation, the requests should be few and far between (1-2 requests per
day, <10kB in size). Do you think we should make a distinction?

[mmenke, 2015/01/16 21:28:34]

Now that I realize how this class actually works, I think we're fine.  (Also,
looking at NCN, it gives us a bogus transaction to being disconnected in that
case, anyways)

+    return;
+
+  has_network_connectivity_ = !net::NetworkChangeNotifier::IsOffline();
+  if (!has_network_connectivity_)
+    return;

[mmenke, 2015/01/14 19:25:06]

While being offline is one of the most common causes of network errors, it only
accounts for about 15% of total network errors for main frames.  If you're
concerned about being offline, you should be concerned about the other errors,
too, and do some sort of retry with backoff for them, too, right?

I'd suggest just retrying on errors, with exponential backoff, gathering
metrics, and then figuring out if adding in NCN support would be helpful in
reducing the number of retries / getting the actual thing we want sooner.

[mmenke, 2015/01/14 19:25:07]

No provisions to retry here?

[engedy, 2015/01/15 19:16:42]

I am not sure I understand. Are you implying that NetworkChangeNotifier can have
false-negatives to the extent that we should retry after some time even while
NetworkChangeNotifier reports no connectivity?

[engedy, 2015/01/15 19:16:42]

Hmm. I believe this is somewhat different than network errors during mainframe
navigations. For one, I would suspect that those statistics apply here only
partially, as users do not try to browse for much longer after they have
realized there is no network connection, but the requests controlled by this
class would be initiated in the background, periodically, and there would be no
way for the user to forcibly trigger them.

For other kinds of network errors, yes, I agree that there should be exponential
back-off and retry. But in the case of the pseudo-error that there is no
connectivity, it would strike me as bad UX if password filling didn't work for
some hours after you returned from a hike in the mountains.

[mmenke, 2015/01/16 21:28:34]

Sorry, got confused about how this would plug in (This works like BackoffEntry,
which works a bit weirdly - instead of being responsible for dealing with the
request itself, the consumer is responsible for telling it the status of the
request, so some of my earlier comments were confused.  Been a long week).

+
+  double grace_ms =
+      kGracePeriodAfterNetworkRestoredInMs *
+      (1 - base::RandDouble() * kAffiliationBackoffParameters.jitter_factor);
+  base::TimeTicks release_time = std::max(
+      exponential_backoff_->GetReleaseTime(),
+      tick_clock_->NowTicks() + base::TimeDelta::FromMillisecondsD(grace_ms));
+  // Note that SetCustomReleaseTime() takes the maximum of the current release
+  // time and the supplied |release_time|.
+  exponential_backoff_->SetCustomReleaseTime(release_time);
+
+  if (state_ == FETCH_NEEDED)
+    EnsureCallbackIsScheduled();
+}
+
+}  // namespace password_manager