Factor out DnsConfigService::HostsReader logic

Similar logic is currently duplicated between DnsConfigServiceWin and
DnsConfigServicePosix, and there would be even more duplication if
DCSPosix is further split by platforms (as I intend to do in a
subsequent CL).

As HOSTS-reading logic is so similar between platforms, the base
implementation is now able to do everything for the Posix platforms with
just a simple constructor param to set the file location, and only
minimal overriding is needed for Windows.

Bug: 1157492
Change-Id: I1051580c891b8c86f46ac6d0b68f89c7b60244b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2625348
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843254}

net/dns/dns_config_service.cc

@@ -8,12 +8,18 @@
 
 #include "base/bind.h"
 #include "base/check_op.h"
+#include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
 #include "base/optional.h"
+#include "base/sequence_checker.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
+#include "net/dns/dns_hosts.h"
+#include "net/dns/serial_worker.h"
 
 namespace net {
 
@@ -22,18 +28,22 @@ const base::TimeDelta DnsConfigService::kInvalidationTimeout =
     base::TimeDelta::FromMilliseconds(150);
 
 DnsConfigService::DnsConfigService(
+    base::FilePath::StringPieceType hosts_file_path,
     base::Optional<base::TimeDelta> config_change_delay)
     : watch_failed_(false),
       have_config_(false),
       have_hosts_(false),
       need_update_(false),
       last_sent_empty_(true),
-      config_change_delay_(config_change_delay) {
+      config_change_delay_(config_change_delay),
+      hosts_file_path_(hosts_file_path) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 DnsConfigService::~DnsConfigService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (hosts_reader_)
+    hosts_reader_->Cancel();
 }
 
 void DnsConfigService::ReadConfig(const CallbackType& callback) {
@@ -81,6 +91,48 @@ void DnsConfigService::Watcher::CheckOnCorrectSequence() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+DnsConfigService::HostsReader::HostsReader(
+    DnsConfigService* service,
+    base::FilePath::StringPieceType hosts_file_path)
+    : service_(service), hosts_file_path_(hosts_file_path) {}
+
+DnsConfigService::HostsReader::~HostsReader() = default;
+
+bool DnsConfigService::HostsReader::ReadHosts(DnsHosts* out_dns_hosts) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::MAY_BLOCK);
+  DCHECK(!hosts_file_path_.empty());
+  return ParseHostsFile(hosts_file_path_, out_dns_hosts);
+}
+
+bool DnsConfigService::HostsReader::AddAdditionalHostsTo(DnsHosts& dns_hosts) {
+  // Nothing to add in base implementation.
+  return true;
+}
+
+void DnsConfigService::HostsReader::DoWork() {
+  success_ = ReadHosts(&hosts_) && AddAdditionalHostsTo(hosts_);
+}
+
+void DnsConfigService::HostsReader::OnWorkFinished() {
+  if (success_) {
+    service_->OnHostsRead(hosts_);
+  } else {
+    LOG(WARNING) << "Failed to read DnsHosts.";
+  }
+}
+
+void DnsConfigService::ReadHostsNow() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (!hosts_reader_) {
+    DCHECK(!hosts_file_path_.empty());
+    hosts_reader_ =
+        base::MakeRefCounted<HostsReader>(this, hosts_file_path_.value());
+  }
+  hosts_reader_->WorkNow();
+}
+
 void DnsConfigService::InvalidateConfig() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!have_config_)

net/dns/dns_config_service.h

@@ -8,7 +8,9 @@
 #include <map>
 #include <memory>
 
+#include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
@@ -17,6 +19,7 @@
 #include "net/base/net_export.h"
 #include "net/dns/dns_config.h"
 #include "net/dns/dns_hosts.h"
