[webkit-reviews] review granted: [Bug 50996] Consider disabling DNS prefetch when proxy is used : [Attachment 76471] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 50996: Consider disabling DNS prefetch when proxy is used
https://bugs.webkit.org/show_bug.cgi?id=50996

Attachment 76471: proposed patch
https://bugs.webkit.org/attachment.cgi?id=76471&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.