Reland "DnsConfigServiceWin::ConfigReader: Add cap on the number of r…

…etries"

This is a reland of 180c0ad which
itself was a reland.

Test failures in an existing test (SerialWorkerTest.DeleteDuringWork)
were seen in the win-asan builder which led to a revert.

I was able to reproduce the issue locally even without this change,
though it happened less frequently. The race involved the reply task
not having completed even after the base::RunLoop().RunUntilIdle() call.
To address it, I've moved to call WithTaskEnvironment::RunUntilIdle; I
ran 100000 iterations and can no longer repro the issue.

Original change's description:
> Reland "DnsConfigServiceWin::ConfigReader: Add cap on the number of
> retries"
>
> This is a reland of 5813733
>
> The prior commit encountered an esoteric race condition in the task
> system. With the changes from https://crrev.com/c/3193150 and
> https://crrev.com/c/3227011, I can no longer repro it (I previously
> could).
>
> Original change's description:
> > DnsConfigServiceWin::ConfigReader: Add cap on the number of retries
> >
> > Add retry logic to SerialWorker and remove the existing retry logic in
> > DnsConfigServiceWin::ConfigReader in favor of using the SerialWorker
> > functionality with new logic to cap the number of retries.
>
> > The DnsConfigServiceWin::ConfigReader retry logic was added in case of
> > race conditions between getting registry change notifications and
> > reading out DNS configuration from the OS.
>
> > When in an offline state, no DNS servers are available. Prior to this
> > change, the ConfigReader wakes up every 5 seconds to try again which
> > has a negative impact on performance and battery life since the CPU
> > can't stay in a low power state. After this change, we will retry 3
> > times with a base delay of 5 seconds for the first retry and a 2x
> > exponential backoff for each additional retry.
>
> > If a state change is seen (e.g. via the registry watchers and hosts
> > file watcher), the retry count is reset.
>
> > Bug: 1225838
> > Change-Id: Idf801b1e2934649021df08dbfe9569fd81e7ae84
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3036528
> > Commit-Queue: Erik Anderson <Erik.Anderson@microsoft.com>
> > Reviewed-by: Eric Orth <ericorth@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#905004}
>
> Bug: 1225838
> Change-Id: I0e889515911660ec7dc5df50ddec18cf774ee1f0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3093529
> Reviewed-by: Adam Rice <ricea@chromium.org>
> Commit-Queue: Erik Anderson <Erik.Anderson@microsoft.com>
> Cr-Commit-Position: refs/heads/main@{#973813}

Bug: 1225838
Change-Id: I62be471bcc5864aca6494b5158f68af9df0c545d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3486516
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Erik Anderson <Erik.Anderson@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#977705}

net/dns/dns_config_service.cc

@@ -137,15 +137,17 @@ DnsConfigService::HostsReader::CreateWorkItem() {
       std::make_unique<DnsHostsFileParser>(hosts_file_path_));
 }
 
