diff options
24 files changed, 982 insertions, 646 deletions
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc index 2a51153..c1b7a3f 100644 --- a/net/base/host_resolver_impl.cc +++ b/net/base/host_resolver_impl.cc @@ -294,6 +294,47 @@ class JobAttachParameters : public NetLog::EventParameters { const RequestPriority priority_; }; +// Parameters of the DNS_CONFIG_CHANGED event. +class DnsConfigParameters : public NetLog::EventParameters { + public: + explicit DnsConfigParameters(const DnsConfig& config) + : num_hosts_(config.hosts.size()) { + config_.CopyIgnoreHosts(config); + } + + virtual Value* ToValue() const OVERRIDE { + DictionaryValue* dict = new DictionaryValue(); + + ListValue* list = new ListValue(); + for (size_t i = 0; i < config_.search.size(); ++i) { + list->Append(Value::CreateStringValue( + config_.nameservers[i].ToString())); + } + dict->Set("nameservers", list); + + list = new ListValue(); + for (size_t i = 0; i < config_.nameservers.size(); ++i) { + list->Append(Value::CreateStringValue(config_.search[i])); + } + dict->Set("search", list); + + dict->SetBoolean("append_to_multi_label_name", + config_.append_to_multi_label_name); + dict->SetInteger("ndots", config_.ndots); + dict->SetDouble("timeout", config_.timeout.InSecondsF()); + dict->SetInteger("attempts", config_.attempts); + dict->SetBoolean("rotate", config_.rotate); + dict->SetBoolean("edns0", config_.edns0); + dict->SetInteger("num_hosts", num_hosts_); + + return dict; + } + + private: + DnsConfig config_; // Does not include DnsHosts to save memory and work. + const size_t num_hosts_; +}; + // The logging routines are defined here because some requests are resolved // without a Request object. @@ -432,7 +473,6 @@ HostResolver* CreateAsyncHostResolver(size_t max_concurrent_resolves, NetLog* net_log) { scoped_ptr<DnsConfigService> config_service = DnsConfigService::CreateSystemService(); - config_service->Watch(); return CreateHostResolver(max_concurrent_resolves, max_retry_attempts, HostCache::CreateDefaultCache(), @@ -1107,18 +1147,6 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job { STLDeleteElements(&requests_); } - const Key key() const { - return key_; - } - - bool is_queued() const { - return !handle_.is_null(); - } - - bool is_running() const { - return is_dns_running() || is_proc_running(); - } - // Add this job to the dispatcher. void Schedule(RequestPriority priority) { handle_ = resolver_->dispatcher_.Add(this, priority); @@ -1218,24 +1246,19 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job { return false; } - private: - RequestPriority priority() const { - return priority_tracker_.highest_priority(); - } - - // Number of non-canceled requests in |requests_|. - size_t num_active_requests() const { - return priority_tracker_.total_count(); + const Key key() const { + return key_; } - bool is_dns_running() const { - return dns_task_.get() != NULL; + bool is_queued() const { + return !handle_.is_null(); } - bool is_proc_running() const { - return proc_task_.get() != NULL; + bool is_running() const { + return is_dns_running() || is_proc_running(); } + private: // PriorityDispatch::Job: virtual void Start() OVERRIDE { DCHECK(!is_running()); @@ -1396,6 +1419,23 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job { } } + RequestPriority priority() const { + return priority_tracker_.highest_priority(); + } + + // Number of non-canceled requests in |requests_|. + size_t num_active_requests() const { + return priority_tracker_.total_count(); + } + + bool is_dns_running() const { + return dns_task_.get() != NULL; + } + + bool is_proc_running() const { + return proc_task_.get() != NULL; + } + base::WeakPtr<HostResolverImpl> resolver_; Key key_; @@ -1474,7 +1514,9 @@ HostResolverImpl::HostResolverImpl( #endif if (dns_config_service_.get()) { - dns_config_service_->AddObserver(this); + dns_config_service_->Watch( + base::Bind(&HostResolverImpl::OnDnsConfigChanged, + base::Unretained(this))); dns_client_ = DnsClient::CreateClient(net_log_); } } @@ -1844,7 +1886,13 @@ void HostResolverImpl::OnDNSChanged(unsigned detail) { // |this| may be deleted inside AbortAllInProgressJobs(). } -void HostResolverImpl::OnConfigChanged(const DnsConfig& dns_config) { +void HostResolverImpl::OnDnsConfigChanged(const DnsConfig& dns_config) { + if (net_log_) { + net_log_->AddGlobalEntry( + NetLog::TYPE_DNS_CONFIG_CHANGED, + make_scoped_refptr(new DnsConfigParameters(dns_config))); + } + // We want a new factory in place, before we Abort running Jobs, so that the // newly started jobs use the new factory. DCHECK(dns_client_.get()); diff --git a/net/base/host_resolver_impl.h b/net/base/host_resolver_impl.h index 7b0900f..65b25c1 100644 --- a/net/base/host_resolver_impl.h +++ b/net/base/host_resolver_impl.h @@ -56,14 +56,11 @@ namespace net { // threads using PrioritizedDispatcher::Limits. // // Jobs are ordered in the queue based on their priority and order of arrival. -// -// TODO(szym): Change DnsConfigService::Observer to Callback. class NET_EXPORT HostResolverImpl : public HostResolver, NON_EXPORTED_BASE(public base::NonThreadSafe), public NetworkChangeNotifier::IPAddressObserver, public NetworkChangeNotifier::DNSObserver, - NON_EXPORTED_BASE(public DnsConfigService::Observer), public base::SupportsWeakPtr<HostResolverImpl> { public: // Parameters for ProcTask which resolves hostnames using HostResolveProc. @@ -147,12 +144,13 @@ class NET_EXPORT HostResolverImpl virtual void ProbeIPv6Support() OVERRIDE; virtual HostCache* GetHostCache() OVERRIDE; - // Allows the tests to catch slots leaking out of the dispatcher. - size_t num_running_jobs_for_tests() const { - return dispatcher_.num_running_jobs(); - } - private: + FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, + CanceledRequestsReleaseJobSlots); + FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, + ObeyPoolConstraintsAfterIPAddressChange); + FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, + AbortOnlyExistingRequestsOnIPAddressChange); FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, DnsTask); FRIEND_TEST_ALL_PREFIXES(HostResolverImplTest, ServeFromHosts); class Job; @@ -231,11 +229,16 @@ class NET_EXPORT HostResolverImpl // NetworkChangeNotifier::DNSObserver: virtual void OnDNSChanged(unsigned detail) OVERRIDE; - // DnsConfigService::Observer: - virtual void OnConfigChanged(const DnsConfig& dns_config) OVERRIDE; + // DnsConfigService callback: + void OnDnsConfigChanged(const DnsConfig& dns_config); // True if have fully configured DNS client. bool HaveDnsConfig() const; + // Allows the tests to catch slots leaking out of the dispatcher. + + size_t num_running_jobs_for_tests() const { + return dispatcher_.num_running_jobs(); + } // Cache of host resolution results. scoped_ptr<HostCache> cache_; diff --git a/net/base/net_log_event_type_list.h b/net/base/net_log_event_type_list.h index 68c2b4f..55b1dd0 100644 --- a/net/base/net_log_event_type_list.h +++ b/net/base/net_log_event_type_list.h @@ -1237,6 +1237,22 @@ EVENT_TYPE(APPCACHE_DELIVERING_ERROR_RESPONSE) // underlying network has changed. EVENT_TYPE(NETWORK_IP_ADDRESSES_CHANGED) + +// This event is emitted whenever HostResolverImpl receives a new DnsConfig +// from the DnsConfigService. +// { +// "nameservers": <List of name server IPs>, +// "search": <List of domain suffixes>, +// "append_to_multi_label_name": <See DnsConfig>, +// "ndots": <See DnsConfig>, +// "timeout": <See DnsConfig>, +// "attempts": <See DnsConfig>, +// "rotate": <See DnsConfig>, +// "edns0": <See DnsConfig>, +// "num_hosts": <Number of entries in the HOSTS file> +// } +EVENT_TYPE(DNS_CONFIG_CHANGED) + // ------------------------------------------------------------------------ // Exponential back-off throttling events // ------------------------------------------------------------------------ diff --git a/net/dns/dns_config_service.cc b/net/dns/dns_config_service.cc index d64b6735..be48d2a 100644 --- a/net/dns/dns_config_service.cc +++ b/net/dns/dns_config_service.cc @@ -4,7 +4,7 @@ #include "net/dns/dns_config_service.h" -#include "base/file_util.h" +#include "base/logging.h" #include "net/base/ip_endpoint.h" namespace net { @@ -20,6 +20,10 @@ DnsConfig::DnsConfig() DnsConfig::~DnsConfig() {} +bool DnsConfig::Equals(const DnsConfig& d) const { + return EqualsIgnoreHosts(d) && (hosts == d.hosts); +} + bool DnsConfig::EqualsIgnoreHosts(const DnsConfig& d) const { return (nameservers == d.nameservers) && (search == d.search) && @@ -31,86 +35,93 @@ bool DnsConfig::EqualsIgnoreHosts(const DnsConfig& d) const { (edns0 == d.edns0); } -bool DnsConfig::Equals(const DnsConfig& d) const { - return EqualsIgnoreHosts(d) && (hosts == d.hosts); +void DnsConfig::CopyIgnoreHosts(const DnsConfig& d) { + nameservers = d.nameservers; + search = d.search; + append_to_multi_label_name = d.append_to_multi_label_name; + ndots = d.ndots; + timeout = d.timeout; + attempts = d.attempts; + rotate = d.rotate; + edns0 = d.edns0; } + DnsConfigService::DnsConfigService() : have_config_(false), - have_hosts_(false) {} + have_hosts_(false), + need_update_(false) {} DnsConfigService::~DnsConfigService() {} -void DnsConfigService::AddObserver(Observer* observer) { +void DnsConfigService::InvalidateConfig() { DCHECK(CalledOnValidThread()); - observers_.AddObserver(observer); - if (have_config_ && have_hosts_) { - observer->OnConfigChanged(dns_config_); - } + if (!have_config_) + return; + have_config_ = false; + StartTimer(); } -void DnsConfigService::RemoveObserver(Observer* observer) { +void DnsConfigService::InvalidateHosts() { DCHECK(CalledOnValidThread()); - observers_.RemoveObserver(observer); + if (!have_hosts_) + return; + have_hosts_ = false; + StartTimer(); } void DnsConfigService::OnConfigRead(const DnsConfig& config) { DCHECK(CalledOnValidThread()); + DCHECK(config.IsValid()); + if (!config.EqualsIgnoreHosts(dns_config_)) { - DnsHosts current_hosts = dns_config_.hosts; - dns_config_ = config; - dns_config_.hosts.swap(current_hosts); - have_config_ = true; - if (have_hosts_) { - FOR_EACH_OBSERVER(Observer, observers_, OnConfigChanged(dns_config_)); - } + dns_config_.CopyIgnoreHosts(config); + need_update_ = true; } + + have_config_ = true; + if (have_hosts_) + OnCompleteConfig(); } void DnsConfigService::OnHostsRead(const DnsHosts& hosts) { DCHECK(CalledOnValidThread()); - if (hosts != dns_config_.hosts || !have_hosts_) { + + if (hosts != dns_config_.hosts) { dns_config_.hosts = hosts; - have_hosts_ = true; - if (have_config_) { - FOR_EACH_OBSERVER(Observer, observers_, OnConfigChanged(dns_config_)); - } + need_update_ = true; } -} -DnsHostsReader::DnsHostsReader(const FilePath& path, DnsConfigService* service) - : path_(path), - service_(service), - success_(false) { - DCHECK(service); + have_hosts_ = true; + if (have_config_) + OnCompleteConfig(); } -// Reads the contents of the file at |path| into |str| if the total length is -// less than |size|. -static bool ReadFile(const FilePath& path, int64 size, std::string* str) { - int64 sz; - if (!file_util::GetFileSize(path, &sz) || sz > size) - return false; - return file_util::ReadFileToString(path, str); +void DnsConfigService::StartTimer() { + DCHECK(CalledOnValidThread()); + timer_.Stop(); + const base::TimeDelta kTimeout = base::TimeDelta::FromMilliseconds(100); + timer_.Start(FROM_HERE, + kTimeout, + this, + &DnsConfigService::OnTimeout); } -void DnsHostsReader::DoWork() { - success_ = false; - std::string contents; - const int64 kMaxHostsSize = 1 << 16; - if (ReadFile(path_, kMaxHostsSize, &contents)) { - success_ = true; - ParseHosts(contents, &dns_hosts_); - } +void DnsConfigService::OnTimeout() { + DCHECK(CalledOnValidThread()); + // Indicate that even if there is no change in On*Read, we will need to + // update the receiver when the config becomes complete. + need_update_ = true; + callback_.Run(DnsConfig()); } -void DnsHostsReader::OnWorkFinished() { - DCHECK(!IsCancelled()); - if (success_) - service_->OnHostsRead(dns_hosts_); +void DnsConfigService::OnCompleteConfig() { + timer_.Stop(); + if (need_update_) { + need_update_ = false; + callback_.Run(dns_config_); + } } -DnsHostsReader::~DnsHostsReader() {} - } // namespace net diff --git a/net/dns/dns_config_service.h b/net/dns/dns_config_service.h index 51e26b3..cdbdb40 100644 --- a/net/dns/dns_config_service.h +++ b/net/dns/dns_config_service.h @@ -10,16 +10,14 @@ #include <string> #include <vector> -#include "base/file_path.h" #include "base/gtest_prod_util.h" #include "base/memory/scoped_ptr.h" -#include "base/observer_list.h" #include "base/threading/non_thread_safe.h" #include "base/time.h" +#include "base/timer.h" #include "net/base/ip_endpoint.h" // win requires size of IPEndPoint #include "net/base/net_export.h" #include "net/dns/dns_hosts.h" -#include "net/dns/serial_worker.h" namespace net { @@ -32,6 +30,8 @@ struct NET_EXPORT_PRIVATE DnsConfig { bool EqualsIgnoreHosts(const DnsConfig& d) const; + void CopyIgnoreHosts(const DnsConfig& src); + bool IsValid() const { return !nameservers.empty(); } @@ -64,20 +64,13 @@ struct NET_EXPORT_PRIVATE DnsConfig { // Service for watching when the system DNS settings have changed. -// Depending on the platform, watches files in /etc/ or win registry. -// This class also serves as the do-nothing mock implementation. +// Depending on the platform, watches files in /etc/ or Windows registry. class NET_EXPORT_PRIVATE DnsConfigService : NON_EXPORTED_BASE(public base::NonThreadSafe) { public: // Callback interface for the client. The observer is called on the same // thread as Watch(). Observer must outlive the service. - class Observer { - public: - virtual ~Observer() {} - - // Called only when |dns_config| is different from the last call. - virtual void OnConfigChanged(const DnsConfig& dns_config) = 0; - }; + typedef base::Callback<void(const DnsConfig& config)> CallbackType; // Creates the platform-specific DnsConfigService. static scoped_ptr<DnsConfigService> CreateSystemService(); @@ -88,54 +81,50 @@ class NET_EXPORT_PRIVATE DnsConfigService // Immediately starts watching system configuration for changes and attempts // to read the configuration. For some platform implementations, the current // thread must have an IO loop (for base::files::FilePathWatcher). - virtual void Watch() {} - - // If a config is available, |observer| will immediately be called with - // OnConfigChanged. - void AddObserver(Observer* observer); - void RemoveObserver(Observer* observer); + virtual void Watch(const CallbackType& callback) = 0; protected: - FRIEND_TEST_ALL_PREFIXES(DnsConfigServiceTest, NotifyOnChange); friend class DnsHostsReader; - // To be called with new config. |config|.hosts is ignored. - virtual void OnConfigRead(const DnsConfig& config); - // To be called with new hosts. Rest of the config is assumed unchanged. - virtual void OnHostsRead(const DnsHosts& hosts); - DnsConfig dns_config_; - // True after first OnConfigRead and OnHostsRead; indicate complete config. - bool have_config_; - bool have_hosts_; + // Called when the current config (except hosts) has changed. + void InvalidateConfig(); + // Called when the current hosts have changed. + void InvalidateHosts(); + + // Called with new config. |config|.hosts is ignored. + void OnConfigRead(const DnsConfig& config); + // Called with new hosts. Rest of the config is assumed unchanged. + void OnHostsRead(const DnsHosts& hosts); + + void set_callback(const CallbackType& callback) { + callback_ = callback; + } - ObserverList<Observer> observers_; private: - DISALLOW_COPY_AND_ASSIGN(DnsConfigService); -}; + void StartTimer(); + // Called when the timer expires. + void OnTimeout(); + // Called when the config becomes complete. + void OnCompleteConfig(); -// A WatchingFileReader that reads a HOSTS file and notifies -// DnsConfigService::OnHostsRead(). -// Client should call Cancel() when |service| is going away. -class NET_EXPORT_PRIVATE DnsHostsReader - : NON_EXPORTED_BASE(public SerialWorker) { - public: - DnsHostsReader(const FilePath& path, DnsConfigService* service); + CallbackType callback_; - virtual void DoWork() OVERRIDE; - virtual void OnWorkFinished() OVERRIDE; + DnsConfig dns_config_; - private: - virtual ~DnsHostsReader(); + // True after On*Read, before Invalidate*. Tell if the config is complete. + bool have_config_; + bool have_hosts_; + // True if receiver needs to be updated when the config becomes complete. + bool need_update_; - FilePath path_; - DnsConfigService* service_; - // Written in DoRead, read in OnReadFinished, no locking necessary. - DnsHosts dns_hosts_; - bool success_; + // Started in Invalidate*, cleared in On*Read. + base::OneShotTimer<DnsConfigService> timer_; - DISALLOW_COPY_AND_ASSIGN(DnsHostsReader); + private: + DISALLOW_COPY_AND_ASSIGN(DnsConfigService); }; + } // namespace net #endif // NET_DNS_DNS_CONFIG_SERVICE_H_ diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc index 681405f..b3977fc 100644 --- a/net/dns/dns_config_service_posix.cc +++ b/net/dns/dns_config_service_posix.cc @@ -6,14 +6,15 @@ #include <string> -#include "base/compiler_specific.h" +#include "base/basictypes.h" +#include "base/bind.h" #include "base/file_path.h" #include "base/file_util.h" #include "base/memory/scoped_ptr.h" #include "net/base/ip_endpoint.h" #include "net/base/net_util.h" +#include "net/dns/file_path_watcher_wrapper.h" #include "net/dns/serial_worker.h" -#include "net/dns/watching_file_reader.h" #ifndef _PATH_RESCONF // Normally defined in <resolv.h> #define _PATH_RESCONF "/etc/resolv.conf" @@ -21,13 +22,19 @@ namespace net { -// A SerialWorker that uses ResolverLib to initialize res_state and converts +namespace { + +const FilePath::CharType* kFilePathConfig = FILE_PATH_LITERAL(_PATH_RESCONF); +const FilePath::CharType* kFilePathHosts = FILE_PATH_LITERAL("/etc/hosts"); + +// A SerialWorker that uses libresolv to initialize res_state and converts // it to DnsConfig. -class DnsConfigServicePosix::ConfigReader : public SerialWorker { +class ConfigReader : public SerialWorker { public: - explicit ConfigReader(DnsConfigServicePosix* service) - : service_(service), - success_(false) {} + typedef base::Callback<void(const DnsConfig& config)> CallbackType; + explicit ConfigReader(const CallbackType& callback) + : callback_(callback), + success_(false) {} void DoWork() OVERRIDE { success_ = false; @@ -38,13 +45,13 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { // res_init behaves the same way. memset(&_res, 0, sizeof(_res)); if ((res_init() == 0) && (_res.options & RES_INIT)) { - success_ = ConvertResStateToDnsConfig(_res, &dns_config_); + success_ = internal::ConvertResStateToDnsConfig(_res, &dns_config_); } #else // all other OS_POSIX struct __res_state res; memset(&res, 0, sizeof(res)); if ((res_ninit(&res) == 0) && (res.options & RES_INIT)) { - success_ = ConvertResStateToDnsConfig(res, &dns_config_); + success_ = internal::ConvertResStateToDnsConfig(res, &dns_config_); } // Prefer res_ndestroy where available. #if defined(OS_MACOSX) @@ -59,35 +66,80 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { void OnWorkFinished() OVERRIDE { DCHECK(!IsCancelled()); if (success_) - service_->OnConfigRead(dns_config_); + callback_.Run(dns_config_); } private: virtual ~ConfigReader() {} - DnsConfigServicePosix* service_; + CallbackType callback_; // Written in DoWork, read in OnWorkFinished, no locking necessary. DnsConfig dns_config_; bool success_; }; +} // namespace + +namespace internal { + DnsConfigServicePosix::DnsConfigServicePosix() - : config_watcher_(new WatchingFileReader()), - hosts_watcher_(new WatchingFileReader()) {} + : config_watcher_(new FilePathWatcherWrapper()), + hosts_watcher_(new FilePathWatcherWrapper()) { + config_reader_ = new ConfigReader( + base::Bind(&DnsConfigServicePosix::OnConfigRead, + base::Unretained(this))); + hosts_reader_ = new DnsHostsReader( + FilePath(kFilePathHosts), + base::Bind(&DnsConfigServicePosix::OnHostsRead, + base::Unretained(this))); +} -DnsConfigServicePosix::~DnsConfigServicePosix() {} +DnsConfigServicePosix::~DnsConfigServicePosix() { + config_reader_->Cancel(); + hosts_reader_->Cancel(); +} -void DnsConfigServicePosix::Watch() { +void DnsConfigServicePosix::Watch(const CallbackType& callback) { DCHECK(CalledOnValidThread()); - config_watcher_->StartWatch(FilePath(FILE_PATH_LITERAL(_PATH_RESCONF)), - new ConfigReader(this)); - FilePath hosts_path(FILE_PATH_LITERAL("/etc/hosts")); - hosts_watcher_->StartWatch(hosts_path, new DnsHostsReader(hosts_path, this)); + DCHECK(!callback.is_null()); + set_callback(callback); + + // Even if watchers fail, we keep the other one as it provides useful signals. + if (config_watcher_->Watch( + FilePath(kFilePathConfig), + base::Bind(&DnsConfigServicePosix::OnConfigChanged, + base::Unretained(this)))) { + OnConfigChanged(true); + } else { + OnConfigChanged(false); + } + + if (hosts_watcher_->Watch( + FilePath(kFilePathHosts), + base::Bind(&DnsConfigServicePosix::OnHostsChanged, + base::Unretained(this)))) { + OnHostsChanged(true); + } else { + OnHostsChanged(false); + } } -// static -scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() { - return scoped_ptr<DnsConfigService>(new DnsConfigServicePosix()); +void DnsConfigServicePosix::OnConfigChanged(bool watch_succeeded) { + InvalidateConfig(); + // We don't trust a config that we cannot watch in the future. + // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139 + if (watch_succeeded) + config_reader_->WorkNow(); + else + LOG(ERROR) << "Failed to watch DNS config"; +} + +void DnsConfigServicePosix::OnHostsChanged(bool watch_succeeded) { + InvalidateHosts(); + if (watch_succeeded) + hosts_reader_->WorkNow(); + else + LOG(ERROR) << "Failed to watch DNS hosts"; } #if !defined(OS_ANDROID) @@ -146,4 +198,11 @@ bool ConvertResStateToDnsConfig(const struct __res_state& res, } #endif // !defined(OS_ANDROID) +} // namespace internal + +// static +scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() { + return scoped_ptr<DnsConfigService>(new internal::DnsConfigServicePosix()); +} + } // namespace net diff --git a/net/dns/dns_config_service_posix.h b/net/dns/dns_config_service_posix.h index fa9599e..e266d06 100644 --- a/net/dns/dns_config_service_posix.h +++ b/net/dns/dns_config_service_posix.h @@ -17,28 +17,38 @@ namespace net { -class WatchingFileReader; +class FilePathWatcherWrapper; + +// Use DnsConfigService::CreateSystemService to use it outside of tests. +namespace internal { class NET_EXPORT_PRIVATE DnsConfigServicePosix - : NON_EXPORTED_BASE(public DnsConfigService) { + : NON_EXPORTED_BASE(public DnsConfigService) { public: DnsConfigServicePosix(); virtual ~DnsConfigServicePosix(); - virtual void Watch() OVERRIDE; + virtual void Watch(const CallbackType& callback) OVERRIDE; private: - class ConfigReader; - scoped_ptr<WatchingFileReader> config_watcher_; - scoped_ptr<WatchingFileReader> hosts_watcher_; + void OnConfigChanged(bool watch_succeeded); + void OnHostsChanged(bool watch_succeeded); + + scoped_ptr<FilePathWatcherWrapper> config_watcher_; + scoped_ptr<FilePathWatcherWrapper> hosts_watcher_; + + scoped_refptr<SerialWorker> config_reader_; + scoped_refptr<SerialWorker> hosts_reader_; DISALLOW_COPY_AND_ASSIGN(DnsConfigServicePosix); }; -// Fills in |dns_config| from |res|. Exposed for tests. +// Fills in |dns_config| from |res|. bool NET_EXPORT_PRIVATE ConvertResStateToDnsConfig( const struct __res_state& res, DnsConfig* dns_config); +} // namespace internal + } // namespace net #endif // NET_DNS_DNS_CONFIG_SERVICE_POSIX_H_ diff --git a/net/dns/dns_config_service_posix_unittest.cc b/net/dns/dns_config_service_posix_unittest.cc index f1ff015..e4ae0d8 100644 --- a/net/dns/dns_config_service_posix_unittest.cc +++ b/net/dns/dns_config_service_posix_unittest.cc @@ -10,7 +10,6 @@ #include "testing/gtest/include/gtest/gtest.h" namespace net { - namespace { void CompareConfig(const struct __res_state &res, const DnsConfig& config) { @@ -108,15 +107,13 @@ void CloseResState(res_state res) { #endif } -} // namespace - TEST(DnsConfigServicePosixTest, ConvertResStateToDnsConfig) { struct __res_state res[2]; DnsConfig config[2]; for (unsigned i = 0; i < 2; ++i) { EXPECT_FALSE(config[i].IsValid()); InitializeResState(&res[i], i); - ASSERT_TRUE(ConvertResStateToDnsConfig(res[i], &config[i])); + ASSERT_TRUE(internal::ConvertResStateToDnsConfig(res[i], &config[i])); } for (unsigned i = 0; i < 2; ++i) { EXPECT_TRUE(config[i].IsValid()); @@ -128,6 +125,7 @@ TEST(DnsConfigServicePosixTest, ConvertResStateToDnsConfig) { EXPECT_FALSE(config[1].Equals(config[0])); } +} // namespace } // namespace net diff --git a/net/dns/dns_config_service_unittest.cc b/net/dns/dns_config_service_unittest.cc index ee76a9a..9d27cb7 100644 --- a/net/dns/dns_config_service_unittest.cc +++ b/net/dns/dns_config_service_unittest.cc @@ -4,8 +4,9 @@ #include "net/dns/dns_config_service.h" +#include "base/basictypes.h" #include "base/bind.h" -#include "base/compiler_specific.h" +#include "base/cancelable_callback.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop.h" #include "base/test/test_timeouts.h" @@ -15,92 +16,189 @@ namespace net { namespace { -class DnsConfigServiceTest : public testing::Test, - public DnsConfigService::Observer { +class DnsConfigServiceTest : public testing::Test { public: - void OnConfigChanged(const DnsConfig& config) OVERRIDE { + void OnConfigChanged(const DnsConfig& config) { EXPECT_FALSE(config.Equals(last_config_)) << "Config must be different from last call."; last_config_ = config; if (quit_on_config_) MessageLoop::current()->Quit(); } - void Timeout() const { - MessageLoop::current()->Quit(); - } protected: - // test::Test methods + class TestDnsConfigService : public DnsConfigService { + public: + virtual void Watch(const CallbackType& callback) OVERRIDE { + set_callback(callback); + } + + // Expose the protected methods to this test suite. + void InvalidateConfig() { + DnsConfigService::InvalidateConfig(); + } + + void InvalidateHosts() { + DnsConfigService::InvalidateHosts(); + } + + void OnConfigRead(const DnsConfig& config) { + DnsConfigService::OnConfigRead(config); + } + + void OnHostsRead(const DnsHosts& hosts) { + DnsConfigService::OnHostsRead(hosts); + } + }; + + void WaitForConfig(base::TimeDelta timeout) { + base::CancelableClosure closure(MessageLoop::QuitClosure()); + MessageLoop::current()->PostDelayedTask(FROM_HERE, + closure.callback(), + timeout); + quit_on_config_ = true; + MessageLoop::current()->Run(); + quit_on_config_ = false; + closure.Cancel(); + } + + // Generate a config using the given seed.. + DnsConfig MakeConfig(unsigned seed) { + DnsConfig config; + IPAddressNumber ip; + EXPECT_TRUE(ParseIPLiteralToNumber("1.2.3.4", &ip)); + config.nameservers.push_back(IPEndPoint(ip, seed & 0xFFFF)); + EXPECT_TRUE(config.IsValid()); + return config; + } + + // Generate hosts using the given seed. + DnsHosts MakeHosts(unsigned seed) { + DnsHosts hosts; + std::string hosts_content = "127.0.0.1 localhost"; + hosts_content.append(seed, '1'); + ParseHosts(hosts_content, &hosts); + EXPECT_FALSE(hosts.empty()); + return hosts; + } + virtual void SetUp() OVERRIDE { - service_.reset(new DnsConfigService()); quit_on_config_ = false; + + service_.reset(new TestDnsConfigService()); + service_->Watch(base::Bind(&DnsConfigServiceTest::OnConfigChanged, + base::Unretained(this))); + EXPECT_FALSE(last_config_.IsValid()); } DnsConfig last_config_; bool quit_on_config_; // Service under test. - scoped_ptr<DnsConfigService> service_; + scoped_ptr<TestDnsConfigService> service_; }; } // namespace -TEST_F(DnsConfigServiceTest, NotifyOnChange) { - service_->AddObserver(this); - service_->Watch(); +TEST_F(DnsConfigServiceTest, FirstConfig) { + DnsConfig config = MakeConfig(1); + + service_->OnConfigRead(config); + // No hosts yet, so no config. + EXPECT_FALSE(last_config_.IsValid()); + + service_->OnHostsRead(config.hosts); + // Empty hosts is acceptable. + EXPECT_TRUE(last_config_.IsValid()); + EXPECT_TRUE(last_config_.Equals(config)); +} + +TEST_F(DnsConfigServiceTest, Timeout) { + DnsConfig config = MakeConfig(1); + config.hosts = MakeHosts(1); - DnsConfig config; - IPAddressNumber ip; - ASSERT_TRUE(ParseIPLiteralToNumber("1.2.3.4", &ip)); - config.nameservers.push_back(IPEndPoint(ip, 53)); - ASSERT_TRUE(config.IsValid()); service_->OnConfigRead(config); + service_->OnHostsRead(config.hosts); + EXPECT_TRUE(last_config_.IsValid()); + EXPECT_TRUE(last_config_.Equals(config)); - EXPECT_FALSE(last_config_.Equals(config)) << - "Unexpected notification with incomplete config."; + service_->InvalidateConfig(); + WaitForConfig(TestTimeouts::action_timeout()); + EXPECT_FALSE(last_config_.IsValid()); - DnsHosts hosts; - ParseHosts("127.0.0.1 localhost", &hosts); - ASSERT_FALSE(hosts.empty()); - service_->OnHostsRead(hosts); + service_->OnConfigRead(config); + EXPECT_TRUE(last_config_.Equals(config)); + + service_->InvalidateHosts(); + WaitForConfig(TestTimeouts::action_timeout()); + EXPECT_FALSE(last_config_.IsValid()); - EXPECT_TRUE(last_config_.hosts == hosts); - EXPECT_TRUE(last_config_.EqualsIgnoreHosts(config)); + service_->OnHostsRead(config.hosts); + EXPECT_TRUE(last_config_.Equals(config)); +} - DnsConfig complete_config = config; - complete_config.hosts = hosts; - EXPECT_TRUE(last_config_.Equals(complete_config)); +TEST_F(DnsConfigServiceTest, SameConfig) { + DnsConfig config = MakeConfig(1); + config.hosts = MakeHosts(1); - // Check if it ignores no-change in config or hosts. service_->OnConfigRead(config); - service_->OnHostsRead(hosts); + service_->OnHostsRead(config.hosts); + EXPECT_TRUE(last_config_.Equals(config)); - // Check if it provides config and hosts after AddObserver; - service_->RemoveObserver(this); - last_config_ = DnsConfig(); - service_->AddObserver(this); - EXPECT_TRUE(last_config_.Equals(complete_config)); + // OnConfigChanged will catch if there was no change. + service_->OnConfigRead(config); + EXPECT_TRUE(last_config_.Equals(config)); + + service_->OnHostsRead(config.hosts); + EXPECT_TRUE(last_config_.Equals(config)); +} + +TEST_F(DnsConfigServiceTest, DifferentConfig) { + DnsConfig config1 = MakeConfig(1); + DnsConfig config2 = MakeConfig(2); + DnsConfig config3 = MakeConfig(1); + config1.hosts = MakeHosts(1); + config2.hosts = MakeHosts(1); + config3.hosts = MakeHosts(2); + ASSERT_TRUE(config1.EqualsIgnoreHosts(config3)); + ASSERT_FALSE(config1.Equals(config2)); + ASSERT_FALSE(config1.Equals(config3)); + ASSERT_FALSE(config2.Equals(config3)); + + service_->OnConfigRead(config1); + service_->OnHostsRead(config1.hosts); + EXPECT_TRUE(last_config_.Equals(config1)); + + // It doesn't matter for this tests, but increases coverage. + service_->InvalidateConfig(); + service_->InvalidateHosts(); + + service_->OnConfigRead(config2); + service_->OnHostsRead(config2.hosts); + EXPECT_TRUE(last_config_.Equals(config2)); + + service_->OnConfigRead(config3); + service_->OnHostsRead(config3.hosts); + EXPECT_TRUE(last_config_.Equals(config3)); } #if defined(OS_POSIX) || defined(OS_WIN) -// This is really an integration test. -TEST_F(DnsConfigServiceTest, GetSystemConfig) { +// TODO(szym): This is really an integration test and can time out if HOSTS is +// huge. http://crbug.com/107810 +TEST_F(DnsConfigServiceTest, FLAKY_GetSystemConfig) { + service_.reset(); scoped_ptr<DnsConfigService> service(DnsConfigService::CreateSystemService()); - // Quit the loop after timeout unless cancelled - const base::TimeDelta kTimeout = TestTimeouts::action_timeout(); - base::WeakPtrFactory<DnsConfigServiceTest> factory_(this); - MessageLoop::current()->PostDelayedTask( - FROM_HERE, - base::Bind(&DnsConfigServiceTest::Timeout, factory_.GetWeakPtr()), - kTimeout); - - service->AddObserver(this); - service->Watch(); - quit_on_config_ = true; - MessageLoop::current()->Run(); + service->Watch(base::Bind(&DnsConfigServiceTest::OnConfigChanged, + base::Unretained(this))); + base::TimeDelta kTimeout = TestTimeouts::action_max_timeout(); + WaitForConfig(kTimeout); ASSERT_TRUE(last_config_.IsValid()) << "Did not receive DnsConfig in " << - kTimeout.InMilliseconds() << "ms"; + kTimeout.InSecondsF() << "s"; + + // Restart watch to confirm it's allowed. + service->Watch(base::Bind(&DnsConfigServiceTest::OnConfigChanged, + base::Unretained(this))); } #endif // OS_POSIX || OS_WIN diff --git a/net/dns/dns_config_service_win.cc b/net/dns/dns_config_service_win.cc index 14bb773..047b92d 100644 --- a/net/dns/dns_config_service_win.cc +++ b/net/dns/dns_config_service_win.cc @@ -11,9 +11,11 @@ #include "base/callback.h" #include "base/compiler_specific.h" #include "base/file_path.h" +#include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "base/string_split.h" #include "base/string_util.h" +#include "base/synchronization/lock.h" #include "base/threading/thread_restrictions.h" #include "base/utf_string_conversions.h" #include "base/win/object_watcher.h" @@ -22,26 +24,27 @@ #include "googleurl/src/url_canon.h" #include "net/base/net_util.h" #include "net/dns/dns_protocol.h" +#include "net/dns/file_path_watcher_wrapper.h" #include "net/dns/serial_worker.h" -#include "net/dns/watching_file_reader.h" #pragma comment(lib, "iphlpapi.lib") namespace net { +namespace internal { + namespace { -// Watches a single registry key for changes. This class is thread-safe with -// the exception of StartWatch which must be called once and only once. +// Watches a single registry key for changes. Thread-safe. class RegistryWatcher : public base::win::ObjectWatcher::Delegate { public: + typedef base::Callback<void(bool succeeded)> CallbackType; RegistryWatcher() {} - bool StartWatch(const wchar_t* key, const base::Closure& callback) { + bool Watch(const wchar_t* key, const CallbackType& callback) { + base::AutoLock lock(lock_); DCHECK(!callback.is_null()); - DCHECK(callback_.is_null()); - // TODO(szym): This will need to change to address http://crbug.com/99509 - DCHECK(!key_.Valid()); + CancelLocked(); if (key_.Open(HKEY_LOCAL_MACHINE, key, KEY_NOTIFY | KEY_QUERY_VALUE) != ERROR_SUCCESS) return false; @@ -54,22 +57,21 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate { } void Cancel() { - callback_.Reset(); - key_.StopWatching(); - watcher_.StopWatching(); + base::AutoLock lock(lock_); + CancelLocked(); } virtual void OnObjectSignaled(HANDLE object) OVERRIDE { + base::AutoLock lock(lock_); + bool succeeded = (key_.StartWatching() == ERROR_SUCCESS) && + watcher_.StartWatching(key_.watch_event(), this); if (!callback_.is_null()) - callback_.Run(); - // TODO(szym): handle errors - bool success = key_.StartWatching() && - watcher_.StartWatching(key_.watch_event(), this); - DCHECK(success); + callback_.Run(succeeded); } bool ReadString(const wchar_t* name, DnsSystemSettings::RegString* out) const { + base::AutoLock lock(lock_); out->set = false; if (!key_.Valid()) { // Assume that if the |key_| is invalid then the key is missing. @@ -85,6 +87,7 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate { bool ReadDword(const wchar_t* name, DnsSystemSettings::RegDword* out) const { + base::AutoLock lock(lock_); out->set = false; if (!key_.Valid()) { // Assume that if the |key_| is invalid then the key is missing. @@ -99,7 +102,17 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate { } private: - base::Closure callback_; + void CancelLocked() { + lock_.AssertAcquired(); + callback_.Reset(); + if (key_.Valid()) { + watcher_.StopWatching(); + key_.StopWatching(); + } + } + + mutable base::Lock lock_; + CallbackType callback_; base::win::RegKey key_; base::win::ObjectWatcher watcher_; @@ -123,9 +136,8 @@ bool ParseDomainASCII(const string16& widestr, std::string* domain) { // Otherwise try to convert it from IDN to punycode. const int kInitialBufferSize = 256; url_canon::RawCanonOutputT<char16, kInitialBufferSize> punycode; - if (!url_canon::IDNToASCII(widestr.data(), widestr.length(), &punycode)) { + if (!url_canon::IDNToASCII(widestr.data(), widestr.length(), &punycode)) return false; - } // |punycode_output| should now be ASCII; convert it to a std::string. // (We could use UTF16ToASCII() instead, but that requires an extra string @@ -169,9 +181,10 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings, // Use GetAdapterAddresses to get effective DNS server order and // connection-specific DNS suffix. Ignore disconnected and loopback adapters. - // TODO(szym): Also ignore VM adapters. http://crbug.com/112856 + // The order of adapters is the network binding order, so stick to the + // first good adapter. for (const IP_ADAPTER_ADDRESSES* adapter = settings.addresses.get(); - adapter != NULL; + adapter != NULL && config->nameservers.empty(); adapter = adapter->Next) { if (adapter->OperStatus != IfOperStatusUp) continue; @@ -200,9 +213,8 @@ bool ConvertSettingsToDnsConfig(const DnsSystemSettings& settings, // obtained via DHCP (regkey: Tcpip\Parameters\Interfaces\{XXX}\DhcpDomain) // or specified by the user (regkey: Tcpip\Parameters\Domain). std::string dns_suffix; - if (ParseDomainASCII(adapter->DnsSuffix, &dns_suffix)) { + if (ParseDomainASCII(adapter->DnsSuffix, &dns_suffix)) config->search.push_back(dns_suffix); - } } if (config->nameservers.empty()) @@ -304,31 +316,36 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker { : service_(service), success_(false) {} - bool StartWatch() { + bool Watch() { DCHECK(loop()->BelongsToCurrentThread()); - base::Closure callback = base::Bind(&SerialWorker::WorkNow, - base::Unretained(this)); + RegistryWatcher::CallbackType callback = + base::Bind(&ConfigReader::OnChange, base::Unretained(this)); + + // The Tcpip key must be present. + if (!tcpip_watcher_.Watch( + L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters", + callback)) + return false; + + // Watch for IPv6 nameservers. + tcpip6_watcher_.Watch( + L"SYSTEM\\CurrentControlSet\\Services\\Tcpip6\\Parameters", + callback); // DNS suffix search list and devolution can be configured via group // policy which sets this registry key. If the key is missing, the policy // does not apply, and the DNS client uses Tcpip and Dnscache settings. // If a policy is installed, DnsConfigService will need to be restarted. // BUG=99509 - policy_watcher_.StartWatch( - L"SOFTWARE\\Policies\\Microsoft\\Windows NT\\DNSClient", - callback); - // The Dnscache key might also be missing. - dnscache_watcher_.StartWatch( + dnscache_watcher_.Watch( L"SYSTEM\\CurrentControlSet\\Services\\Dnscache\\Parameters", callback); - // The Tcpip key must be present. - if (!tcpip_watcher_.StartWatch( - L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters", - callback)) - return false; + policy_watcher_.Watch( + L"SOFTWARE\\Policies\\Microsoft\\Windows NT\\DNSClient", + callback); WorkNow(); return true; @@ -339,18 +356,27 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker { SerialWorker::Cancel(); policy_watcher_.Cancel(); dnscache_watcher_.Cancel(); + tcpip6_watcher_.Cancel(); tcpip_watcher_.Cancel(); } - // Delay between calls to WorkerPool::PostTask - static const int kWorkerPoolRetryDelayMs = 100; - private: - friend class RegistryWatcher; virtual ~ConfigReader() { DCHECK(IsCancelled()); } + void OnChange(bool succeeded) { + DCHECK(loop()->BelongsToCurrentThread()); + if (!IsCancelled()) + service_->InvalidateConfig(); + // We don't trust a config that we cannot watch in the future. + // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139 + if (succeeded) + WorkNow(); + else + LOG(ERROR) << "Failed to watch DNS config"; + } + bool ReadIpHelper(DnsSystemSettings* settings) { base::ThreadRestrictions::AssertIOAllowed(); @@ -429,43 +455,68 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker { DnsConfig dns_config_; bool success_; - RegistryWatcher policy_watcher_; - RegistryWatcher dnscache_watcher_; RegistryWatcher tcpip_watcher_; + RegistryWatcher tcpip6_watcher_; + RegistryWatcher dnscache_watcher_; + RegistryWatcher policy_watcher_; }; DnsConfigServiceWin::DnsConfigServiceWin() : config_reader_(new ConfigReader(this)), - hosts_watcher_(new WatchingFileReader()) {} + hosts_watcher_(new FilePathWatcherWrapper()) {} DnsConfigServiceWin::~DnsConfigServiceWin() { DCHECK(CalledOnValidThread()); config_reader_->Cancel(); + if (hosts_reader_) + hosts_reader_->Cancel(); } -void DnsConfigServiceWin::Watch() { +void DnsConfigServiceWin::Watch(const CallbackType& callback) { DCHECK(CalledOnValidThread()); + DCHECK(!callback.is_null()); + set_callback(callback); // This is done only once per lifetime so open the keys and file watcher // handles on this thread. // TODO(szym): Should/can this be avoided? http://crbug.com/114223 base::ThreadRestrictions::ScopedAllowIO allow_io; - bool started = config_reader_->StartWatch(); - // TODO(szym): handle possible failure - DCHECK(started); + if (!config_reader_->Watch()) { + LOG(ERROR) << "Failed to start watching DNS config"; + InvalidateConfig(); + } TCHAR buffer[MAX_PATH]; UINT rc = GetSystemDirectory(buffer, MAX_PATH); DCHECK(0 < rc && rc < MAX_PATH); FilePath hosts_path = FilePath(buffer). Append(FILE_PATH_LITERAL("drivers\\etc\\hosts")); - hosts_watcher_->StartWatch(hosts_path, new DnsHostsReader(hosts_path, this)); + hosts_reader_ = new DnsHostsReader( + hosts_path, + base::Bind(&DnsConfigServiceWin::OnHostsRead, base::Unretained(this))); + if (hosts_watcher_->Watch(hosts_path, + base::Bind(&DnsConfigServiceWin::OnHostsChanged, + base::Unretained(this)))) { + OnHostsChanged(true); + } else { + OnHostsChanged(false); + } +} + +void DnsConfigServiceWin::OnHostsChanged(bool succeeded) { + InvalidateHosts(); + if (succeeded) + hosts_reader_->WorkNow(); + else + LOG(ERROR) << "Failed to watch DNS hosts"; } +} // namespace internal + // static scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() { - return scoped_ptr<DnsConfigService>(new DnsConfigServiceWin()); + return scoped_ptr<DnsConfigService>(new internal::DnsConfigServiceWin()); } } // namespace net diff --git a/net/dns/dns_config_service_win.h b/net/dns/dns_config_service_win.h index d60ae04..3159f77 100644 --- a/net/dns/dns_config_service_win.h +++ b/net/dns/dns_config_service_win.h @@ -33,20 +33,27 @@ namespace net { -class WatchingFileReader; +class FilePathWatcherWrapper; + +// Use DnsConfigService::CreateSystemService to use it outside of tests. +namespace internal { class NET_EXPORT_PRIVATE DnsConfigServiceWin - : NON_EXPORTED_BASE(public DnsConfigService) { + : NON_EXPORTED_BASE(public DnsConfigService) { public: DnsConfigServiceWin(); virtual ~DnsConfigServiceWin(); - virtual void Watch() OVERRIDE; + virtual void Watch(const CallbackType& callback) OVERRIDE; private: class ConfigReader; + + void OnHostsChanged(bool succeeded); + scoped_refptr<ConfigReader> config_reader_; - scoped_ptr<WatchingFileReader> hosts_watcher_; + scoped_ptr<FilePathWatcherWrapper> hosts_watcher_; + scoped_refptr<SerialWorker> hosts_reader_; DISALLOW_COPY_AND_ASSIGN(DnsConfigServiceWin); }; @@ -73,7 +80,8 @@ struct NET_EXPORT_PRIVATE DnsSystemSettings { RegDword level; }; - // Filled in by GetAdapterAddresses. + // Filled in by GetAdapterAddresses. Note that the alternative + // GetNetworkParams does not include IPv6 addresses. scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> addresses; // SOFTWARE\Policies\Microsoft\Windows NT\DNSClient\SearchList @@ -105,6 +113,8 @@ bool NET_EXPORT_PRIVATE ParseSearchList(const string16& value, bool NET_EXPORT_PRIVATE ConvertSettingsToDnsConfig( const DnsSystemSettings& settings, DnsConfig* dns_config); +} // namespace internal + } // namespace net #endif // NET_DNS_DNS_CONFIG_SERVICE_WIN_H_ diff --git a/net/dns/dns_config_service_win_unittest.cc b/net/dns/dns_config_service_win_unittest.cc index 269bb55..fa5da54 100644 --- a/net/dns/dns_config_service_win_unittest.cc +++ b/net/dns/dns_config_service_win_unittest.cc @@ -4,6 +4,7 @@ #include "net/dns/dns_config_service_win.h" +#include "base/logging.h" #include "base/win/windows_version.h" #include "net/dns/dns_protocol.h" #include "testing/gtest/include/gtest/gtest.h" @@ -38,7 +39,7 @@ TEST(DnsConfigServiceWinTest, ParseSearchList) { for (const char* const* output = t.output; *output; ++output) { expected_output.push_back(*output); } - bool result = ParseSearchList(t.input, &actual_output); + bool result = internal::ParseSearchList(t.input, &actual_output); if (!expected_output.empty()) { EXPECT_TRUE(result); EXPECT_EQ(expected_output, actual_output); @@ -143,6 +144,19 @@ TEST(DnsConfigServiceWinTest, ConvertAdapterAddresses) { "chromium.org", { 1024, 24 }, }, + { // Use the preferred adapter (first in binding order). + { + { IF_TYPE_SOFTWARE_LOOPBACK, IfOperStatusUp, L"funnyloop", + { "2.0.0.2" } }, + { IF_TYPE_FASTETHER, IfOperStatusUp, L"example.com", + { "1.0.0.1" } }, + { IF_TYPE_USB, IfOperStatusUp, L"chromium.org", + { "10.0.0.10", "2001:FFFF::1111" } }, + { 0 }, + }, + { "1.0.0.1" }, + "example.com", + }, { // No usable nameservers. { { IF_TYPE_SOFTWARE_LOOPBACK, IfOperStatusUp, L"localhost", @@ -157,7 +171,7 @@ TEST(DnsConfigServiceWinTest, ConvertAdapterAddresses) { for (size_t i = 0; i < arraysize(cases); ++i) { const TestCase& t = cases[i]; - DnsSystemSettings settings = { + internal::DnsSystemSettings settings = { CreateAdapterAddresses(t.input_adapters), // Default settings for the rest. }; @@ -172,7 +186,7 @@ TEST(DnsConfigServiceWinTest, ConvertAdapterAddresses) { } DnsConfig config; - bool result = ConvertSettingsToDnsConfig(settings, &config); + bool result = internal::ConvertSettingsToDnsConfig(settings, &config); bool expected_result = !expected_nameservers.empty(); ASSERT_EQ(expected_result, result); EXPECT_EQ(expected_nameservers, config.nameservers); @@ -190,7 +204,7 @@ TEST(DnsConfigServiceWinTest, ConvertSuffixSearch) { }; const struct TestCase { - DnsSystemSettings input_settings; + internal::DnsSystemSettings input_settings; std::string expected_search[5]; } cases[] = { { // Policy SearchList override. @@ -320,7 +334,8 @@ TEST(DnsConfigServiceWinTest, ConvertSuffixSearch) { for (size_t i = 0; i < arraysize(cases); ++i) { const TestCase& t = cases[i]; DnsConfig config; - ASSERT_TRUE(ConvertSettingsToDnsConfig(t.input_settings, &config)); + ASSERT_TRUE(internal::ConvertSettingsToDnsConfig(t.input_settings, + &config)); std::vector<std::string> expected_search; for (size_t j = 0; !t.expected_search[j].empty(); ++j) { expected_search.push_back(t.expected_search[j]); @@ -339,7 +354,7 @@ TEST(DnsConfigServiceWinTest, AppendToMultiLabelName) { bool default_value = (base::win::GetVersion() < base::win::VERSION_VISTA); const struct TestCase { - DnsSystemSettings::RegDword input; + internal::DnsSystemSettings::RegDword input; bool expected_output; } cases[] = { { { true, 0 }, false }, @@ -349,7 +364,7 @@ TEST(DnsConfigServiceWinTest, AppendToMultiLabelName) { for (size_t i = 0; i < arraysize(cases); ++i) { const TestCase& t = cases[i]; - DnsSystemSettings settings = { + internal::DnsSystemSettings settings = { CreateAdapterAddresses(infos), { false }, { false }, { false }, { { false }, { false } }, @@ -358,7 +373,7 @@ TEST(DnsConfigServiceWinTest, AppendToMultiLabelName) { t.input, }; DnsConfig config; - ASSERT_TRUE(ConvertSettingsToDnsConfig(settings, &config)); + ASSERT_TRUE(internal::ConvertSettingsToDnsConfig(settings, &config)); EXPECT_EQ(config.append_to_multi_label_name, t.expected_output); } } diff --git a/net/dns/dns_hosts.cc b/net/dns/dns_hosts.cc index 1342808..bd46912 100644 --- a/net/dns/dns_hosts.cc +++ b/net/dns/dns_hosts.cc @@ -1,9 +1,10 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 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 "net/dns/dns_hosts.h" +#include "base/file_util.h" #include "base/logging.h" #include "base/string_tokenizer.h" @@ -43,5 +44,48 @@ void ParseHosts(const std::string& contents, DnsHosts* dns_hosts) { } } +DnsHostsReader::DnsHostsReader(const FilePath& path, + const CallbackType& callback) + : path_(path), + callback_(callback), + success_(false) { + DCHECK(!callback.is_null()); +} + +// Reads the contents of the file at |path| into |str| if the total length is +// less than |max_size|. +static bool ReadFile(const FilePath& path, int64 max_size, std::string* str) { + int64 size; + if (!file_util::GetFileSize(path, &size) || size > max_size) + return false; + return file_util::ReadFileToString(path, str); +} + +void DnsHostsReader::DoWork() { + success_ = false; + dns_hosts_.clear(); + + // Missing file indicates empty HOSTS. + if (!file_util::PathExists(path_)) { + success_ = true; + return; + } + + std::string contents; + const int64 kMaxHostsSize = 1 << 16; + if (ReadFile(path_, kMaxHostsSize, &contents)) { + success_ = true; + ParseHosts(contents, &dns_hosts_); + } +} + +void DnsHostsReader::OnWorkFinished() { + DCHECK(!IsCancelled()); + if (success_) + callback_.Run(dns_hosts_); +} + +DnsHostsReader::~DnsHostsReader() {} + } // namespace net diff --git a/net/dns/dns_hosts.h b/net/dns/dns_hosts.h index fda7733..91c35ba 100644 --- a/net/dns/dns_hosts.h +++ b/net/dns/dns_hosts.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 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. @@ -11,9 +11,12 @@ #include <utility> #include <vector> +#include "base/callback.h" +#include "base/file_path.h" #include "net/base/address_family.h" #include "net/base/net_export.h" #include "net/base/net_util.h" // can't forward-declare IPAddressNumber +#include "net/dns/serial_worker.h" namespace net { @@ -35,6 +38,32 @@ typedef std::map<DnsHostsKey, IPAddressNumber> DnsHosts; void NET_EXPORT_PRIVATE ParseHosts(const std::string& contents, DnsHosts* dns_hosts); +// A SerialWorker that reads a HOSTS file and runs Callback. +// Call WorkNow() to indicate file needs to be re-read. +// Call Cancel() to disable the callback. +class NET_EXPORT_PRIVATE DnsHostsReader + : NON_EXPORTED_BASE(public SerialWorker) { + public: + typedef base::Callback<void(const DnsHosts& hosts)> CallbackType; + + DnsHostsReader(const FilePath& path, const CallbackType& callback); + + private: + virtual ~DnsHostsReader(); + + virtual void DoWork() OVERRIDE; + virtual void OnWorkFinished() OVERRIDE; + + FilePath path_; + CallbackType callback_; + // Written in DoWork, read in OnWorkFinished, no locking necessary. + DnsHosts dns_hosts_; + bool success_; + + DISALLOW_COPY_AND_ASSIGN(DnsHostsReader); +}; + + } // namespace net #endif // NET_DNS_DNS_HOSTS_H_ diff --git a/net/dns/dns_test_util.cc b/net/dns/dns_test_util.cc index fa1bed1..a3c26cf 100644 --- a/net/dns/dns_test_util.cc +++ b/net/dns/dns_test_util.cc @@ -162,5 +162,9 @@ scoped_ptr<DnsClient> CreateMockDnsClient(const DnsConfig& config) { return scoped_ptr<DnsClient>(new MockDnsClient(config)); } +void MockDnsConfigService::Watch(const CallbackType& callback) { + set_callback(callback); +} + } // namespace net diff --git a/net/dns/dns_test_util.h b/net/dns/dns_test_util.h index e247be0..019dba8 100644 --- a/net/dns/dns_test_util.h +++ b/net/dns/dns_test_util.h @@ -166,6 +166,8 @@ class MockDnsConfigService : public DnsConfigService { public: virtual ~MockDnsConfigService() {} + virtual void Watch(const CallbackType& callback) OVERRIDE; + // Expose the protected methods for tests. void ChangeConfig(const DnsConfig& config) { DnsConfigService::OnConfigRead(config); diff --git a/net/dns/file_path_watcher_wrapper.cc b/net/dns/file_path_watcher_wrapper.cc new file mode 100644 index 0000000..909c03c --- /dev/null +++ b/net/dns/file_path_watcher_wrapper.cc @@ -0,0 +1,99 @@ +// Copyright (c) 2012 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 "net/dns/file_path_watcher_wrapper.h" + +#include "base/callback.h" +#include "base/files/file_path_watcher.h" +#include "base/logging.h" + +namespace net { + +// A FilePathWatcher::Delegate that forwards calls to FilePathWatcherCallback. +class FilePathWatcherWrapper::WatcherDelegate + : public base::files::FilePathWatcher::Delegate { + public: + explicit WatcherDelegate(FilePathWatcherWrapper* watcher) + : watcher_(watcher) {} + + void Cancel() { + watcher_ = NULL; + } + + void OnFilePathChanged(const FilePath& path) OVERRIDE { + if (watcher_) + watcher_->OnFilePathChanged(); + } + + void OnFilePathError(const FilePath& path) OVERRIDE { + if (watcher_) + watcher_->OnFilePathError(); + } + + private: + virtual ~WatcherDelegate() {} + FilePathWatcherWrapper* watcher_; +}; + +FilePathWatcherWrapper::FilePathWatcherWrapper() { + // No need to restrict the thread until the state is created in Watch. + DetachFromThread(); +} + +FilePathWatcherWrapper::~FilePathWatcherWrapper() { + Cancel(); +} + +void FilePathWatcherWrapper::Cancel() { + DCHECK(CalledOnValidThread()); + if (delegate_) { + delegate_->Cancel(); + delegate_.release(); + } + watcher_.reset(NULL); + // All state is gone so it's okay to detach from thread. + DetachFromThread(); +} + +bool FilePathWatcherWrapper::Watch( + const FilePath& path, + const base::Callback<void(bool succeeded)>& callback) { + DCHECK(CalledOnValidThread()); + if (delegate_) + delegate_->Cancel(); + callback_ = callback; + watcher_ = this->CreateFilePathWatcher(); + delegate_ = new WatcherDelegate(this); + if (watcher_->Watch(path, delegate_.get())) + return true; + + Cancel(); + return false; +} + +bool FilePathWatcherWrapper::IsWatching() const { + DCHECK(CalledOnValidThread()); + return watcher_.get() != NULL; +} + +scoped_ptr<base::files::FilePathWatcher> + FilePathWatcherWrapper::CreateFilePathWatcher() { + return scoped_ptr<base::files::FilePathWatcher>( + new base::files::FilePathWatcher()); +} + +void FilePathWatcherWrapper::OnFilePathChanged() { + DCHECK(CalledOnValidThread()); + callback_.Run(true); +} + +void FilePathWatcherWrapper::OnFilePathError() { + DCHECK(CalledOnValidThread()); + base::Callback<void(bool succeeded)> callback = callback_; + Cancel(); + callback.Run(false); +} + +} // namespace net + diff --git a/net/dns/file_path_watcher_wrapper.h b/net/dns/file_path_watcher_wrapper.h new file mode 100644 index 0000000..172bbaf65 --- /dev/null +++ b/net/dns/file_path_watcher_wrapper.h @@ -0,0 +1,68 @@ +// Copyright (c) 2012 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. + +#ifndef NET_FILE_PATH_WATCHER_CALLBACK_H_ +#define NET_FILE_PATH_WATCHER_WRAPPER_H_ +#pragma once + +#include <string> + +#include "base/basictypes.h" +#include "base/callback.h" +#include "base/file_path.h" +#include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" +#include "base/threading/non_thread_safe.h" +#include "net/base/net_export.h" + +namespace base { +namespace files { +class FilePathWatcher; +} +} + +namespace net { + +// Convenience wrapper which holds a FilePathWatcher and a +// FilePathWatcher::Delegate which forwards its calls to a Callback. +class NET_EXPORT FilePathWatcherWrapper + : NON_EXPORTED_BASE(public base::NonThreadSafe) { + public: + FilePathWatcherWrapper(); + // When deleted, automatically cancels the FilePathWatcher. + virtual ~FilePathWatcherWrapper(); + + // Starts watching the file at |path|. Returns true if Watch succeeds. + // If so, the delegate will call callback.Run(true) from OnFilePathChanged and + // callback.Run(false) from OnFilePathError. After failure the watch is + // cancelled and will have to be restarted. If called again, the previous + // watch is cancelled. + bool Watch(const FilePath& path, + const base::Callback<void(bool succeeded)>& callback); + + bool IsWatching() const; + + // Cancels the current watch. + void Cancel(); + + protected: + // Creates a FilePathWatcher. Override to provide a different implementation. + virtual scoped_ptr<base::files::FilePathWatcher> CreateFilePathWatcher(); + + private: + class WatcherDelegate; + + void OnFilePathChanged(); + void OnFilePathError(); + + base::Callback<void(bool succeeded)> callback_; + scoped_ptr<base::files::FilePathWatcher> watcher_; + scoped_refptr<WatcherDelegate> delegate_; + DISALLOW_COPY_AND_ASSIGN(FilePathWatcherWrapper); +}; + +} // namespace net + +#endif // NET_FILE_PATH_WATCHER_CALLBACK_H_ + diff --git a/net/dns/file_path_watcher_wrapper_unittest.cc b/net/dns/file_path_watcher_wrapper_unittest.cc new file mode 100644 index 0000000..e925e27 --- /dev/null +++ b/net/dns/file_path_watcher_wrapper_unittest.cc @@ -0,0 +1,142 @@ +// Copyright (c) 2012 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 "net/dns/file_path_watcher_wrapper.h" + +#include "base/bind.h" +#include "base/files/file_path_watcher.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace net { + +namespace { + +class FilePathWatcherWrapperTest : public testing::Test { + protected: + class MockFilePathWatcher : public base::files::FilePathWatcher { + public: + virtual bool Watch(const FilePath& path, Delegate* delegate) OVERRIDE { + DCHECK(!delegate_); + DCHECK(delegate); + path_ = path; + delegate_ = delegate; + return true; + } + + void OnFilePathChanged() { + delegate_->OnFilePathChanged(path_); + } + + void OnFilePathError() { + delegate_->OnFilePathError(path_); + } + + private: + FilePath path_; + scoped_refptr<Delegate> delegate_; + }; + + class FailingFilePathWatcher : public MockFilePathWatcher { + public: + virtual bool Watch(const FilePath& path, Delegate* delegate) OVERRIDE { + MockFilePathWatcher::Watch(path, delegate); + return false; + } + }; + + // Class under test. + class TestFilePathWatcherWrapper : public FilePathWatcherWrapper { + public: + TestFilePathWatcherWrapper() : fail_on_watch_(false) {} + + MockFilePathWatcher* file_path_watcher() { + return test_watcher_; + } + + void set_fail_on_watch(bool value) { + fail_on_watch_ = value; + } + + private: + virtual scoped_ptr<base::files::FilePathWatcher> + CreateFilePathWatcher() OVERRIDE { + test_watcher_ = fail_on_watch_ ? new FailingFilePathWatcher() + : new MockFilePathWatcher(); + return scoped_ptr<base::files::FilePathWatcher>(test_watcher_); + } + + MockFilePathWatcher* test_watcher_; + bool fail_on_watch_; + }; + + virtual void SetUp() OVERRIDE { + watcher_wrapper_.reset(new TestFilePathWatcherWrapper()); + got_change_ = false; + } + + bool Watch() { + return watcher_wrapper_->Watch( + FilePath(FILE_PATH_LITERAL("some_file_name")), + base::Bind(&FilePathWatcherWrapperTest::OnChange, + base::Unretained(this))); + } + + void OnChange(bool succeeded) { + got_change_ = true; + last_result_ = succeeded; + } + + scoped_ptr<TestFilePathWatcherWrapper> watcher_wrapper_; + bool got_change_; + bool last_result_; +}; + +TEST_F(FilePathWatcherWrapperTest, Basic) { + watcher_wrapper_->set_fail_on_watch(true); + EXPECT_FALSE(Watch()); + EXPECT_FALSE(watcher_wrapper_->IsWatching()); + EXPECT_FALSE(got_change_); + + watcher_wrapper_->set_fail_on_watch(false); + EXPECT_TRUE(Watch()); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); + EXPECT_FALSE(got_change_); + + EXPECT_TRUE(Watch()); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); + EXPECT_FALSE(got_change_); + + watcher_wrapper_->file_path_watcher()->OnFilePathChanged(); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); + EXPECT_TRUE(got_change_); + EXPECT_TRUE(last_result_); + + got_change_ = false; + watcher_wrapper_->file_path_watcher()->OnFilePathError(); + EXPECT_FALSE(watcher_wrapper_->IsWatching()); + EXPECT_TRUE(got_change_); + EXPECT_FALSE(last_result_); + + got_change_ = false; + EXPECT_TRUE(Watch()); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); + EXPECT_FALSE(got_change_); +} + +TEST_F(FilePathWatcherWrapperTest, Cancel) { + EXPECT_TRUE(Watch()); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); + + watcher_wrapper_->Cancel(); + EXPECT_FALSE(watcher_wrapper_->IsWatching()); + EXPECT_FALSE(got_change_); + + EXPECT_TRUE(Watch()); + EXPECT_TRUE(watcher_wrapper_->IsWatching()); +} + +} // namespace + +} // namespace net + diff --git a/net/dns/serial_worker.h b/net/dns/serial_worker.h index 4d6f8fc..41ca768 100644 --- a/net/dns/serial_worker.h +++ b/net/dns/serial_worker.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 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. @@ -46,7 +46,7 @@ class NET_EXPORT_PRIVATE SerialWorker // Made virtual to allow mocking. virtual void WorkNow(); - // Stop scheduling read jobs. + // Stop scheduling jobs. void Cancel(); bool IsCancelled() const { return state_ == CANCELLED; } diff --git a/net/dns/watching_file_reader.cc b/net/dns/watching_file_reader.cc deleted file mode 100644 index cf4f2e9..0000000 --- a/net/dns/watching_file_reader.cc +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright (c) 2012 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 "net/dns/watching_file_reader.h" - -#include "base/bind.h" -#include "base/location.h" -#include "base/message_loop.h" -#include "base/threading/worker_pool.h" - -namespace net { - -FilePathWatcherShim::FilePathWatcherShim() - : watcher_(new base::files::FilePathWatcher()) {} -FilePathWatcherShim::~FilePathWatcherShim() {} - -bool FilePathWatcherShim::Watch( - const FilePath& path, - base::files::FilePathWatcher::Delegate* delegate) { - DCHECK(CalledOnValidThread()); - return watcher_->Watch(path, delegate); -} - -FilePathWatcherShim* -FilePathWatcherFactory::CreateFilePathWatcher() { - DCHECK(CalledOnValidThread()); - return new FilePathWatcherShim(); -} - -// A FilePathWatcher::Delegate that forwards calls to the WatchingFileReader. -// This is separated out to keep WatchingFileReader strictly single-threaded. -class WatchingFileReader::WatcherDelegate - : public base::files::FilePathWatcher::Delegate { - public: - explicit WatcherDelegate(base::WeakPtr<WatchingFileReader> reader) - : reader_(reader) {} - void OnFilePathChanged(const FilePath& path) OVERRIDE { - if (reader_) reader_->OnFilePathChanged(path); - } - void OnFilePathError(const FilePath& path) OVERRIDE { - if (reader_) reader_->OnFilePathError(path); - } - private: - virtual ~WatcherDelegate() {} - base::WeakPtr<WatchingFileReader> reader_; -}; - -WatchingFileReader::WatchingFileReader() - : watcher_factory_(new FilePathWatcherFactory()) {} - -WatchingFileReader::~WatchingFileReader() { - if (reader_) - reader_->Cancel(); -} - -void WatchingFileReader::StartWatch(const FilePath& path, - SerialWorker* reader) { - DCHECK(CalledOnValidThread()); - DCHECK(!watcher_.get()); - DCHECK(!watcher_delegate_.get()); - DCHECK(path_.empty()); - path_ = path; - watcher_delegate_ = new WatcherDelegate(AsWeakPtr()); - reader_ = reader; - RestartWatch(); -} - -void WatchingFileReader::OnFilePathChanged(const FilePath& path) { - DCHECK(CalledOnValidThread()); - reader_->WorkNow(); -} - -void WatchingFileReader::OnFilePathError(const FilePath& path) { - DCHECK(CalledOnValidThread()); - RestartWatch(); -} - -void WatchingFileReader::RescheduleWatch() { - DCHECK(CalledOnValidThread()); - MessageLoop::current()->PostDelayedTask( - FROM_HERE, - base::Bind(&WatchingFileReader::RestartWatch, AsWeakPtr()), - base::TimeDelta::FromMilliseconds(kWatchRetryDelayMs)); -} - -void WatchingFileReader::RestartWatch() { - DCHECK(CalledOnValidThread()); - if (reader_->IsCancelled()) - return; - watcher_.reset(watcher_factory_->CreateFilePathWatcher()); - if (watcher_->Watch(path_, watcher_delegate_)) { - reader_->WorkNow(); - } else { - LOG(WARNING) << "Watch on " << - path_.LossyDisplayName() << - " failed, scheduling restart"; - RescheduleWatch(); - } -} - -} // namespace net diff --git a/net/dns/watching_file_reader.h b/net/dns/watching_file_reader.h deleted file mode 100644 index 6290dd3..0000000 --- a/net/dns/watching_file_reader.h +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright (c) 2011 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. - -#ifndef NET_DNS_WATCHING_FILE_READER_H_ -#define NET_DNS_WATCHING_FILE_READER_H_ -#pragma once - -#include <string> - -#include "base/compiler_specific.h" -#include "base/files/file_path_watcher.h" -#include "base/memory/scoped_ptr.h" -#include "base/memory/weak_ptr.h" -#include "base/threading/non_thread_safe.h" -#include "net/base/net_export.h" -#include "net/dns/serial_worker.h" - -namespace net { - -// Allows mocking FilePathWatcher -class NET_EXPORT_PRIVATE FilePathWatcherShim - : NON_EXPORTED_BASE(public base::NonThreadSafe) { - public: - FilePathWatcherShim(); - virtual ~FilePathWatcherShim(); - virtual bool Watch( - const FilePath& path, - base::files::FilePathWatcher::Delegate* delegate) WARN_UNUSED_RESULT; - private: - scoped_ptr<base::files::FilePathWatcher> watcher_; - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherShim); -}; - -class NET_EXPORT_PRIVATE FilePathWatcherFactory - : NON_EXPORTED_BASE(public base::NonThreadSafe) { - public: - FilePathWatcherFactory() {} - virtual ~FilePathWatcherFactory() {} - virtual FilePathWatcherShim* CreateFilePathWatcher(); - private: - DISALLOW_COPY_AND_ASSIGN(FilePathWatcherFactory); -}; - -// Convenience wrapper which maintains a FilePathWatcher::Delegate, restarts -// Watch on failures and calls PooledReader::ReadNow when OnFilePathChanged. -// Cancels the PooledReader upon destruction. -class NET_EXPORT_PRIVATE WatchingFileReader - : NON_EXPORTED_BASE(public base::NonThreadSafe), - NON_EXPORTED_BASE(public base::SupportsWeakPtr<WatchingFileReader>) { - public: - WatchingFileReader(); - virtual ~WatchingFileReader(); - - // Must be called at most once. Made virtual to allow mocking. - virtual void StartWatch(const FilePath& path, SerialWorker* reader); - - void set_watcher_factory(FilePathWatcherFactory* factory) { - DCHECK(!watcher_.get()); - watcher_factory_.reset(factory); - } - - FilePath get_path() const { - return path_; - } - - // Delay between calls to FilePathWatcher::Watch. - static const int kWatchRetryDelayMs = 100; - - private: - class WatcherDelegate; - // FilePathWatcher::Delegate interface exported via WatcherDelegate - virtual void OnFilePathChanged(const FilePath& path); - virtual void OnFilePathError(const FilePath& path); - - void RescheduleWatch(); - void RestartWatch(); - - FilePath path_; - scoped_ptr<FilePathWatcherFactory> watcher_factory_; - scoped_ptr<FilePathWatcherShim> watcher_; - scoped_refptr<WatcherDelegate> watcher_delegate_; - scoped_refptr<SerialWorker> reader_; - - DISALLOW_COPY_AND_ASSIGN(WatchingFileReader); -}; - -} // namespace net - -#endif // NET_DNS_WATCHING_FILE_READER_H_ diff --git a/net/dns/watching_file_reader_unittest.cc b/net/dns/watching_file_reader_unittest.cc deleted file mode 100644 index 7ff5534..0000000 --- a/net/dns/watching_file_reader_unittest.cc +++ /dev/null @@ -1,168 +0,0 @@ -// Copyright (c) 2011 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 "net/dns/watching_file_reader.h" - -#include "base/bind.h" -#include "base/message_loop.h" -#include "base/synchronization/lock.h" -#include "base/synchronization/waitable_event.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace net { - -namespace { - -class WatchingFileReaderTest : public testing::Test { - public: - - // Mocks - - class MockWorker : public SerialWorker { - public: - explicit MockWorker(WatchingFileReaderTest* t) : test_(t) {} - virtual void WorkNow() OVERRIDE { - test_->OnWork(); - } - virtual void DoWork() OVERRIDE {} - virtual void OnWorkFinished() OVERRIDE {} - private: - virtual ~MockWorker() {} - WatchingFileReaderTest* test_; - }; - - class MockFilePathWatcherShim - : public FilePathWatcherShim { - public: - typedef base::files::FilePathWatcher::Delegate Delegate; - - explicit MockFilePathWatcherShim(WatchingFileReaderTest* t) : test_(t) {} - virtual ~MockFilePathWatcherShim() { - test_->OnShimDestroyed(this); - } - - // Enforce one-Watch-per-lifetime as the original FilePathWatcher - virtual bool Watch(const FilePath& path, - Delegate* delegate) OVERRIDE { - EXPECT_TRUE(path_.empty()) << "Only one-Watch-per-lifetime allowed."; - EXPECT_TRUE(!delegate_.get()); - path_ = path; - delegate_ = delegate; - return test_->OnWatch(); - } - - void PathChanged() { - delegate_->OnFilePathChanged(path_); - } - - void PathError() { - delegate_->OnFilePathError(path_); - } - - private: - FilePath path_; - scoped_refptr<Delegate> delegate_; - WatchingFileReaderTest* test_; - }; - - class MockFilePathWatcherFactory - : public FilePathWatcherFactory { - public: - explicit MockFilePathWatcherFactory(WatchingFileReaderTest* t) - : test(t) {} - virtual FilePathWatcherShim* - CreateFilePathWatcher() OVERRIDE { - return test->CreateFilePathWatcher(); - } - WatchingFileReaderTest* test; - }; - - // Helpers for mocks. - - FilePathWatcherShim* CreateFilePathWatcher() { - watcher_shim_ = new MockFilePathWatcherShim(this); - return watcher_shim_; - } - - void OnShimDestroyed(MockFilePathWatcherShim* destroyed_shim) { - // Precaution to avoid segfault. - if (watcher_shim_ == destroyed_shim) - watcher_shim_ = NULL; - } - - // On each event, post QuitTask to allow use of MessageLoop::Run() to - // synchronize the threads. - - bool OnWatch() { - EXPECT_TRUE(message_loop_ == MessageLoop::current()); - BreakNow("OnWatch"); - return !fail_on_watch_; - } - - void OnWork() { - EXPECT_TRUE(message_loop_ == MessageLoop::current()); - BreakNow("OnWork"); - } - - protected: - void BreakCallback(std::string breakpoint) { - breakpoint_ = breakpoint; - MessageLoop::current()->QuitNow(); - } - - void BreakNow(std::string b) { - message_loop_->PostTask(FROM_HERE, - base::Bind(&WatchingFileReaderTest::BreakCallback, - base::Unretained(this), b)); - } - - void RunUntilBreak(std::string b) { - message_loop_->Run(); - ASSERT_EQ(breakpoint_, b); - } - - WatchingFileReaderTest() - : watcher_shim_(NULL), - fail_on_watch_(false), - message_loop_(MessageLoop::current()), - watcher_(new WatchingFileReader()) { - watcher_->set_watcher_factory(new MockFilePathWatcherFactory(this)); - } - - MockFilePathWatcherShim* watcher_shim_; - bool fail_on_watch_; - MessageLoop* message_loop_; - scoped_ptr<WatchingFileReader> watcher_; - - std::string breakpoint_; -}; - -TEST_F(WatchingFileReaderTest, FilePathWatcherFailures) { - fail_on_watch_ = true; - watcher_->StartWatch(FilePath(FILE_PATH_LITERAL("some_file_name")), - new MockWorker(this)); - RunUntilBreak("OnWatch"); - - fail_on_watch_ = false; - RunUntilBreak("OnWatch"); // Due to backoff this will take 100ms. - RunUntilBreak("OnWork"); - - ASSERT_TRUE(watcher_shim_); - watcher_shim_->PathError(); - RunUntilBreak("OnWatch"); - RunUntilBreak("OnWork"); - - message_loop_->AssertIdle(); - - ASSERT_TRUE(watcher_shim_); - watcher_shim_->PathChanged(); - RunUntilBreak("OnWork"); - - message_loop_->AssertIdle(); -} - -} // namespace - -} // namespace net - diff --git a/net/net.gyp b/net/net.gyp index 454676c..4360fc0 100644 --- a/net/net.gyp +++ b/net/net.gyp @@ -353,10 +353,10 @@ 'dns/dns_session.h', 'dns/dns_transaction.cc', 'dns/dns_transaction.h', + 'dns/file_path_watcher_wrapper.cc', + 'dns/file_path_watcher_wrapper.h', 'dns/serial_worker.cc', 'dns/serial_worker.h', - 'dns/watching_file_reader.cc', - 'dns/watching_file_reader.h', 'ftp/ftp_auth_cache.cc', 'ftp/ftp_auth_cache.h', 'ftp/ftp_ctrl_response_buffer.cc', @@ -1091,8 +1091,8 @@ 'dns/dns_query_unittest.cc', 'dns/dns_response_unittest.cc', 'dns/dns_transaction_unittest.cc', + 'dns/file_path_watcher_wrapper_unittest.cc', 'dns/serial_worker_unittest.cc', - 'dns/watching_file_reader_unittest.cc', 'ftp/ftp_auth_cache_unittest.cc', 'ftp/ftp_ctrl_response_buffer_unittest.cc', 'ftp/ftp_directory_listing_parser_ls_unittest.cc', |