diff options
author | pauljensen <pauljensen@chromium.org> | 2015-04-16 17:11:42 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-04-17 00:12:08 +0000 |
commit | 101ed37a9cb2f1a7e906ab9f4e3a1c6331cdf8b3 (patch) | |
tree | d5800843c08d572a92598810ba107bb5a8b3bb80 | |
parent | 6ae2d0ad19bfe63d071d5dd24240e3b5cc7273ca (diff) | |
download | chromium_src-101ed37a9cb2f1a7e906ab9f4e3a1c6331cdf8b3.zip chromium_src-101ed37a9cb2f1a7e906ab9f4e3a1c6331cdf8b3.tar.gz chromium_src-101ed37a9cb2f1a7e906ab9f4e3a1c6331cdf8b3.tar.bz2 |
Avoid initial NetworkChangeNotifier OnDNSChanged() signal on Android
When the DnsConfigServicePosix finishes its initial reading of the
system DNS config, it normally triggers a NetworkChangeNotifier (NCN)
OnDNSChanged() signal. This can cause in-flight network requests to
abort with ERR_NETWORK_CHANGED. Avoid aborting requests by:
1. Adding a new NCN signal, OnInitialDNSConfigRead which indicates
the initial DNS config reading completed but does not represent
a change in DNS config.
2. Modify HostResolverImpl to not abort requests upon this new
signal (like it does for the OnDNSChanged signal).
3. Add logic to NetworkChangeNotifierAndroid to emit this new
signal when safe to do so. Network requests begin being issued
immediately after the NCN (and hence DnsConfigService) is
initialized, so we need to be sure no network change signals
are missed between NCN initialization completing and the
OnInitialDNSConfigRead signal. This is tricky because the
NCN (and hence DnsConfigService) is initialized on threads
where file I/O is not allowed. Were file I/O allowed we could
simply slurp up the DNS config and hosts file. Instead we
start listening for network changes (which is our trigger signal
for DNS changes on Android) on the initialization thread and
record the current time to later compare against the hosts
file's last-modified time to check for changes to the file.
Actual loading of the DNS config and hosts file is done on
another thread that allows file I/O.
BUG=470897
Review URL: https://codereview.chromium.org/1047103002
Cr-Commit-Position: refs/heads/master@{#325560}
-rw-r--r-- | chrome/browser/net/dns_probe_service.cc | 4 | ||||
-rw-r--r-- | chrome/browser/net/dns_probe_service.h | 1 | ||||
-rw-r--r-- | net/android/network_change_notifier_android.cc | 54 | ||||
-rw-r--r-- | net/android/network_change_notifier_android.h | 5 | ||||
-rw-r--r-- | net/android/network_change_notifier_android_unittest.cc | 64 | ||||
-rw-r--r-- | net/android/network_change_notifier_factory_android.cc | 2 | ||||
-rw-r--r-- | net/base/network_change_notifier.cc | 36 | ||||
-rw-r--r-- | net/base/network_change_notifier.h | 11 | ||||
-rw-r--r-- | net/dns/dns_config_service_posix.cc | 93 | ||||
-rw-r--r-- | net/dns/dns_config_service_posix.h | 33 | ||||
-rw-r--r-- | net/dns/dns_config_service_posix_unittest.cc | 118 | ||||
-rw-r--r-- | net/dns/dns_config_service_unittest.cc | 4 | ||||
-rw-r--r-- | net/dns/host_resolver_impl.cc | 44 | ||||
-rw-r--r-- | net/dns/host_resolver_impl.h | 3 | ||||
-rw-r--r-- | net/dns/host_resolver_impl_unittest.cc | 22 | ||||
-rw-r--r-- | net/net.gyp | 9 | ||||
-rw-r--r-- | net/tools/net_watcher/net_watcher.cc | 3 |
17 files changed, 431 insertions, 75 deletions
diff --git a/chrome/browser/net/dns_probe_service.cc b/chrome/browser/net/dns_probe_service.cc index 38b7904..09c852c 100644 --- a/chrome/browser/net/dns_probe_service.cc +++ b/chrome/browser/net/dns_probe_service.cc @@ -120,6 +120,10 @@ void DnsProbeService::OnDNSChanged() { SetSystemClientToCurrentConfig(); } +void DnsProbeService::OnInitialDNSConfigRead() { + OnDNSChanged(); +} + void DnsProbeService::SetSystemClientForTesting( scoped_ptr<DnsClient> system_client) { system_runner_.SetClient(system_client.Pass()); diff --git a/chrome/browser/net/dns_probe_service.h b/chrome/browser/net/dns_probe_service.h index 37a294a..f20265d 100644 --- a/chrome/browser/net/dns_probe_service.h +++ b/chrome/browser/net/dns_probe_service.h @@ -39,6 +39,7 @@ class DnsProbeService : public net::NetworkChangeNotifier::DNSObserver { // NetworkChangeNotifier::DNSObserver implementation: void OnDNSChanged() override; + void OnInitialDNSConfigRead() override; void SetSystemClientForTesting(scoped_ptr<net::DnsClient> system_client); void SetPublicClientForTesting(scoped_ptr<net::DnsClient> public_client); diff --git a/net/android/network_change_notifier_android.cc b/net/android/network_change_notifier_android.cc index d1ad338..36ebda6 100644 --- a/net/android/network_change_notifier_android.cc +++ b/net/android/network_change_notifier_android.cc @@ -61,29 +61,43 @@ #include "base/threading/thread.h" #include "net/base/address_tracker_linux.h" -#include "net/dns/dns_config_service.h" +#include "net/dns/dns_config_service_posix.h" namespace net { // Thread on which we can run DnsConfigService, which requires a TYPE_IO // message loop to monitor /system/etc/hosts. class NetworkChangeNotifierAndroid::DnsConfigServiceThread - : public base::Thread { + : public base::Thread, + public NetworkChangeNotifier::NetworkChangeObserver { public: - DnsConfigServiceThread() + DnsConfigServiceThread(const DnsConfig* dns_config_for_testing) : base::Thread("DnsConfigService"), + dns_config_for_testing_(dns_config_for_testing), + creation_time_(base::Time::Now()), address_tracker_(base::Bind(base::DoNothing), base::Bind(base::DoNothing), // We're only interested in tunnel interface changes. base::Bind(NotifyNetworkChangeNotifierObservers)) {} - ~DnsConfigServiceThread() override { Stop(); } + ~DnsConfigServiceThread() override { + NetworkChangeNotifier::RemoveNetworkChangeObserver(this); + Stop(); + } + + void InitAfterStart() { + DCHECK(IsRunning()); + NetworkChangeNotifier::AddNetworkChangeObserver(this); + } void Init() override { address_tracker_.Init(); - dns_config_service_ = DnsConfigService::CreateSystemService(); + dns_config_service_.reset(new internal::DnsConfigServicePosix()); + if (dns_config_for_testing_) + dns_config_service_->SetDnsConfigForTesting(dns_config_for_testing_); dns_config_service_->WatchConfig( - base::Bind(&NetworkChangeNotifier::SetDnsConfig)); + base::Bind(&DnsConfigServiceThread::DnsConfigChangeCallback, + base::Unretained(this))); } void CleanUp() override { dns_config_service_.reset(); } @@ -94,7 +108,26 @@ class NetworkChangeNotifierAndroid::DnsConfigServiceThread } private: - scoped_ptr<DnsConfigService> dns_config_service_; + void DnsConfigChangeCallback(const DnsConfig& config) { + DCHECK(task_runner()->BelongsToCurrentThread()); + if (dns_config_service_->SeenChangeSince(creation_time_)) { + NetworkChangeNotifier::SetDnsConfig(config); + } else { + NetworkChangeNotifier::SetInitialDnsConfig(config); + } + } + + // NetworkChangeNotifier::NetworkChangeObserver implementation: + void OnNetworkChanged(NetworkChangeNotifier::ConnectionType type) override { + task_runner()->PostTask( + FROM_HERE, + base::Bind(&internal::DnsConfigServicePosix::OnNetworkChanged, + base::Unretained(dns_config_service_.get()), type)); + } + + const DnsConfig* dns_config_for_testing_; + const base::Time creation_time_; + scoped_ptr<internal::DnsConfigServicePosix> dns_config_service_; // Used to detect tunnel state changes. internal::AddressTrackerLinux address_tracker_; @@ -130,13 +163,16 @@ bool NetworkChangeNotifierAndroid::Register(JNIEnv* env) { } NetworkChangeNotifierAndroid::NetworkChangeNotifierAndroid( - NetworkChangeNotifierDelegateAndroid* delegate) + NetworkChangeNotifierDelegateAndroid* delegate, + const DnsConfig* dns_config_for_testing) : NetworkChangeNotifier(NetworkChangeCalculatorParamsAndroid()), delegate_(delegate), - dns_config_service_thread_(new DnsConfigServiceThread()) { + dns_config_service_thread_( + new DnsConfigServiceThread(dns_config_for_testing)) { delegate_->AddObserver(this); dns_config_service_thread_->StartWithOptions( base::Thread::Options(base::MessageLoop::TYPE_IO, 0)); + dns_config_service_thread_->InitAfterStart(); } // static diff --git a/net/android/network_change_notifier_android.h b/net/android/network_change_notifier_android.h index f6d43d0..bb46693 100644 --- a/net/android/network_change_notifier_android.h +++ b/net/android/network_change_notifier_android.h @@ -13,6 +13,7 @@ namespace net { +struct DnsConfig; class NetworkChangeNotifierAndroidTest; class NetworkChangeNotifierFactoryAndroid; @@ -66,8 +67,8 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierAndroid class DnsConfigServiceThread; - explicit NetworkChangeNotifierAndroid( - NetworkChangeNotifierDelegateAndroid* delegate); + NetworkChangeNotifierAndroid(NetworkChangeNotifierDelegateAndroid* delegate, + const DnsConfig* dns_config_for_testing); static NetworkChangeCalculatorParams NetworkChangeCalculatorParamsAndroid(); diff --git a/net/android/network_change_notifier_android_unittest.cc b/net/android/network_change_notifier_android_unittest.cc index 83dd70e..30ee4f0 100644 --- a/net/android/network_change_notifier_android_unittest.cc +++ b/net/android/network_change_notifier_android_unittest.cc @@ -12,6 +12,8 @@ #include "net/android/network_change_notifier_android.h" #include "net/android/network_change_notifier_delegate_android.h" #include "net/base/network_change_notifier.h" +#include "net/dns/dns_config_service.h" +#include "net/dns/dns_protocol.h" #include "testing/gtest/include/gtest/gtest.h" namespace net { @@ -60,6 +62,30 @@ class NetworkChangeNotifierObserver int notifications_count_; }; +class DNSChangeObserver : public NetworkChangeNotifier::DNSObserver { + public: + DNSChangeObserver() + : change_notifications_count_(0), initial_notifications_count_(0) {} + + // NetworkChangeNotifier::DNSObserver: + void OnDNSChanged() override { change_notifications_count_++; } + + void OnInitialDNSConfigRead() override { + initial_notifications_count_++; + base::MessageLoop::current()->Quit(); + } + + int change_notifications_count() const { return change_notifications_count_; } + + int initial_notifications_count() const { + return initial_notifications_count_; + } + + private: + int change_notifications_count_; + int initial_notifications_count_; +}; + } // namespace class BaseNetworkChangeNotifierAndroidTest : public testing::Test { @@ -175,7 +201,12 @@ TEST_F(NetworkChangeNotifierDelegateAndroidTest, DelegateObserverNotified) { class NetworkChangeNotifierAndroidTest : public BaseNetworkChangeNotifierAndroidTest { protected: - NetworkChangeNotifierAndroidTest() : notifier_(&delegate_) { + void SetUp() override { + IPAddressNumber dns_number; + ASSERT_TRUE(ParseIPLiteralToNumber("8.8.8.8", &dns_number)); + dns_config_.nameservers.push_back( + IPEndPoint(dns_number, dns_protocol::kDefaultPort)); + notifier_.reset(new NetworkChangeNotifierAndroid(&delegate_, &dns_config_)); NetworkChangeNotifier::AddConnectionTypeObserver( &connection_type_observer_); NetworkChangeNotifier::AddConnectionTypeObserver( @@ -185,7 +216,8 @@ class NetworkChangeNotifierAndroidTest NetworkChangeNotifierObserver connection_type_observer_; NetworkChangeNotifierObserver other_connection_type_observer_; NetworkChangeNotifier::DisableForTest disable_for_test_; - NetworkChangeNotifierAndroid notifier_; + DnsConfig dns_config_; + scoped_ptr<NetworkChangeNotifierAndroid> notifier_; }; // When a NetworkChangeNotifierAndroid is observing a @@ -194,13 +226,10 @@ class NetworkChangeNotifierAndroidTest // NetworkChangeNotifierAndroid should reflect that state. TEST_F(NetworkChangeNotifierAndroidTest, NotificationsSentToNetworkChangeNotifierAndroid) { - RunTest( - base::Bind( - &NetworkChangeNotifierObserver::notifications_count, - base::Unretained(&connection_type_observer_)), - base::Bind( - &NetworkChangeNotifierAndroid::GetCurrentConnectionType, - base::Unretained(¬ifier_))); + RunTest(base::Bind(&NetworkChangeNotifierObserver::notifications_count, + base::Unretained(&connection_type_observer_)), + base::Bind(&NetworkChangeNotifierAndroid::GetCurrentConnectionType, + base::Unretained(notifier_.get()))); } // When a NetworkChangeNotifierAndroid's connection state changes, it should @@ -220,13 +249,13 @@ TEST_F(NetworkChangeNotifierAndroidTest, TEST_F(NetworkChangeNotifierAndroidTest, MaxBandwidth) { SetOnline(); EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN, - notifier_.GetConnectionType()); + notifier_->GetConnectionType()); EXPECT_EQ(std::numeric_limits<double>::infinity(), - notifier_.GetMaxBandwidth()); + notifier_->GetMaxBandwidth()); SetOffline(); EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE, - notifier_.GetConnectionType()); - EXPECT_EQ(0.0, notifier_.GetMaxBandwidth()); + notifier_->GetConnectionType()); + EXPECT_EQ(0.0, notifier_->GetMaxBandwidth()); } TEST_F(NetworkChangeNotifierDelegateAndroidTest, @@ -240,4 +269,13 @@ TEST_F(NetworkChangeNotifierDelegateAndroidTest, EXPECT_EQ(2, delegate_observer_.bandwidth_notifications_count()); } +TEST_F(NetworkChangeNotifierAndroidTest, InitialSignal) { + DNSChangeObserver dns_change_observer; + NetworkChangeNotifier::AddDNSObserver(&dns_change_observer); + base::MessageLoop::current()->Run(); + EXPECT_EQ(1, dns_change_observer.initial_notifications_count()); + EXPECT_EQ(0, dns_change_observer.change_notifications_count()); + NetworkChangeNotifier::RemoveDNSObserver(&dns_change_observer); +} + } // namespace net diff --git a/net/android/network_change_notifier_factory_android.cc b/net/android/network_change_notifier_factory_android.cc index 64b9ca2..3296918 100644 --- a/net/android/network_change_notifier_factory_android.cc +++ b/net/android/network_change_notifier_factory_android.cc @@ -14,7 +14,7 @@ NetworkChangeNotifierFactoryAndroid::NetworkChangeNotifierFactoryAndroid() {} NetworkChangeNotifierFactoryAndroid::~NetworkChangeNotifierFactoryAndroid() {} NetworkChangeNotifier* NetworkChangeNotifierFactoryAndroid::CreateInstance() { - return new NetworkChangeNotifierAndroid(&delegate_); + return new NetworkChangeNotifierAndroid(&delegate_, nullptr); } } // namespace net diff --git a/net/base/network_change_notifier.cc b/net/base/network_change_notifier.cc index 3ec6369..86bc253 100644 --- a/net/base/network_change_notifier.cc +++ b/net/base/network_change_notifier.cc @@ -776,6 +776,12 @@ void NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests( } // static +void NetworkChangeNotifier::NotifyObserversOfInitialDNSConfigReadForTests() { + if (g_network_change_notifier) + g_network_change_notifier->NotifyObserversOfInitialDNSConfigReadImpl(); +} + +// static void NetworkChangeNotifier::SetTestNotificationsOnly(bool test_only) { if (g_network_change_notifier) g_network_change_notifier->test_notifications_only_ = test_only; @@ -941,6 +947,14 @@ void NetworkChangeNotifier::NotifyObserversOfDNSChange() { } // static +void NetworkChangeNotifier::NotifyObserversOfInitialDNSConfigRead() { + if (g_network_change_notifier && + !g_network_change_notifier->test_notifications_only_) { + g_network_change_notifier->NotifyObserversOfInitialDNSConfigReadImpl(); + } +} + +// static void NetworkChangeNotifier::SetDnsConfig(const DnsConfig& config) { if (!g_network_change_notifier) return; @@ -948,6 +962,20 @@ void NetworkChangeNotifier::SetDnsConfig(const DnsConfig& config) { NotifyObserversOfDNSChange(); } +// static +void NetworkChangeNotifier::SetInitialDnsConfig(const DnsConfig& config) { + if (!g_network_change_notifier) + return; +#if DCHECK_IS_ON() + // Verify we've never received a valid DnsConfig previously. + DnsConfig old_config; + g_network_change_notifier->network_state_->GetDnsConfig(&old_config); + DCHECK(!old_config.IsValid()); +#endif + g_network_change_notifier->network_state_->SetDnsConfig(config); + NotifyObserversOfInitialDNSConfigRead(); +} + void NetworkChangeNotifier::NotifyObserversOfIPAddressChangeImpl() { ip_address_observer_list_->Notify(FROM_HERE, &IPAddressObserver::OnIPAddressChanged); @@ -969,6 +997,11 @@ void NetworkChangeNotifier::NotifyObserversOfDNSChangeImpl() { resolver_state_observer_list_->Notify(FROM_HERE, &DNSObserver::OnDNSChanged); } +void NetworkChangeNotifier::NotifyObserversOfInitialDNSConfigReadImpl() { + resolver_state_observer_list_->Notify(FROM_HERE, + &DNSObserver::OnInitialDNSConfigRead); +} + void NetworkChangeNotifier::NotifyObserversOfMaxBandwidthChangeImpl( double max_bandwidth_mbps) { max_bandwidth_observer_list_->Notify( @@ -987,4 +1020,7 @@ NetworkChangeNotifier::DisableForTest::~DisableForTest() { g_network_change_notifier = network_change_notifier_; } +void NetworkChangeNotifier::DNSObserver::OnInitialDNSConfigRead() { +} + } // namespace net diff --git a/net/base/network_change_notifier.h b/net/base/network_change_notifier.h index bce7373..b2e54f5 100644 --- a/net/base/network_change_notifier.h +++ b/net/base/network_change_notifier.h @@ -135,6 +135,9 @@ class NET_EXPORT NetworkChangeNotifier { // Will be called when the DNS settings of the system may have changed. // Use GetDnsConfig to obtain the current settings. virtual void OnDNSChanged() = 0; + // Will be called when DNS settings of the system have been loaded. + // Use GetDnsConfig to obtain the current settings. + virtual void OnInitialDNSConfigRead(); protected: DNSObserver() {} @@ -305,6 +308,7 @@ class NET_EXPORT NetworkChangeNotifier { static void NotifyObserversOfConnectionTypeChangeForTests( ConnectionType type); static void NotifyObserversOfNetworkChangeForTests(ConnectionType type); + static void NotifyObserversOfInitialDNSConfigReadForTests(); // Enable or disable notifications from the host. After setting to true, be // sure to pump the RunLoop until idle to finish any preexisting @@ -400,11 +404,15 @@ class NET_EXPORT NetworkChangeNotifier { static void NotifyObserversOfIPAddressChange(); static void NotifyObserversOfConnectionTypeChange(); static void NotifyObserversOfDNSChange(); + static void NotifyObserversOfInitialDNSConfigRead(); static void NotifyObserversOfNetworkChange(ConnectionType type); static void NotifyObserversOfMaxBandwidthChange(double max_bandwidth_mbps); - // Stores |config| in NetworkState and notifies observers. + // Stores |config| in NetworkState and notifies OnDNSChanged observers. static void SetDnsConfig(const DnsConfig& config); + // Stores |config| in NetworkState and notifies OnInitialDNSConfigRead + // observers. + static void SetInitialDnsConfig(const DnsConfig& config); private: friend class HostResolverImplDnsTest; @@ -418,6 +426,7 @@ class NET_EXPORT NetworkChangeNotifier { void NotifyObserversOfIPAddressChangeImpl(); void NotifyObserversOfConnectionTypeChangeImpl(ConnectionType type); void NotifyObserversOfDNSChangeImpl(); + void NotifyObserversOfInitialDNSConfigReadImpl(); void NotifyObserversOfNetworkChangeImpl(ConnectionType type); void NotifyObserversOfMaxBandwidthChangeImpl(double max_bandwidth_mbps); diff --git a/net/dns/dns_config_service_posix.cc b/net/dns/dns_config_service_posix.cc index 135f3fd..6f10355 100644 --- a/net/dns/dns_config_service_posix.cc +++ b/net/dns/dns_config_service_posix.cc @@ -8,6 +8,7 @@ #include "base/basictypes.h" #include "base/bind.h" +#include "base/files/file.h" #include "base/files/file_path.h" #include "base/files/file_path_watcher.h" #include "base/lazy_instance.h" @@ -58,22 +59,14 @@ class DnsConfigWatcher { #elif defined(OS_ANDROID) // On Android, assume DNS config may have changed on every network change. -class DnsConfigWatcher : public NetworkChangeNotifier::NetworkChangeObserver { +class DnsConfigWatcher { public: - DnsConfigWatcher() { - NetworkChangeNotifier::AddNetworkChangeObserver(this); - } - - ~DnsConfigWatcher() override { - NetworkChangeNotifier::RemoveNetworkChangeObserver(this); - } - bool Watch(const base::Callback<void(bool succeeded)>& callback) { callback_ = callback; return true; } - void OnNetworkChanged(NetworkChangeNotifier::ConnectionType type) override { + void OnNetworkChanged(NetworkChangeNotifier::ConnectionType type) { if (!callback_.is_null() && type != NetworkChangeNotifier::CONNECTION_NONE) callback_.Run(true); } @@ -201,8 +194,7 @@ ConfigParsePosixResult ReadDnsConfig(DnsConfig* dns_config) { class DnsConfigServicePosix::Watcher { public: explicit Watcher(DnsConfigServicePosix* service) - : service_(service), - weak_factory_(this) {} + : service_(service), weak_factory_(this) {} ~Watcher() {} bool Watch() { @@ -215,9 +207,9 @@ class DnsConfigServicePosix::Watcher { DNS_CONFIG_WATCH_FAILED_TO_START_CONFIG, DNS_CONFIG_WATCH_MAX); } - if (!hosts_watcher_.Watch(base::FilePath(kFilePathHosts), false, - base::Bind(&Watcher::OnHostsChanged, - base::Unretained(this)))) { + if (!hosts_watcher_.Watch( + base::FilePath(service_->file_path_hosts_), false, + base::Bind(&Watcher::OnHostsChanged, base::Unretained(this)))) { LOG(ERROR) << "DNS hosts watch failed to start."; success = false; UMA_HISTOGRAM_ENUMERATION("AsyncDNS.WatchStatus", @@ -227,8 +219,17 @@ class DnsConfigServicePosix::Watcher { return success; } +#if defined(OS_ANDROID) + void OnNetworkChanged(NetworkChangeNotifier::ConnectionType type) { + config_watcher_.OnNetworkChanged(type); + } +#endif // defined(OS_ANDROID) + private: void OnConfigChanged(bool succeeded) { +#if defined(OS_ANDROID) + service_->seen_config_change_ = true; +#endif // defined(OS_ANDROID) // Ignore transient flutter of resolv.conf by delaying the signal a bit. const base::TimeDelta kDelay = base::TimeDelta::FromMilliseconds(50); base::MessageLoop::current()->PostDelayedTask( @@ -265,6 +266,10 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { void DoWork() override { base::TimeTicks start_time = base::TimeTicks::Now(); ConfigParsePosixResult result = ReadDnsConfig(&dns_config_); + if (service_->dns_config_for_testing_) { + dns_config_ = *service_->dns_config_for_testing_; + result = CONFIG_PARSE_POSIX_OK; + } switch (result) { case CONFIG_PARSE_POSIX_MISSING_OPTIONS: case CONFIG_PARSE_POSIX_UNHANDLED_OPTIONS: @@ -308,14 +313,15 @@ class DnsConfigServicePosix::ConfigReader : public SerialWorker { class DnsConfigServicePosix::HostsReader : public SerialWorker { public: explicit HostsReader(DnsConfigServicePosix* service) - : service_(service), path_(kFilePathHosts), success_(false) {} + : service_(service), success_(false) {} private: ~HostsReader() override {} void DoWork() override { base::TimeTicks start_time = base::TimeTicks::Now(); - success_ = ParseHostsFile(path_, &hosts_); + success_ = + ParseHostsFile(base::FilePath(service_->file_path_hosts_), &hosts_); UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HostParseResult", success_); UMA_HISTOGRAM_TIMES("AsyncDNS.HostsParseDuration", base::TimeTicks::Now() - start_time); @@ -330,7 +336,6 @@ class DnsConfigServicePosix::HostsReader : public SerialWorker { } DnsConfigServicePosix* service_; - const base::FilePath path_; // Written in DoWork, read in OnWorkFinished, no locking necessary. DnsHosts hosts_; bool success_; @@ -339,8 +344,16 @@ class DnsConfigServicePosix::HostsReader : public SerialWorker { }; DnsConfigServicePosix::DnsConfigServicePosix() - : config_reader_(new ConfigReader(this)), - hosts_reader_(new HostsReader(this)) {} + : file_path_hosts_(kFilePathHosts), + dns_config_for_testing_(nullptr), + config_reader_(new ConfigReader(this)), + hosts_reader_(new HostsReader(this)) +#if defined(OS_ANDROID) + , + seen_config_change_(false) +#endif // defined(OS_ANDROID) +{ +} DnsConfigServicePosix::~DnsConfigServicePosix() { config_reader_->Cancel(); @@ -386,6 +399,12 @@ void DnsConfigServicePosix::OnHostsChanged(bool succeeded) { } } +void DnsConfigServicePosix::SetDnsConfigForTesting( + const DnsConfig* dns_config) { + DCHECK(CalledOnValidThread()); + dns_config_for_testing_ = dns_config; +} + #if !defined(OS_ANDROID) ConfigParsePosixResult ConvertResStateToDnsConfig(const struct __res_state& res, DnsConfig* dns_config) { @@ -493,7 +512,39 @@ ConfigParsePosixResult ConvertResStateToDnsConfig(const struct __res_state& res, } return CONFIG_PARSE_POSIX_OK; } -#endif // !defined(OS_ANDROID) + +#else // defined(OS_ANDROID) + +bool DnsConfigServicePosix::SeenChangeSince( + const base::Time& since_time) const { + DCHECK(CalledOnValidThread()); + if (seen_config_change_) + return true; + base::File hosts(base::FilePath(file_path_hosts_), + base::File::FLAG_OPEN | base::File::FLAG_READ); + base::File::Info hosts_info; + // File last modified times are not nearly as accurate as Time::Now() and are + // rounded down. This means a file modified at 1:23.456 might only + // be given a last modified time of 1:23.450. If we compared the last + // modified time directly to |since_time| we might miss changes to the hosts + // file because of this rounding down. To account for this the |since_time| + // is pushed back by 1s which should more than account for any rounding. + // In practice file modified times on Android are two orders of magnitude + // more accurate than this 1s. In practice the hosts file on Android always + // contains "127.0.0.1 localhost" and is never modified after Android is + // installed. + return !hosts.GetInfo(&hosts_info) || + hosts_info.last_modified >= + (since_time - base::TimeDelta::FromSeconds(1)); +} + +void DnsConfigServicePosix::OnNetworkChanged( + NetworkChangeNotifier::ConnectionType type) { + DCHECK(CalledOnValidThread()); + DCHECK(watcher_); + watcher_->OnNetworkChanged(type); +} +#endif // defined(OS_ANDROID) } // namespace internal diff --git a/net/dns/dns_config_service_posix.h b/net/dns/dns_config_service_posix.h index 349c82d..7e266ba 100644 --- a/net/dns/dns_config_service_posix.h +++ b/net/dns/dns_config_service_posix.h @@ -12,25 +12,50 @@ #endif #include "base/compiler_specific.h" +#include "base/files/file_path.h" #include "net/base/net_export.h" +#include "net/base/network_change_notifier.h" #include "net/dns/dns_config_service.h" +namespace base { +class Time; +} // namespace base + namespace net { // Use DnsConfigService::CreateSystemService to use it outside of tests. namespace internal { +// Note: On Android NetworkChangeNotifier::OnNetworkChanged() signals must be +// passed in via calls to OnNetworkChanged(). class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService { public: DnsConfigServicePosix(); ~DnsConfigServicePosix() override; +#if defined(OS_ANDROID) + // Returns whether the DnsConfigServicePosix witnessed a DNS configuration + // change since |since_time|. Requires that callers have started listening + // for NetworkChangeNotifier::OnNetworkChanged() signals, and passing them in + // via OnNetworkChanged(), prior to |since_time|. + bool SeenChangeSince(const base::Time& since_time) const; + // NetworkChangeNotifier::OnNetworkChanged() signals must be passed + // in via calls to OnNetworkChanged(). Allowing external sources of + // this signal allows users of DnsConfigServicePosix to start watching for + // NetworkChangeNotifier::OnNetworkChanged() signals prior to the + // DnsConfigServicePosix even being created. + void OnNetworkChanged(NetworkChangeNotifier::ConnectionType type); +#endif + + void SetDnsConfigForTesting(const DnsConfig* dns_config); + protected: // DnsConfigService: void ReadNow() override; bool StartWatching() override; private: + friend class DnsConfigServicePosixTest; class Watcher; class ConfigReader; class HostsReader; @@ -39,8 +64,16 @@ class NET_EXPORT_PRIVATE DnsConfigServicePosix : public DnsConfigService { void OnHostsChanged(bool succeeded); scoped_ptr<Watcher> watcher_; + // Allow a mock hosts file for testing purposes. + const base::FilePath::CharType* file_path_hosts_; + // Allow a mock DNS server for testing purposes. + const DnsConfig* dns_config_for_testing_; scoped_refptr<ConfigReader> config_reader_; scoped_refptr<HostsReader> hosts_reader_; +#if defined(OS_ANDROID) + // Has DnsConfigWatcher detected any config chagnes yet? + bool seen_config_change_; +#endif DISALLOW_COPY_AND_ASSIGN(DnsConfigServicePosix); }; diff --git a/net/dns/dns_config_service_posix_unittest.cc b/net/dns/dns_config_service_posix_unittest.cc index b208ffd..4a99370 100644 --- a/net/dns/dns_config_service_posix_unittest.cc +++ b/net/dns/dns_config_service_posix_unittest.cc @@ -4,14 +4,24 @@ #include <resolv.h> +#include "base/cancelable_callback.h" +#include "base/files/file_util.h" #include "base/sys_byteorder.h" +#include "base/test/test_timeouts.h" +#include "base/threading/platform_thread.h" #include "net/dns/dns_config_service_posix.h" +#include "net/dns/dns_protocol.h" #include "testing/gtest/include/gtest/gtest.h" -#if !defined(OS_ANDROID) +#if defined(OS_ANDROID) +#include "base/android/path_utils.h" +#endif // defined(OS_ANDROID) namespace net { + +#if !defined(OS_ANDROID) + namespace { // MAXNS is normally 3, but let's test 4 if possible. @@ -155,6 +165,108 @@ TEST(DnsConfigServicePosixTest, RejectEmptyNameserver) { } } // namespace -} // namespace net -#endif // !OS_ANDROID +#else // OS_ANDROID + +namespace internal { + +const char kTempHosts1[] = "127.0.0.1 localhost"; +const char kTempHosts2[] = "127.0.0.2 localhost"; + +class DnsConfigServicePosixTest : public testing::Test { + public: + DnsConfigServicePosixTest() : seen_config_(false) {} + ~DnsConfigServicePosixTest() override {} + + void OnConfigChanged(const DnsConfig& config) { + EXPECT_TRUE(config.IsValid()); + seen_config_ = true; + base::MessageLoop::current()->Quit(); + } + + void WriteMockHostsFile(const char* hosts_string) { + ASSERT_EQ(base::WriteFile(temp_file_, hosts_string, strlen(hosts_string)), + static_cast<int>(strlen(hosts_string))); + } + + void MockDNSConfig(const char* dns_server) { + IPAddressNumber dns_number; + ASSERT_TRUE(ParseIPLiteralToNumber(dns_server, &dns_number)); + test_config_.nameservers.clear(); + test_config_.nameservers.push_back( + IPEndPoint(dns_number, dns_protocol::kDefaultPort)); + service_->SetDnsConfigForTesting(&test_config_); + } + + void SetUp() override { + // TODO(pauljensen): Get rid of GetExternalStorageDirectory() when + // crbug.com/475568 is fixed. For now creating a temp file in the + // default temp directory (/data/data/...) will cause FilePathWatcher + // to fail, so create the temp file in /sdcard. + base::FilePath parent_dir; + ASSERT_TRUE(base::android::GetExternalStorageDirectory(&parent_dir)); + ASSERT_TRUE(base::CreateTemporaryFileInDir(parent_dir, &temp_file_)); + WriteMockHostsFile(kTempHosts1); + // Set the time on the hosts file back so it appears older than the + // 1s safety offset in DnsConfigServicePosix::SeenChangeSince(). + // TODO(pauljensen): Switch from Sleep() to TouchFile() when + // crbug.com/475568 is fixed. For now TouchFile() will fail in /sdcard. + base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1100)); + // // Copy real hosts file's last modified time to mock hosts file. + // base::File hosts(base::FilePath(DnsConfigServicePosix::kFilePathHosts), + // base::File::FLAG_OPEN | base::File::FLAG_READ); + // base::File::Info hosts_info; + // ASSERT_TRUE(hosts.GetInfo(&hosts_info)); + // ASSERT_TRUE(base::TouchFile(temp_file_, hosts_info.last_modified, + // hosts_info.last_accessed)); + } + + void TearDown() override { ASSERT_TRUE(base::DeleteFile(temp_file_, false)); } + + void StartWatching() { + creation_time_ = base::Time::Now(); + service_.reset(new DnsConfigServicePosix()); + service_->file_path_hosts_ = temp_file_.value().c_str(); + MockDNSConfig("8.8.8.8"); + seen_config_ = false; + service_->WatchConfig(base::Bind( + &DnsConfigServicePosixTest::OnConfigChanged, base::Unretained(this))); + ExpectChange(); + } + + void ExpectChange() { + EXPECT_FALSE(seen_config_); + base::MessageLoop::current()->Run(); + EXPECT_TRUE(seen_config_); + seen_config_ = false; + } + + bool seen_config_; + base::Time creation_time_; + base::FilePath temp_file_; + scoped_ptr<DnsConfigServicePosix> service_; + DnsConfig test_config_; +}; + +TEST_F(DnsConfigServicePosixTest, SeenChangeSince) { + // Verify SeenChangeSince() returns false if no changes + StartWatching(); + EXPECT_FALSE(service_->SeenChangeSince(creation_time_)); + // Verify SeenChangeSince() returns true if network change + MockDNSConfig("8.8.4.4"); + service_->OnNetworkChanged(NetworkChangeNotifier::CONNECTION_WIFI); + EXPECT_TRUE(service_->SeenChangeSince(creation_time_)); + ExpectChange(); + // Verify SeenChangeSince() returns true if hosts file changes + StartWatching(); + EXPECT_FALSE(service_->SeenChangeSince(creation_time_)); + WriteMockHostsFile(kTempHosts2); + EXPECT_TRUE(service_->SeenChangeSince(creation_time_)); + ExpectChange(); +} + +} // namespace internal + +#endif // OS_ANDROID + +} // namespace net diff --git a/net/dns/dns_config_service_unittest.cc b/net/dns/dns_config_service_unittest.cc index e3abafd..be3cacd 100644 --- a/net/dns/dns_config_service_unittest.cc +++ b/net/dns/dns_config_service_unittest.cc @@ -335,6 +335,10 @@ TEST_F(DnsConfigServiceTest, WatchFailure) { #if (defined(OS_POSIX) && !defined(OS_ANDROID)) || defined(OS_WIN) // TODO(szym): This is really an integration test and can time out if HOSTS is // huge. http://crbug.com/107810 +// On Android the hosts file is not user modifyable, so it's always tiny, +// however devices used for testing are likely to have no network connectivity +// and hence no DNS configuration so this test will just fail to find a valid +// config. TEST_F(DnsConfigServiceTest, DISABLED_GetSystemConfig) { service_.reset(); scoped_ptr<DnsConfigService> service(DnsConfigService::CreateSystemService()); diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc index f294b8c..35ea2fa 100644 --- a/net/dns/host_resolver_impl.cc +++ b/net/dns/host_resolver_impl.cc @@ -2289,7 +2289,15 @@ void HostResolverImpl::OnIPAddressChanged() { // |this| may be deleted inside AbortAllInProgressJobs(). } +void HostResolverImpl::OnInitialDNSConfigRead() { + UpdateDNSConfig(false); +} + void HostResolverImpl::OnDNSChanged() { + UpdateDNSConfig(true); +} + +void HostResolverImpl::UpdateDNSConfig(bool config_changed) { DnsConfig dns_config; NetworkChangeNotifier::GetDnsConfig(&dns_config); @@ -2310,27 +2318,33 @@ void HostResolverImpl::OnDNSChanged() { // the newly started jobs use the new config. if (dns_client_.get()) { dns_client_->SetConfig(dns_config); - if (dns_client_->GetConfig()) + if (dns_client_->GetConfig()) { UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true); + // If we just switched DnsClients, restart jobs using new resolver. + // TODO(pauljensen): Is this necessary? + config_changed = true; + } } - // If the DNS server has changed, existing cached info could be wrong so we - // have to drop our internal cache :( Note that OS level DNS caches, such - // as NSCD's cache should be dropped automatically by the OS when - // resolv.conf changes so we don't need to do anything to clear that cache. - if (cache_.get()) - cache_->clear(); + if (config_changed) { + // If the DNS server has changed, existing cached info could be wrong so we + // have to drop our internal cache :( Note that OS level DNS caches, such + // as NSCD's cache should be dropped automatically by the OS when + // resolv.conf changes so we don't need to do anything to clear that cache. + if (cache_.get()) + cache_->clear(); - // Life check to bail once |this| is deleted. - base::WeakPtr<HostResolverImpl> self = weak_ptr_factory_.GetWeakPtr(); + // Life check to bail once |this| is deleted. + base::WeakPtr<HostResolverImpl> self = weak_ptr_factory_.GetWeakPtr(); - // Existing jobs will have been sent to the original server so they need to - // be aborted. - AbortAllInProgressJobs(); + // Existing jobs will have been sent to the original server so they need to + // be aborted. + AbortAllInProgressJobs(); - // |this| may be deleted inside AbortAllInProgressJobs(). - if (self.get()) - TryServingAllJobsFromHosts(); + // |this| may be deleted inside AbortAllInProgressJobs(). + if (self.get()) + TryServingAllJobsFromHosts(); + } } bool HostResolverImpl::HaveDnsConfig() const { diff --git a/net/dns/host_resolver_impl.h b/net/dns/host_resolver_impl.h index f2380f5f..8090742 100644 --- a/net/dns/host_resolver_impl.h +++ b/net/dns/host_resolver_impl.h @@ -228,6 +228,9 @@ class NET_EXPORT HostResolverImpl // NetworkChangeNotifier::DNSObserver: void OnDNSChanged() override; + void OnInitialDNSConfigRead() override; + + void UpdateDNSConfig(bool config_changed); // True if have a DnsClient with a valid DnsConfig. bool HaveDnsConfig() const; diff --git a/net/dns/host_resolver_impl_unittest.cc b/net/dns/host_resolver_impl_unittest.cc index d71db2f..9a886a7 100644 --- a/net/dns/host_resolver_impl_unittest.cc +++ b/net/dns/host_resolver_impl_unittest.cc @@ -899,7 +899,8 @@ TEST_F(HostResolverImplTest, BypassCache) { EXPECT_EQ(2u, proc_->GetCaptureList().size()); } -// Test that IP address changes flush the cache. +// Test that IP address changes flush the cache but initial DNS config reads do +// not. TEST_F(HostResolverImplTest, FlushCacheOnIPAddressChange) { proc_->SignalMultiple(2u); // One before the flush, one after. @@ -910,6 +911,11 @@ TEST_F(HostResolverImplTest, FlushCacheOnIPAddressChange) { req = CreateRequest("host1", 75); EXPECT_EQ(OK, req->Resolve()); // Should complete synchronously. + // Verify initial DNS config read does not flush cache. + NetworkChangeNotifier::NotifyObserversOfInitialDNSConfigReadForTests(); + req = CreateRequest("host1", 75); + EXPECT_EQ(OK, req->Resolve()); // Should complete synchronously. + // Flush cache by triggering an IP address change. NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests(); base::MessageLoop::current()->RunUntilIdle(); // Notification happens async. @@ -936,6 +942,20 @@ TEST_F(HostResolverImplTest, AbortOnIPAddressChanged) { EXPECT_EQ(0u, resolver_->GetHostCache()->size()); } +// Test that initial DNS config read signals do not abort pending requests. +TEST_F(HostResolverImplTest, DontAbortOnInitialDNSConfigRead) { + Request* req = CreateRequest("host1", 70); + EXPECT_EQ(ERR_IO_PENDING, req->Resolve()); + + EXPECT_TRUE(proc_->WaitFor(1u)); + // Triggering initial DNS config read signal. + NetworkChangeNotifier::NotifyObserversOfInitialDNSConfigReadForTests(); + base::MessageLoop::current()->RunUntilIdle(); // Notification happens async. + proc_->SignalAll(); + + EXPECT_EQ(OK, req->WaitForResult()); +} + // Obey pool constraints after IP address has changed. TEST_F(HostResolverImplTest, ObeyPoolConstraintsAfterIPAddressChange) { // Runs at most one job at a time. diff --git a/net/net.gyp b/net/net.gyp index 71058af..aac3855 100644 --- a/net/net.gyp +++ b/net/net.gyp @@ -170,9 +170,6 @@ 'sources!': [ # See bug http://crbug.com/344533. 'disk_cache/blockfile/index_table_v3_unittest.cc', - # No res_ninit() et al on Android, so this doesn't make a lot of - # sense. - 'dns/dns_config_service_posix_unittest.cc', ], 'dependencies': [ 'net_javatests', @@ -401,12 +398,6 @@ 'disk_cache/blockfile/index_table_v3_unittest.cc', ], }], - [ 'OS == "android"', { - 'sources!': [ - 'dns/dns_config_service_posix_unittest.cc', - ], - }, - ], ['OS == "android"', { # TODO(mmenke): This depends on test_support_base, which depends on # icu. Figure out a way to remove that dependency. diff --git a/net/tools/net_watcher/net_watcher.cc b/net/tools/net_watcher/net_watcher.cc index b6cf393..1643bc4 100644 --- a/net/tools/net_watcher/net_watcher.cc +++ b/net/tools/net_watcher/net_watcher.cc @@ -102,6 +102,9 @@ class NetWatcher : // net::NetworkChangeNotifier::DNSObserver implementation. void OnDNSChanged() override { LOG(INFO) << "OnDNSChanged()"; } + void OnInitialDNSConfigRead() override { + LOG(INFO) << "OnInitialDNSConfigRead()"; + } // net::NetworkChangeNotifier::NetworkChangeObserver implementation. void OnNetworkChanged( |