-void DnsConfigService::HostsReader::OnWorkFinished(
+bool DnsConfigService::HostsReader::OnWorkFinished(
     std::unique_ptr<SerialWorker::WorkItem> serial_worker_work_item) {
   DCHECK(serial_worker_work_item);
 
   WorkItem* work_item = static_cast<WorkItem*>(serial_worker_work_item.get());
   if (work_item->hosts_.has_value()) {
     service_->OnHostsRead(std::move(work_item->hosts_).value());
+    return true;
   } else {
     LOG(WARNING) << "Failed to read DnsHosts.";
+    return false;
   }
 }
 

net/dns/dns_config_service.h

@@ -156,7 +156,7 @@ class NET_EXPORT_PRIVATE DnsConfigService {
 
     // SerialWorker:
     std::unique_ptr<SerialWorker::WorkItem> CreateWorkItem() override;
-    void OnWorkFinished(
+    bool OnWorkFinished(
         std::unique_ptr<SerialWorker::WorkItem> work_item) final;
 
    private:

net/dns/dns_config_service_android.cc

@@ -102,16 +102,18 @@ class DnsConfigServiceAndroid::ConfigReader : public SerialWorker {
     return std::make_unique<WorkItem>(dns_server_getter_);
   }
 
-  void OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
+  bool OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
                           serial_worker_work_item) override {
     DCHECK(serial_worker_work_item);
     DCHECK(!IsCancelled());
 
     WorkItem* work_item = static_cast<WorkItem*>(serial_worker_work_item.get());
     if (work_item->dns_config_.has_value()) {
       service_->OnConfigRead(std::move(work_item->dns_config_).value());
+      return true;
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
+      return false;
     }
   }
 

net/dns/dns_config_service_linux.cc

@@ -414,7 +414,7 @@ class DnsConfigServiceLinux::ConfigReader : public SerialWorker {
     return std::move(work_item_);
   }
 
-  void OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
+  bool OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
                           serial_worker_work_item) override {
     DCHECK(serial_worker_work_item);
     DCHECK(!work_item_);
@@ -423,8 +423,10 @@ class DnsConfigServiceLinux::ConfigReader : public SerialWorker {
     work_item_.reset(static_cast<WorkItem*>(serial_worker_work_item.release()));
     if (work_item_->dns_config_.has_value()) {
       service_->OnConfigRead(std::move(work_item_->dns_config_).value());
+      return true;
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
+      return false;
     }
   }
 

net/dns/dns_config_service_posix.cc

@@ -182,16 +182,18 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker {
     return std::make_unique<WorkItem>();
   }
 
-  void OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
+  bool OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
                           serial_worker_work_item) override {
     DCHECK(serial_worker_work_item);
     DCHECK(!IsCancelled());
 
     WorkItem* work_item = static_cast<WorkItem*>(serial_worker_work_item.get());
     if (work_item->dns_config_.has_value()) {
       service_->OnConfigRead(std::move(work_item->dns_config_).value());
+      return true;
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
+      return false;
     }
   }
 

net/dns/dns_config_service_win.cc

@@ -28,8 +28,6 @@
 #include "base/synchronization/lock.h"
 #include "base/task/single_thread_task_runner.h"
 #include "base/threading/scoped_blocking_call.h"
-#include "base/threading/sequenced_task_runner_handle.h"
-#include "base/time/time.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
 #include "base/win/windows_types.h"
@@ -46,9 +44,6 @@ namespace internal {
 
 namespace {
 
-// Interval between retries to parse config. Used only until parsing succeeds.
-const int kRetryIntervalSeconds = 5;
-
 // Registry key paths.
 const wchar_t kTcpipPath[] =
     L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters";
@@ -491,15 +486,16 @@ class DnsConfigServiceWin::Watcher
 // Reads config from registry and IpHelper. All work performed in ThreadPool.
 class DnsConfigServiceWin::ConfigReader : public SerialWorker {
  public:
-  explicit ConfigReader(DnsConfigServiceWin& service) : service_(&service) {}
+  explicit ConfigReader(DnsConfigServiceWin& service)
+      : SerialWorker(/*max_number_of_retries=*/3), service_(&service) {}
   ~ConfigReader() override {}
 
   // SerialWorker::
   std::unique_ptr<SerialWorker::WorkItem> CreateWorkItem() override {
     return std::make_unique<WorkItem>();
   }
 
-  void OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
+  bool OnWorkFinished(std::unique_ptr<SerialWorker::WorkItem>
                           serial_worker_work_item) override {
     DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DCHECK(serial_worker_work_item);
@@ -508,12 +504,10 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
     WorkItem* work_item = static_cast<WorkItem*>(serial_worker_work_item.get());
     if (work_item->dns_config_.has_value()) {
       service_->OnConfigRead(std::move(work_item->dns_config_).value());
+      return true;
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
-      // Try again in a while in case DnsConfigWatcher missed the signal.
-      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
-          FROM_HERE, base::BindOnce(&ConfigReader::WorkNow, AsWeakPtr()),
-          base::Seconds(kRetryIntervalSeconds));
+      return false;
     }
   }
 

net/dns/serial_worker.cc

@@ -16,9 +16,25 @@
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/timer/timer.h"
+#include "net/base/backoff_entry.h"
 
 namespace net {
 
+namespace {
+// Default retry configuration. Only in effect if |max_number_of_retries| is
+// greater than 0.
+constexpr BackoffEntry::Policy kDefaultBackoffPolicy = {
+    0,     // Number of initial errors to ignore without backoff.
+    5000,  // Initial delay for backoff in ms: 5 seconds.
+    2,     // Factor to multiply for exponential backoff.
+    0,     // Fuzzing percentage.
+    -1,    // No maximum delay.
+    -1,    // Don't discard entry.
+    false  // Don't use initial delay unless the last was an error.
+};
+}  // namespace
+
 namespace {
 std::unique_ptr<SerialWorker::WorkItem> DoWork(
     std::unique_ptr<SerialWorker::WorkItem> work_item) {
@@ -32,15 +48,28 @@ void SerialWorker::WorkItem::FollowupWork(base::OnceClosure closure) {
   std::move(closure).Run();
 }
 
-SerialWorker::SerialWorker() : state_(State::kIdle) {}
+SerialWorker::SerialWorker(int max_number_of_retries,
+                           const net::BackoffEntry::Policy* backoff_policy)
+    : state_(State::kIdle),
+      max_number_of_retries_(max_number_of_retries),
+      backoff_entry_(backoff_policy ? backoff_policy : &kDefaultBackoffPolicy) {
+}
 
 SerialWorker::~SerialWorker() = default;
 
 void SerialWorker::WorkNow() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Not a retry; reset failure count and cancel the pending retry (if any).
+  backoff_entry_.Reset();
+  retry_timer_.Stop();
+  WorkNowInternal();
+}
+
+void SerialWorker::WorkNowInternal() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   switch (state_) {
     case State::kIdle:
-      // We are posting weak pointer to OnWorkJobFinished to avoid leak when
+      // We are posting weak pointer to OnWorkJobFinished to avoid a leak when
       // PostTaskAndReply fails to post task back to the original
       // task runner. In this case the callback is not destroyed, and the
       // weak reference allows SerialWorker instance to be deleted.
@@ -95,7 +124,16 @@ void SerialWorker::OnFollowupWorkFinished(std::unique_ptr<WorkItem> work_item) {
       return;
     case State::kWorking:
       state_ = State::kIdle;
-      OnWorkFinished(std::move(work_item));
+      if (OnWorkFinished(std::move(work_item)) ||
+          backoff_entry_.failure_count() >= max_number_of_retries_) {
+        backoff_entry_.Reset();
+      } else {
+        backoff_entry_.InformOfRequest(/*succeeded=*/false);
+
+        // Try again after a delay.
+        retry_timer_.Start(FROM_HERE, backoff_entry_.GetTimeUntilRelease(),
+                           this, &SerialWorker::WorkNowInternal);
+      }
       return;
     case State::kPending:
       RerunWork(std::move(work_item));
@@ -119,6 +157,16 @@ void SerialWorker::RerunWork(std::unique_ptr<WorkItem> work_item) {
       base::BindOnce(&SerialWorker::OnDoWorkFinished, AsWeakPtr()));
 }
 
+const BackoffEntry& SerialWorker::GetBackoffEntryForTesting() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return backoff_entry_;
+}
+
+const base::OneShotTimer& SerialWorker::GetRetryTimerForTesting() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return retry_timer_;
+}
+
 base::WeakPtr<SerialWorker> SerialWorker::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

net/dns/serial_worker.h

@@ -12,6 +12,8 @@
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/task/task_traits.h"
+#include "base/timer/timer.h"
+#include "net/base/backoff_entry.h"
 #include "net/base/net_export.h"
 
 namespace net {
@@ -26,6 +28,10 @@ namespace net {
 // made to `OnWorkFinished()` the same `WorkItem` will be passed back to the
 // `ThreadPool`, and `DoWork()` will be called once more.
 //
+// If |OnWorkFinished| returns a failure and |max_number_of_retries|
+// is non-zero, retries will be scheduled according to the |backoff_policy|.
+// A default backoff policy is used if one is not provided.
+//
 // This behavior is designed for updating a result after some trigger, for
 // example reading a file once FilePathWatcher indicates it changed.
 //
@@ -56,7 +62,9 @@ class NET_EXPORT_PRIVATE SerialWorker {
     virtual void FollowupWork(base::OnceClosure closure);
   };
 
-  SerialWorker();
+  explicit SerialWorker(
+      int max_number_of_retries = 0,
+      const net::BackoffEntry::Policy* backoff_policy = nullptr);
 
   SerialWorker(const SerialWorker&) = delete;
   SerialWorker& operator=(const SerialWorker&) = delete;
@@ -70,6 +78,10 @@ class NET_EXPORT_PRIVATE SerialWorker {
 
   bool IsCancelled() const { return state_ == State::kCancelled; }
 
+  // Allows tests to inspect the current backoff/retry state.
+  const BackoffEntry& GetBackoffEntryForTesting() const;
+  const base::OneShotTimer& GetRetryTimerForTesting() const;
+
  protected:
   // protected to allow sub-classing, but prevent deleting
   virtual ~SerialWorker();
@@ -78,7 +90,8 @@ class NET_EXPORT_PRIVATE SerialWorker {
   virtual std::unique_ptr<WorkItem> CreateWorkItem() = 0;
 
   // Executed on origin thread after `WorkItem` completes.
-  virtual void OnWorkFinished(std::unique_ptr<WorkItem> work_item) = 0;
+  // Must return true on success.
+  virtual bool OnWorkFinished(std::unique_ptr<WorkItem> work_item) = 0;
 
   base::WeakPtr<SerialWorker> AsWeakPtr();
 
@@ -94,6 +107,8 @@ class NET_EXPORT_PRIVATE SerialWorker {
     kPending,  // |WorkNow| while WORKING, must re-do work
   };
 
+  void WorkNowInternal();
+
   // Called on the origin thread after `WorkItem::DoWork()` completes.
   void OnDoWorkFinished(std::unique_ptr<WorkItem> work_item);
 
@@ -104,6 +119,11 @@ class NET_EXPORT_PRIVATE SerialWorker {
 
   State state_;
 
+  // Max retries and backoff entry to control timing.
+  const int max_number_of_retries_;
+  BackoffEntry backoff_entry_;
+  base::OneShotTimer retry_timer_;
+
   base::WeakPtrFactory<SerialWorker> weak_factory_{this};
 };