Bug 50996 – Consider disabling DNS prefetch when proxy is used

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

DNS prefetching increases load on proxies, confuses certain enterprise security systems, and possibly even causes pages load failures due to overwhelming proxies.

Benefit from DNS prefetch is limited with SOCKSv5 or HTTP proxies, because WebKit will talk to a proxy server, and not to the actual host (I believe that names are still resolved on the client with SOCKSv4). When inside a corporate network, DNS may well return incorrect results, poisoning client DNS cache due to DNS prefetch.

See also: <https://bugzilla.mozilla.org/show_bug.cgi?id=507578>.

<rdar://problem/8098862>

[Alexey Proskuryakov]

Created attachment 76471 [details]
proposed patch

I'm not sure if this can affect page load speed.

[Darin Adler]

Comment on attachment 76471 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76471&action=review

The comments and function names make it seem like this function can detect all proxies. But there is a whole class of “transparent proxies” that will not be detected. The function names could be more precise.

While I’m saying review+ here, I think you need to do something to be sure this does not slow down web browsing. So you could also consider it a review-.

> WebCore/platform/network/cf/DNSCFNet.cpp:166
> +    // The proxy is enabled if the key is present and the associated value is nonzero.

Don’t you need to use CFNumberGetValue instead? Is comparing a CFNumber of 0 a good enough way to check? CFNumberGetValue does some conversion.

> WebCore/platform/network/cf/DNSCFNet.cpp:176
> +    RetainPtr<CFDictionaryRef> proxySettings(AdoptCF, CFNetworkCopySystemProxySettings());

It seems to me that we should not be calling CFNetworkCopySystemProxySettings so many times. Calling it every single time we consider prefetching the hostname seems likely to be slow. We should either prove it’s not slow or do something to make sure we’re not affected.

It’s ironic to cache the CFNumber of 0, but not the result here.

One way to do this is to subscribe to the “network configuration changed” notification and clear the “proxy is enabled” cache only when we get that notification.

> WebCore/platform/network/cf/DNSCFNet.cpp:198
> +    // Don't do DNS prefetch is proxies are involved. For many proxy types, the user agent is never exposed
> +    // to the IP address during normal operation, and the address returned by an internal DNS server
> +    // may be incorrect.

This comment does not seem explicit enough. You say that the address returned by the DNS server may be incorrect, but that doesn’t state why that means that prefetch is therefore not helpful. The reader is left to “connect the dots”. I suggest you state the reason for the policy a little more explicitly.

[Alexey Proskuryakov]

> One way to do this is to subscribe to the “network configuration changed” notification

I couldn't find one that would be available on Windows - do you know of such?

[Darin Adler]

(In reply to comment #3)
> > One way to do this is to subscribe to the “network configuration changed” notification
> 
> I couldn't find one that would be available on Windows - do you know of such?

No, I overlooked Windows.

[Alexey Proskuryakov]

Created attachment 76700 [details]
updated patch

> Don’t you need to use CFNumberGetValue instead? Is comparing a CFNumber of 0 a good enough way to check? CFNumberGetValue does some conversion.

When checking that, I came to the conclusion that we can't second-guess CFNetwork here. The logic is much more complicated - for example, an *Enabled key can be missing altogether if there are keys for port and host.

So, I changed the code to use a CFNetwork API.

> We should either prove it’s not slow or do something to make sure we’re not affected.

This worries me, too.

Shark sees the proxyIsEnabledInSystemPreferences(), but on one sample out of 6500. I've also made the check happen later, as the queue coalesces identical requests.

[Darin Adler]

Comment on attachment 76700 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76700&action=review

> WebCore/platform/network/cf/DNSCFNet.cpp:89
> +    RetainPtr<CFArrayRef> httpProxyArray = CFNetworkCopyProxiesForURL(httpCFURL, proxySettings.get());
> +    RetainPtr<CFArrayRef> httpsProxyArray = CFNetworkCopyProxiesForURL(httpsCFURL, proxySettings.get());

You need to adopt here since it’s a “copy”-style function.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/74197>.

> You need to adopt here since it’s a “copy”-style function.

Oops!

[WebKit Review Bot]

http://trac.webkit.org/changeset/74197 might have broken Leopard Intel Debug (Tests)