Skip to content

Commit

Permalink
Make DNS notifications to proxy code much less expensive
Browse files Browse the repository at this point in the history 
Instead of completely resetting fetched proxy configuration and blocking
all URL requests until after a 2 second delay and a new config is
retrieved, just signal the poller that it is a good time to recheck
proxy autoconfiguration.

This high expense of DNS notifications was especially noticeable as DoH
usage became more common because DoH causes additional DNS notifications
on the first DoH server becoming available (often 5 seconds after Chrome
startup when the DoH activation probes wake up) or on the last DoH
server becoming unavailable (after multiple consecutive errors with the
server).

Bug: 1176970
Change-Id: I6773818c619ad7f4de4397c9c0abebac1d52de08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2703095
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#856985}
  • Loading branch information
Eric Orth authored and Chromium LUCI CQ committed Feb 24, 2021
1 parent 0d5c475 commit 94251c5
Show file tree
Hide file tree
Showing 2 changed files with 196 additions and 3 deletions.
19 changes: 17 additions & 2 deletions net/proxy_resolution/configured_proxy_resolution_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1185,8 +1185,9 @@ void ConfiguredProxyResolutionService::ReportSuccess(const ProxyInfo& result) {
const ProxyRetryInfo& proxy_retry_info = iter.second;
proxy_delegate_->OnFallback(bad_proxy, proxy_retry_info.net_error);
}
} else if (existing->second.bad_until < iter.second.bad_until)
} else if (existing->second.bad_until < iter.second.bad_until) {
existing->second.bad_until = iter.second.bad_until;
}
}
if (net_log_) {
net_log_->AddGlobalEntry(NetLogEventType::BAD_PROXY_LIST_REPORTED, [&] {
Expand Down Expand Up @@ -1562,13 +1563,27 @@ void ConfiguredProxyResolutionService::OnIPAddressChanged() {
stall_proxy_autoconfig_until_ =
TimeTicks::Now() + stall_proxy_auto_config_delay_;

// With a new network connection, using the proper proxy configuration for the
// new connection may be essential for URL requests to work properly. Reset
// the config to ensure new URL requests are blocked until the potential new
// proxy configuration is loaded.
State previous_state = ResetProxyConfig(false);
if (previous_state != STATE_NONE)
ApplyProxyConfigIfAvailable();
}

void ConfiguredProxyResolutionService::OnDNSChanged() {
OnIPAddressChanged();
// Do not fully reset proxy config on DNS change notifications. Instead,
// inform the poller that it would be a good time to check for changes.
//
// While a change to DNS servers in use could lead to different WPAD results,
// and thus a different proxy configuration, it is extremely unlikely to ever
// be essential for that changed proxy configuration to be picked up
// immediately. Either URL requests on the connection are generally working
// fine without the proxy, or requests are already broken, leaving little harm
// in letting a couple more requests fail until Chrome picks up the new proxy.
if (script_poller_.get())
script_poller_->OnLazyPoll();
}

} // namespace net
180 changes: 179 additions & 1 deletion net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
#include "net/proxy_resolution/configured_proxy_resolution_service.h"

#include <cstdarg>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "base/bind.h"
Expand All @@ -17,7 +19,9 @@
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/metrics/histogram_tester.h"
#include "net/base/mock_network_change_notifier.h"
#include "net/base/net_errors.h"
#include "net/base/network_change_notifier.h"
#include "net/base/network_isolation_key.h"
#include "net/base/proxy_delegate.h"
#include "net/base/proxy_server.h"
Expand Down Expand Up @@ -119,8 +123,13 @@ class ImmediateAfterActivityPollPolicy
//
// The tests which verify the polling code re-enable the polling behavior but
// are careful to avoid timing problems.
class ConfiguredProxyResolutionServiceTest : public TestWithTaskEnvironment {
class ConfiguredProxyResolutionServiceTest : public ::testing::Test,
public WithTaskEnvironment {
protected:
ConfiguredProxyResolutionServiceTest()
: WithTaskEnvironment(
base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}

void SetUp() override {
testing::Test::SetUp();
previous_policy_ =
Expand Down Expand Up @@ -3733,6 +3742,175 @@ TEST_F(ConfiguredProxyResolutionServiceTest, PACScriptRefetchAfterActivity) {
EXPECT_TRUE(info3.is_direct());
}

TEST_F(ConfiguredProxyResolutionServiceTest, IpAddressChangeResetsProxy) {
NeverPollPolicy poll_policy;
ConfiguredProxyResolutionService::set_pac_script_poll_policy(&poll_policy);

MockAsyncProxyResolver resolver;
auto factory = std::make_unique<MockAsyncProxyResolverFactory>(
/*resolvers_expect_pac_bytes=*/true);
MockAsyncProxyResolverFactory* factory_ptr = factory.get();
ConfiguredProxyResolutionService service(
std::make_unique<MockProxyConfigService>(ProxyConfig::CreateAutoDetect()),
std::move(factory),
/*net_log=*/nullptr, /*quick_check_enabled=*/true);
auto fetcher = std::make_unique<MockPacFileFetcher>();
MockPacFileFetcher* fetcher_ptr = fetcher.get();
service.SetPacFileFetchers(std::move(fetcher),
std::make_unique<DoNothingDhcpPacFileFetcher>());

const base::TimeDelta kConfigDelay = base::TimeDelta::FromSeconds(5);
service.set_stall_proxy_auto_config_delay(kConfigDelay);

// Initialize by making and completing a proxy request.
ProxyInfo info1;
TestCompletionCallback callback1;
std::unique_ptr<ProxyResolutionRequest> request1;
int rv = service.ResolveProxy(
GURL("http://request1"), std::string(), NetworkIsolationKey(), &info1,
callback1.callback(), &request1, NetLogWithSource());
ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
ASSERT_TRUE(fetcher_ptr->has_pending_request());
fetcher_ptr->NotifyFetchCompletion(OK, kValidPacScript1);
ASSERT_THAT(factory_ptr->pending_requests(), testing::SizeIs(1));
EXPECT_EQ(ASCIIToUTF16(kValidPacScript1),
factory_ptr->pending_requests()[0]->script_data()->utf16());
factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(OK, &resolver);
ASSERT_THAT(resolver.pending_jobs(), testing::SizeIs(1));
resolver.pending_jobs()[0]->CompleteNow(OK);
ASSERT_THAT(callback1.WaitForResult(), IsOk());
EXPECT_FALSE(fetcher_ptr->has_pending_request());

// Expect IP address notification to trigger a fetch after wait period.
NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
FastForwardBy(kConfigDelay - base::TimeDelta::FromMilliseconds(2));
EXPECT_FALSE(fetcher_ptr->has_pending_request());
FastForwardBy(base::TimeDelta::FromMilliseconds(2));
EXPECT_TRUE(fetcher_ptr->has_pending_request());

// Leave pending fetch hanging.

// Expect proxy requests are blocked on completion of change-triggered fetch.
ProxyInfo info2;
TestCompletionCallback callback2;
std::unique_ptr<ProxyResolutionRequest> request2;
rv = service.ResolveProxy(GURL("http://request1"), std::string(),
NetworkIsolationKey(), &info2, callback2.callback(),
&request2, NetLogWithSource());
EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
EXPECT_THAT(resolver.pending_jobs(), testing::IsEmpty());

// Finish pending fetch and expect proxy request to be able to complete.
fetcher_ptr->NotifyFetchCompletion(OK, kValidPacScript2);
ASSERT_THAT(factory_ptr->pending_requests(), testing::SizeIs(1));
EXPECT_EQ(ASCIIToUTF16(kValidPacScript2),
factory_ptr->pending_requests()[0]->script_data()->utf16());
factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(OK, &resolver);
ASSERT_THAT(resolver.pending_jobs(), testing::SizeIs(1));
resolver.pending_jobs()[0]->CompleteNow(OK);
EXPECT_THAT(callback2.WaitForResult(), IsOk());
EXPECT_FALSE(fetcher_ptr->has_pending_request());
}

TEST_F(ConfiguredProxyResolutionServiceTest, DnsChangeTriggersPoll) {
ImmediateAfterActivityPollPolicy poll_policy;
ConfiguredProxyResolutionService::set_pac_script_poll_policy(&poll_policy);

MockAsyncProxyResolver resolver;
auto factory = std::make_unique<MockAsyncProxyResolverFactory>(
/*resolvers_expect_pac_bytes=*/true);
MockAsyncProxyResolverFactory* factory_ptr = factory.get();
ConfiguredProxyResolutionService service(
std::make_unique<MockProxyConfigService>(ProxyConfig::CreateAutoDetect()),
std::move(factory),
/*net_log=*/nullptr, /*quick_check_enabled=*/true);
auto fetcher = std::make_unique<MockPacFileFetcher>();
MockPacFileFetcher* fetcher_ptr = fetcher.get();
service.SetPacFileFetchers(std::move(fetcher),
std::make_unique<DoNothingDhcpPacFileFetcher>());

// Initialize config and poller by making and completing a proxy request.
ProxyInfo info1;
TestCompletionCallback callback1;
std::unique_ptr<ProxyResolutionRequest> request1;
int rv = service.ResolveProxy(
GURL("http://request1"), std::string(), NetworkIsolationKey(), &info1,
callback1.callback(), &request1, NetLogWithSource());
ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
ASSERT_TRUE(fetcher_ptr->has_pending_request());
fetcher_ptr->NotifyFetchCompletion(OK, kValidPacScript1);
ASSERT_THAT(factory_ptr->pending_requests(), testing::SizeIs(1));
EXPECT_EQ(ASCIIToUTF16(kValidPacScript1),
factory_ptr->pending_requests()[0]->script_data()->utf16());
factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(OK, &resolver);
ASSERT_THAT(resolver.pending_jobs(), testing::SizeIs(1));
resolver.pending_jobs()[0]->CompleteNow(OK);
ASSERT_THAT(callback1.WaitForResult(), IsOk());
EXPECT_FALSE(fetcher_ptr->has_pending_request());

// Expect DNS notification to trigger a fetch.
NetworkChangeNotifier::NotifyObserversOfDNSChangeForTests();
fetcher_ptr->WaitUntilFetch();
EXPECT_TRUE(fetcher_ptr->has_pending_request());

// Leave pending fetch hanging.

// Expect proxy requests are not blocked on completion of DNS-triggered fetch.
ProxyInfo info2;
TestCompletionCallback callback2;
std::unique_ptr<ProxyResolutionRequest> request2;
rv = service.ResolveProxy(GURL("http://request2"), std::string(),
NetworkIsolationKey(), &info2, callback2.callback(),
&request2, NetLogWithSource());
EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
ASSERT_THAT(resolver.pending_jobs(), testing::SizeIs(1));
resolver.pending_jobs()[0]->CompleteNow(OK);
EXPECT_THAT(callback2.WaitForResult(), IsOk());

// Complete DNS-triggered fetch.
fetcher_ptr->NotifyFetchCompletion(OK, kValidPacScript2);
RunUntilIdle();

// Expect further proxy requests to use the new fetch result.
ProxyInfo info3;
TestCompletionCallback callback3;
std::unique_ptr<ProxyResolutionRequest> request3;
rv = service.ResolveProxy(GURL("http://request3"), std::string(),
NetworkIsolationKey(), &info3, callback3.callback(),
&request3, NetLogWithSource());
ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
ASSERT_THAT(factory_ptr->pending_requests(), testing::SizeIs(1));
EXPECT_EQ(ASCIIToUTF16(kValidPacScript2),
factory_ptr->pending_requests()[0]->script_data()->utf16());
factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(OK, &resolver);
ASSERT_THAT(resolver.pending_jobs(), testing::SizeIs(1));
resolver.pending_jobs()[0]->CompleteNow(OK);
ASSERT_THAT(callback3.WaitForResult(), IsOk());
EXPECT_FALSE(fetcher_ptr->has_pending_request());
}

TEST_F(ConfiguredProxyResolutionServiceTest, DnsChangeNoopWithoutResolver) {
ImmediateAfterActivityPollPolicy poll_policy;
ConfiguredProxyResolutionService::set_pac_script_poll_policy(&poll_policy);

MockAsyncProxyResolver resolver;
ConfiguredProxyResolutionService service(
std::make_unique<MockProxyConfigService>(ProxyConfig::CreateAutoDetect()),
std::make_unique<MockAsyncProxyResolverFactory>(
/*resolvers_expect_pac_bytes=*/true),
/*net_log=*/nullptr, /*quick_check_enabled=*/true);
auto fetcher = std::make_unique<MockPacFileFetcher>();
MockPacFileFetcher* fetcher_ptr = fetcher.get();
service.SetPacFileFetchers(std::move(fetcher),
std::make_unique<DoNothingDhcpPacFileFetcher>());

// Expect DNS notification to do nothing because no proxy requests have yet
// been made.
NetworkChangeNotifier::NotifyObserversOfDNSChangeForTests();
RunUntilIdle();
EXPECT_FALSE(fetcher_ptr->has_pending_request());
}

// Helper class to exercise URL sanitization by submitting URLs to the
// ConfiguredProxyResolutionService and returning the URL passed to the
// ProxyResolver.
Expand Down

0 comments on commit 94251c5

Please sign in to comment.