+#include "net/dns/serial_worker.h"
 #include "url/gurl.h"
 
 namespace net {
@@ -45,6 +48,7 @@ class NET_EXPORT_PRIVATE DnsConfigService {
   // Useful for platforms where multiple changes may be made and detected before
   // the config is stabilized and ready to be read.
   explicit DnsConfigService(
+      base::FilePath::StringPieceType hosts_file_path,
       base::Optional<base::TimeDelta> config_change_delay =
           base::TimeDelta::FromMilliseconds(50));
   virtual ~DnsConfigService();
@@ -102,9 +106,53 @@ class NET_EXPORT_PRIVATE DnsConfigService {
     SEQUENCE_CHECKER(sequence_checker_);
   };
 
+  // Reader of HOSTS files. In this base implementation, uses standard logic
+  // appropriate to most platforms to read the HOSTS file located at
+  // `service->GetHostsFilePath()`.
+  class HostsReader : public SerialWorker {
+   public:
+    HostsReader(DnsConfigService* service,
+                base::FilePath::StringPieceType hosts_file_path);
+
+    HostsReader(const HostsReader&) = delete;
+    HostsReader& operator=(const HostsReader&) = delete;
+
+   protected:
+    ~HostsReader() override;
+
+    // Reads the HOSTS file and parses to a `DnsHosts`. Returns false on
+    // failure. Will be called on a separate blockable ThreadPool thread.
+    //
+    // Override if needed to implement platform-specific behavior, e.g. for a
+    // platform-specific HOSTS format.
+    virtual bool ReadHosts(DnsHosts* out_dns_hosts);
+
+    // Adds any necessary additional entries to the given `DnsHosts`. Returns
+    // false on failure. Will be called on a separate blockable ThreadPool
+    // thread.
+    //
+    // Override if needed to implement platform-specific behavior.
+    virtual bool AddAdditionalHostsTo(DnsHosts& dns_hosts);
+
+    // SerialWorker:
+    void DoWork() final;
+    void OnWorkFinished() final;
+
+   private:
+    // Raw pointer to owning DnsConfigService. This must never be accessed
+    // inside DoWork(), since service may be destroyed while SerialWorker is
+    // running on worker thread.
+    DnsConfigService* const service_;
+    // Written in DoWork, read in OnWorkFinished, no locking necessary.
+    DnsHosts hosts_;
+    bool success_ = false;
+
+    const base::FilePath hosts_file_path_;
+  };
+
   // Immediately attempts to read the current configuration.
   virtual void ReadConfigNow() = 0;
-  virtual void ReadHostsNow() = 0;
+  virtual void ReadHostsNow();
   // Registers system watchers. Returns true iff succeeds.
   virtual bool StartWatching() = 0;
 
@@ -150,6 +198,11 @@ class NET_EXPORT_PRIVATE DnsConfigService {
   bool last_sent_empty_;
 
   const base::Optional<base::TimeDelta> config_change_delay_;
+  const base::FilePath hosts_file_path_;
+
+  // Created only if needed in ReadHostsNow() to avoid creating unnecessarily if
+  // overridden for a platform-specific implementation.
+  scoped_refptr<HostsReader> hosts_reader_;
 
   // Started in Invalidate*, cleared in On*Read.
   base::OneShotTimer timer_;

net/dns/dns_config_service_fuchsia.cc

@@ -6,13 +6,16 @@
 
 #include <memory>
 
+#include "base/files/file_path.h"
 #include "net/dns/dns_config.h"
 #include "net/dns/dns_hosts.h"
 
 namespace net {
 namespace internal {
 
-DnsConfigServiceFuchsia::DnsConfigServiceFuchsia() = default;
+DnsConfigServiceFuchsia::DnsConfigServiceFuchsia()
+    : DnsConfigService(
+          base::FilePath::StringPieceType() /* hosts_file_path */) {}
 DnsConfigServiceFuchsia::~DnsConfigServiceFuchsia() = default;
 
 void DnsConfigServiceFuchsia::ReadConfigNow() {

net/dns/dns_config_service_posix.cc

@@ -306,52 +306,14 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker {
   DISALLOW_COPY_AND_ASSIGN(ConfigReader);
 };
 
-// A SerialWorker that reads the HOSTS file and runs Callback.
-class DnsConfigServicePosix::HostsReader : public SerialWorker {
- public:
-  explicit HostsReader(DnsConfigServicePosix* service)
-      : service_(service), success_(false) {
-    // Allow execution on another thread; nothing thread-specific about
-    // constructor.
-    DETACH_FROM_SEQUENCE(sequence_checker_);
-  }
-
- private:
-  ~HostsReader() override {}
-
-  void DoWork() override {
-    base::ScopedBlockingCall scoped_blocking_call(
-        FROM_HERE, base::BlockingType::MAY_BLOCK);
-    success_ = ParseHostsFile(base::FilePath(kFilePathHosts), &hosts_);
-  }
-
-  void OnWorkFinished() override {
-    if (success_) {
-      service_->OnHostsRead(hosts_);
-    } else {
-      LOG(WARNING) << "Failed to read DnsHosts.";
-    }
-  }
-
-  // Raw pointer to owning DnsConfigService. This must never be accessed inside
-  // DoWork(), since service may be destroyed while SerialWorker is running
-  // on worker thread.
-  DnsConfigServicePosix* const service_;
-  // Written in DoWork, read in OnWorkFinished, no locking necessary.
-  DnsHosts hosts_;
-  bool success_;
-
-  DISALLOW_COPY_AND_ASSIGN(HostsReader);
-};
-
-DnsConfigServicePosix::DnsConfigServicePosix() {
+DnsConfigServicePosix::DnsConfigServicePosix()
+    : DnsConfigService(kFilePathHosts) {
   // Allow constructing on one thread and living on another.
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 DnsConfigServicePosix::~DnsConfigServicePosix() {
   config_reader_->Cancel();
-  hosts_reader_->Cancel();
 }
 
 void DnsConfigServicePosix::RefreshConfig() {
@@ -365,23 +327,17 @@ void DnsConfigServicePosix::ReadConfigNow() {
   config_reader_->WorkNow();
 }
 
-void DnsConfigServicePosix::ReadHostsNow() {
-  hosts_reader_->WorkNow();
-}
-
 bool DnsConfigServicePosix::StartWatching() {
-  CreateReaders();
+  CreateReader();
   // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139
   watcher_.reset(new Watcher(this));
   return watcher_->Watch();
 }
 
-void DnsConfigServicePosix::CreateReaders() {
+void DnsConfigServicePosix::CreateReader() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!config_reader_);
   config_reader_ = base::MakeRefCounted<ConfigReader>(this);
-  DCHECK(!hosts_reader_);
-  hosts_reader_ = base::MakeRefCounted<HostsReader>(this);
 }
 
 #if !defined(OS_ANDROID)

net/dns/dns_config_service_posix.h

@@ -14,7 +14,6 @@
 #endif
 
 #include "base/compiler_specific.h"
-#include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/optional.h"
@@ -42,22 +41,19 @@ class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService {
  protected:
   // DnsConfigService:
   void ReadConfigNow() override;
-  void ReadHostsNow() override;
   bool StartWatching() override;
 
-  // Create |config_reader_| and |hosts_reader_|.
-  void CreateReaders();
+  // Create |config_reader_|.
+  void CreateReader();
 
  private:
   FRIEND_TEST_ALL_PREFIXES(DnsConfigServicePosixTest,
                            ChangeConfigMultipleTimes);
   class Watcher;
   class ConfigReader;
-  class HostsReader;
 
   std::unique_ptr<Watcher> watcher_;
   scoped_refptr<ConfigReader> config_reader_;
-  scoped_refptr<HostsReader> hosts_reader_;
 
   DISALLOW_COPY_AND_ASSIGN(DnsConfigServicePosix);
 };

net/dns/dns_config_service_win.cc

@@ -4,6 +4,8 @@
 
 #include "net/dns/dns_config_service_win.h"
 
+#include <sysinfoapi.h>
+
 #include <algorithm>
 #include <memory>
 #include <string>
@@ -29,6 +31,7 @@
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/windows_types.h"
 #include "net/base/ip_address.h"
 #include "net/base/network_change_notifier.h"
 #include "net/dns/dns_hosts.h"
@@ -679,47 +682,30 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
   bool success_;
 };
 
-// Reads hosts from HOSTS file and fills in localhost and local computer name if
-// necessary. All work performed in ThreadPool.
-class DnsConfigServiceWin::HostsReader : public SerialWorker {
+// Extension of DnsConfigService::HostsReader that fills in localhost and local
+// computer name if necessary.
+class DnsConfigServiceWin::HostsReader : public DnsConfigService::HostsReader {
  public:
   explicit HostsReader(DnsConfigServiceWin* service)
-      : path_(GetHostsPath()),
-        service_(service),
-        success_(false) {
-  }
+      : DnsConfigService::HostsReader(service, GetHostsPath().value()) {}
 
- private:
-  ~HostsReader() override {}
+  HostsReader(const HostsReader&) = delete;
+  HostsReader& operator=(const HostsReader&) = delete;
 
-  void DoWork() override {
+ protected:
+  bool AddAdditionalHostsTo(DnsHosts& dns_hosts) override {
     base::ScopedBlockingCall scoped_blocking_call(
         FROM_HERE, base::BlockingType::MAY_BLOCK);
-    success_ = false;
-    if (ParseHostsFile(path_, &hosts_))
-      success_ = AddLocalhostEntries(&hosts_);
-  }
-
-  void OnWorkFinished() override {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-    if (success_) {
-      service_->OnHostsRead(hosts_);
-    } else {
-      LOG(WARNING) << "Failed to read DnsHosts.";
-    }
+    return AddLocalhostEntries(&dns_hosts);
   }
 
-  const base::FilePath path_;
-  DnsConfigServiceWin* service_;
-  // Written in DoWork, read in OnWorkFinished, no locking necessary.
-  DnsHosts hosts_;
-  bool success_;
-
-  DISALLOW_COPY_AND_ASSIGN(HostsReader);
+ private:
+  ~HostsReader() override = default;
 };
 
 DnsConfigServiceWin::DnsConfigServiceWin()
-    : DnsConfigService(base::nullopt /* config_change_delay */) {
+    : DnsConfigService(GetHostsPath().value(),
+                       base::nullopt /* config_change_delay */) {
   // Allow constructing on one sequence and living on another.
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }

net/dns/test_dns_config_service.cc

@@ -4,10 +4,15 @@
 
 #include "net/dns/test_dns_config_service.h"
 
+#include "base/check.h"
+#include "base/files/file_path.h"
+#include "base/optional.h"
+
 namespace net {
 
 TestDnsConfigService::TestDnsConfigService()
-    : DnsConfigService(base::nullopt /* config_change_delay */) {}
+    : DnsConfigService(base::FilePath::StringPieceType() /* hosts_file_path */,
+                       base::nullopt /* config_change_delay */) {}
 
 TestDnsConfigService::~TestDnsConfigService() = default;