Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS lookup should request then wait for IPv4 and IPv6 addresses #14413

Conversation

achristensen07
Copy link
Contributor

@achristensen07 achristensen07 commented May 27, 2023

5aa25a1

DNS lookup should request then wait for IPv4 and IPv6 addresses
https://bugs.webkit.org/show_bug.cgi?id=257404
rdar://109911384

Reviewed by Youenn Fablet.

As advised by one who knows DNS better than I do, with the code I wrote earlier today
in #14326 I am missing some of the IP addresses.

To receive a set of IP addresses equivalent to what CFHost received, I need to add
the kDNSServiceFlagsReturnIntermediates flag.

Also, there are some times when we receive IPv4 addresses then get a callback with
kDNSServiceFlagsMoreComing and then sometime later receive IPv6 addresses, or vice versa.
The addresses from the second set of callbacks were being dropped.  To fix this,
I explicitly add the protocols kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6
to request callbacks for both protocols.  This makes it so that sometimes I get
callbacks with kDNSServiceErr_NoSuchRecord and callbacks with IP addresses that are all
zeros.  We simiply ignore kDNSServiceErr_NoSuchRecord and filter out all the all-zero
IP addresses.  That way the result of DNSResolveQueueCFNet::performDNSLookup always
contains all the IP addresses.

I manually verified that IP address lists were incomplete sometimes before this change,
and now they are complete all the time.  There are also two unit tests that exercise
this code path, webrtc/datachannel/mdns-ice-candidates.html and TestWebKitAPI.WebKit2.RTCDataChannelPostMessage
and I verified that they both pass.

* Source/WebCore/platform/network/DNS.cpp:
(WebCore::IPAddress::isAllZeros const):
* Source/WebCore/platform/network/DNS.h:
* Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.cpp:
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::complete):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::addIPAddress):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::shouldWaitForMoreAddresses const):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::filterZeroAddresses):
(WebCore::dnsLookupCallback):
(WebCore::DNSResolveQueueCFNet::performDNSLookup):

Canonical link: https://commits.webkit.org/264655@main

f4b8be8

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2   πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@achristensen07 achristensen07 self-assigned this May 27, 2023
@achristensen07 achristensen07 added the WebKit Process Model Bugs related to WebKit's multi-process architecture label May 27, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 27, 2023
@achristensen07 achristensen07 force-pushed the eng/DNS-lookup-should-request-then-wait-for-IPv4-and-IPv6-addresses branch from ffd7ca9 to 383c541 Compare May 27, 2023 02:43
void addIPAddress(IPAddress&& address) { m_addresses.append(WTFMove(address)); }
void addIPAddress(IPAddress&& address)
{
m_addedIPv4 |= address.isIPv4();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more m_hasReceivedIPv4

@achristensen07 achristensen07 force-pushed the eng/DNS-lookup-should-request-then-wait-for-IPv4-and-IPv6-addresses branch from 383c541 to f4b8be8 Compare May 29, 2023 15:39
@achristensen07 achristensen07 added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels May 29, 2023
https://bugs.webkit.org/show_bug.cgi?id=257404
rdar://109911384

Reviewed by Youenn Fablet.

As advised by one who knows DNS better than I do, with the code I wrote earlier today
in WebKit#14326 I am missing some of the IP addresses.

To receive a set of IP addresses equivalent to what CFHost received, I need to add
the kDNSServiceFlagsReturnIntermediates flag.

Also, there are some times when we receive IPv4 addresses then get a callback with
kDNSServiceFlagsMoreComing and then sometime later receive IPv6 addresses, or vice versa.
The addresses from the second set of callbacks were being dropped.  To fix this,
I explicitly add the protocols kDNSServiceProtocol_IPv4 | kDNSServiceProtocol_IPv6
to request callbacks for both protocols.  This makes it so that sometimes I get
callbacks with kDNSServiceErr_NoSuchRecord and callbacks with IP addresses that are all
zeros.  We simiply ignore kDNSServiceErr_NoSuchRecord and filter out all the all-zero
IP addresses.  That way the result of DNSResolveQueueCFNet::performDNSLookup always
contains all the IP addresses.

I manually verified that IP address lists were incomplete sometimes before this change,
and now they are complete all the time.  There are also two unit tests that exercise
this code path, webrtc/datachannel/mdns-ice-candidates.html and TestWebKitAPI.WebKit2.RTCDataChannelPostMessage
and I verified that they both pass.

* Source/WebCore/platform/network/DNS.cpp:
(WebCore::IPAddress::isAllZeros const):
* Source/WebCore/platform/network/DNS.h:
* Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.cpp:
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::complete):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::addIPAddress):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::shouldWaitForMoreAddresses const):
(WebCore::DNSResolveQueueCFNet::CompletionHandlerWrapper::filterZeroAddresses):
(WebCore::dnsLookupCallback):
(WebCore::DNSResolveQueueCFNet::performDNSLookup):

Canonical link: https://commits.webkit.org/264655@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/DNS-lookup-should-request-then-wait-for-IPv4-and-IPv6-addresses branch from f4b8be8 to 5aa25a1 Compare May 29, 2023 15:51
@webkit-commit-queue
Copy link
Collaborator

Committed 264655@main (5aa25a1): https://commits.webkit.org/264655@main

Reviewed commits have been landed. Closing PR #14413 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 5aa25a1 into WebKit:main May 29, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
5 participants