Bug 178855 - Make NetworkRTCResolver port agnostic
Summary: Make NetworkRTCResolver port agnostic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords: InRadar
Depends on:
Blocks: 178860
  Show dependency treegraph
 
Reported: 2017-10-26 03:28 PDT by Alejandro G. Castro
Modified: 2018-03-07 08:14 PST (History)
4 users (show)

See Also:


Attachments
Patch (18.45 KB, patch)
2017-10-26 03:41 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (17.94 KB, patch)
2017-10-26 04:08 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (18.00 KB, patch)
2017-10-26 04:18 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (18.04 KB, patch)
2017-10-26 04:36 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (18.13 KB, patch)
2017-10-26 04:46 PDT, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (33.32 KB, patch)
2018-02-08 05:39 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (33.36 KB, patch)
2018-02-08 06:02 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (33.35 KB, patch)
2018-02-08 06:16 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (37.66 KB, patch)
2018-02-08 06:52 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (37.78 KB, patch)
2018-02-09 01:16 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (42.69 KB, patch)
2018-02-09 01:42 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (41.89 KB, patch)
2018-02-09 01:47 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (41.89 KB, patch)
2018-02-09 02:03 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (41.90 KB, patch)
2018-02-09 02:21 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (41.97 KB, patch)
2018-02-09 02:39 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.08 KB, patch)
2018-02-09 06:36 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.11 KB, patch)
2018-02-09 06:58 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.11 KB, patch)
2018-02-09 07:01 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.18 KB, patch)
2018-02-12 04:07 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.21 KB, patch)
2018-02-12 05:26 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.21 KB, patch)
2018-02-12 05:47 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.26 KB, patch)
2018-02-12 05:55 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.39 KB, patch)
2018-02-12 06:06 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.39 KB, patch)
2018-02-12 06:29 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.50 KB, patch)
2018-02-13 01:18 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.37 KB, patch)
2018-02-13 01:40 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.37 KB, patch)
2018-02-13 03:51 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.37 KB, patch)
2018-02-13 04:11 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (73.37 KB, patch)
2018-02-13 06:24 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (78.95 KB, patch)
2018-02-19 03:44 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (78.94 KB, patch)
2018-02-19 04:10 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (78.98 KB, patch)
2018-02-19 04:29 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (78.98 KB, patch)
2018-02-19 04:54 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (79.25 KB, patch)
2018-02-19 05:39 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (79.25 KB, patch)
2018-02-19 05:48 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (79.50 KB, patch)
2018-02-19 06:12 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.66 KB, patch)
2018-03-06 06:03 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.06 KB, patch)
2018-03-06 06:12 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.04 KB, patch)
2018-03-06 06:27 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.19 KB, patch)
2018-03-06 06:29 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.56 KB, patch)
2018-03-06 06:58 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.57 KB, patch)
2018-03-06 07:05 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.64 KB, patch)
2018-03-07 05:37 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (81.64 KB, patch)
2018-03-07 05:55 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch for landing (81.64 KB, patch)
2018-03-07 06:41 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2017-10-26 03:28:17 PDT
We need these changes to use it in non cocoa ports.
Comment 1 Alejandro G. Castro 2017-10-26 03:41:54 PDT
Created attachment 324982 [details]
Patch
Comment 2 Alejandro G. Castro 2017-10-26 04:08:24 PDT
Created attachment 324984 [details]
Patch
Comment 3 Alejandro G. Castro 2017-10-26 04:18:55 PDT
Created attachment 324986 [details]
Patch
Comment 4 Alejandro G. Castro 2017-10-26 04:36:31 PDT
Created attachment 324988 [details]
Patch
Comment 5 Alejandro G. Castro 2017-10-26 04:46:03 PDT
Created attachment 324989 [details]
Patch
Comment 6 Sam Weinig 2017-10-26 06:07:04 PDT
The normal pattern we use for platform differences is to create a platform abstraction (in this case, maybe a DNSResolver or HostResolver class) in WebCore/platform and then use that. 

In this case, we have the beginning of an abstraction in WebCore already for DNS prefetching, DNSResolveQueue, that perhaps we could add to.
Comment 7 Alejandro G. Castro 2017-10-26 07:00:32 PDT
(In reply to Sam Weinig from comment #6)
> The normal pattern we use for platform differences is to create a platform
> abstraction (in this case, maybe a DNSResolver or HostResolver class) in
> WebCore/platform and then use that. 
> 
> In this case, we have the beginning of an abstraction in WebCore already for
> DNS prefetching, DNSResolveQueue, that perhaps we could add to.

If reusing that class is an option that would be great for us, it was actually my plan to try to use the DNSSoup implementation in our port. If that is an option in your case too we can refactor this code.
Comment 8 Alejandro G. Castro 2018-02-08 05:39:39 PST
Created attachment 333372 [details]
Patch
Comment 9 Alejandro G. Castro 2018-02-08 05:42:40 PST
After talking to youenn about the best approach we have implemented a solution using the DNSResolveQueue that can be used by other ports to move the code platform dependent code to that class in the platform directory.
Comment 10 Alejandro G. Castro 2018-02-08 06:02:12 PST
Created attachment 333374 [details]
Patch
Comment 11 Alejandro G. Castro 2018-02-08 06:16:40 PST
Created attachment 333375 [details]
Patch
Comment 12 Alejandro G. Castro 2018-02-08 06:52:05 PST
Created attachment 333376 [details]
Patch
Comment 13 Alejandro G. Castro 2018-02-09 01:16:08 PST
Created attachment 333462 [details]
Patch
Comment 14 Alejandro G. Castro 2018-02-09 01:42:46 PST
Created attachment 333465 [details]
Patch
Comment 15 Alejandro G. Castro 2018-02-09 01:47:40 PST
Created attachment 333466 [details]
Patch
Comment 16 Alejandro G. Castro 2018-02-09 02:03:19 PST
Created attachment 333471 [details]
Patch
Comment 17 Alejandro G. Castro 2018-02-09 02:21:23 PST
Created attachment 333472 [details]
Patch
Comment 18 Alejandro G. Castro 2018-02-09 02:39:20 PST
Created attachment 333473 [details]
Patch
Comment 19 Alejandro G. Castro 2018-02-09 06:36:29 PST
Created attachment 333482 [details]
Patch
Comment 20 Alejandro G. Castro 2018-02-09 06:58:14 PST
Created attachment 333483 [details]
Patch
Comment 21 Alejandro G. Castro 2018-02-09 07:01:47 PST
Created attachment 333484 [details]
Patch
Comment 22 Alejandro G. Castro 2018-02-12 04:07:10 PST
Created attachment 333588 [details]
Patch
Comment 23 Alejandro G. Castro 2018-02-12 05:26:04 PST
Created attachment 333590 [details]
Patch
Comment 24 Alejandro G. Castro 2018-02-12 05:47:53 PST
Created attachment 333591 [details]
Patch
Comment 25 Alejandro G. Castro 2018-02-12 05:55:52 PST
Created attachment 333592 [details]
Patch
Comment 26 Alejandro G. Castro 2018-02-12 06:06:46 PST
Created attachment 333593 [details]
Patch
Comment 27 Alejandro G. Castro 2018-02-12 06:29:13 PST
Created attachment 333594 [details]
Patch
Comment 28 Alejandro G. Castro 2018-02-13 01:18:26 PST
Created attachment 333676 [details]
Patch
Comment 29 Alejandro G. Castro 2018-02-13 01:40:31 PST
Created attachment 333678 [details]
Patch
Comment 30 Alejandro G. Castro 2018-02-13 03:51:54 PST
Created attachment 333680 [details]
Patch
Comment 31 Alejandro G. Castro 2018-02-13 04:11:54 PST
Created attachment 333681 [details]
Patch
Comment 32 Alejandro G. Castro 2018-02-13 06:24:24 PST
Created attachment 333685 [details]
Patch
Comment 33 youenn fablet 2018-02-13 20:09:17 PST
Comment on attachment 333685 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests because this is a refactor.

It would be nice if we could add some Unit tests to the functionalities exposed in DNS.h

> Source/WebCore/ChangeLog:18
> +        * SourcesCocoa.txt: Mov the DNSCFNet class to DNSResolveQueueCFNet.

s/Mov/Move.

> Source/WebKit/ChangeLog:8
> +        Create an specific Cocoa class to isolate the generic code in the base class, make the base implementation port

s/an/a/

> Source/WebCore/platform/network/DNS.h:40
> +    explicit IPAddress(const struct sockaddr_in* address)

This should take a ref instead of a pointer.

> Source/WebCore/platform/network/DNS.h:46
> +    const struct sockaddr_in* get() { return &m_address; };

Return a ref as well.

> Source/WebCore/platform/network/DNS.h:55
> +    virtual void completed(const Vector<IPAddress>&) { };

I guess sometimes we could get an error, which would call for something like ErrorOr<Vector>.
Probably completed could take a Vector<IPAddress>&&.
Instead of DNSResolveObserver, could we take a lambda, something like a Function<void(Vector<IPAddress>&&)>&&?

> Source/WebCore/platform/network/DNSResolveQueue.cpp:49
> +#endif

Instead of all those #ifdef, DNSResolveQueueSoup.h could directly define DNSResolveQueuePlatform.
Ditto for Curl and CFNet.

> Source/WebCore/platform/network/DNSResolveQueue.cpp:80
>      , m_lastProxyEnabledStatusCheckTime(0)

Maybe m_isUsingProxy and m_lastProxyEnabledStatusCheckTime could be initialized in DNSResolveQueue.h

> Source/WebCore/platform/network/DNSResolveQueue.h:49
> +    virtual void stopResolve(DNSResolveObserver*) = 0;

If we would pass a lambda here, resolve could return a uint64_t and stopResolve would take this uint64_t.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:102
> +    addresses.reserveInitialCapacity(1);

I would create addresses after the early return below.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:109
> +    addresses.uncheckedAppend(WebCore::IPAddress(ipaddress));

Might be able to write something like observer->completed(Vector<WebCore::IPAddress>::from(ipaddress))

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:35
> +class DNSResolveQueueSoup : public DNSResolveQueue {

Should be made final.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:39
> +    void stopResolve(DNSResolveObserver*) final;

These two can be made private?

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:42
> +    void updateIsUsingProxy() final;

Why is it protected? Can it be private as well.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:53
> +#endif

Or we could make NetworkRTCResolver.h declare a std::unique_ptr createNetworkRTCResolver(...) which would be defined in COCOA/Soup specific CPP files

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189
> +            addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address.get()->sin_addr)));

You can probably write something like: 
auto addresses = WTF::map(result.value().get(), [] (auto& address) {
    return RTCNetwork::IPAddress { rtc::IPAddress { address.get()->sin_addr } };
});

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:31
> +#include "WebCore/DNS.h"

Should be <WebCore/DNS.h> probably.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:44
> +    explicit NetworkRTCResolver(uint64_t identifier, CompletionHandler&&);

No need for explicit anymore.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:48
> +    virtual void stop();

Should use override.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:35
> +static void resolvedName(CFHostRef hostRef, CFHostInfoType typeInfo, const CFStreamError *error, void *info)

CFStreamError*, void*

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:60
> +    resolver->completed(addresses);

Ditto for WTF::map.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.h:36
> +class NetworkRTCResolverCocoa : public NetworkRTCResolver {

Make class final.
Maybe add a FIXME to remove it once DNSResolver is implemented as well.
Comment 34 Alejandro G. Castro 2018-02-14 03:41:56 PST
Thanks very much for the review!

(In reply to youenn fablet from comment #33)
> Comment on attachment 333685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333685&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests because this is a refactor.
> 
> It would be nice if we could add some Unit tests to the functionalities
> exposed in DNS.h
> 

Right.

> > Source/WebCore/ChangeLog:18
> > +        * SourcesCocoa.txt: Mov the DNSCFNet class to DNSResolveQueueCFNet.
> 
> s/Mov/Move.
> 

Right.

> > Source/WebKit/ChangeLog:8
> > +        Create an specific Cocoa class to isolate the generic code in the base class, make the base implementation port
> 
> s/an/a/
> 

Right.

> > Source/WebCore/platform/network/DNS.h:40
> > +    explicit IPAddress(const struct sockaddr_in* address)
> 
> This should take a ref instead of a pointer.
> 

Right.

> > Source/WebCore/platform/network/DNS.h:46
> > +    const struct sockaddr_in* get() { return &m_address; };
> 
> Return a ref as well.
> 

Right.

> > Source/WebCore/platform/network/DNS.h:55
> > +    virtual void completed(const Vector<IPAddress>&) { };
> 
> I guess sometimes we could get an error, which would call for something like
> ErrorOr<Vector>.
> Probably completed could take a Vector<IPAddress>&&.
> Instead of DNSResolveObserver, could we take a lambda, something like a
> Function<void(Vector<IPAddress>&&)>&&?
> 

That is a good point, NetworkRTCResolver is already doing that ErrorOr<Vector> with a lambda
created in the NetworkRTCProvider, the lambda is stored in the Resolver, that is why I thought to
avoid a bigger refactor in the Cocoa code using the Observer with the lambda and
the identifier is a better option. But I can try that option, that would mean we could remove
NetworkRTCResolver as soon as Cocoa class is removed, maybe I can even do that for Soup
already and add some ifdef in the Provider class with a FIXME. It is a step further but I can
try that.

Regarding the errors basically Resolver sends errors when we destroy the object and the operation
is still active it could do it in other situations I can add API to the
observer to set it (SetError), Cocoa currently has a FIXME saying there is no information and bailing
without calling complete. Now I think there is a bug in the current Cocoa upstream code, it
must call complete with empty list or libwebrtc would not know it is done. I think I'll fix
this in the patch.

Regarding the information libwebrtc just gets boolean for this so just unknown it is ok
in this case but it is true any other code using the API in the future could take advantage
of that information. Honestly I was tempted to do a bigger refactor of the whole API for the
DNS :-) but I'm not sure if there is any other situation other than RTC that will need it.
If you are hace a clearer understanding of the use of this code I can add more features.


> > Source/WebCore/platform/network/DNSResolveQueue.cpp:49
> > +#endif
> 
> Instead of all those #ifdef, DNSResolveQueueSoup.h could directly define
> DNSResolveQueuePlatform.
> Ditto for Curl and CFNet.
> 

Right.

> > Source/WebCore/platform/network/DNSResolveQueue.cpp:80
> >      , m_lastProxyEnabledStatusCheckTime(0)
> If you are hace a clearer understanding of the use of this code I can add more features.
> Maybe m_isUsingProxy and m_lastProxyEnabledStatusCheckTime could be
> initialized in DNSResolveQueue.h
> 

Right.

> > Source/WebCore/platform/network/DNSResolveQueue.h:49
> > +    virtual void stopResolve(DNSResolveObserver*) = 0;
> 
> If we would pass a lambda here, resolve could return a uint64_t and
> stopResolve would take this uint64_t.
> 

Right. I replied about this in a previous comment.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:102
> > +    addresses.reserveInitialCapacity(1);
> 
> I would create addresses after the early return below.

Right.

> 
> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:109
> > +    addresses.uncheckedAppend(WebCore::IPAddress(ipaddress));
> 
> Might be able to write something like
> observer->completed(Vector<WebCore::IPAddress>::from(ipaddress))
> 

Right, I'll try to add that.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:35
> > +class DNSResolveQueueSoup : public DNSResolveQueue {
> 
> Should be made final.
> 

Right.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:39
> > +    void stopResolve(DNSResolveObserver*) final;
> 
> These two can be made private?
> 

I don't think so, they are used in DNS.cpp.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:42
> > +    void updateIsUsingProxy() final;
> 
> Why is it protected? Can it be private as well.
> 

It is used in the base class DNSResolveQueue and implemented in the
platform implementations. Before is was basically just defined per
platform, without inheritance.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:53
> > +#endif
> 
> Or we could make NetworkRTCResolver.h declare a std::unique_ptr
> createNetworkRTCResolver(...) which would be defined in COCOA/Soup specific
> CPP files
> 

If we pass the completionHandler down to the DNS.h API I would basically try
to remove the NetworkRtcResolver all together and make the provider call
directly: resolve and stopResolve. Passing the id and the completion handler.
I can try to do the dance of the Cocoa code in this step if I do that :-),
compilation is always painful but I think I did so much already I can
basically go all the way in the rabbit hole :-).

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:189
> > +            addresses.uncheckedAppend(RTCNetwork::IPAddress(rtc::IPAddress(address.get()->sin_addr)));
> 
> You can probably write something like: 
> auto addresses = WTF::map(result.value().get(), [] (auto& address) {
>     return RTCNetwork::IPAddress { rtc::IPAddress { address.get()->sin_addr
> } };
> });
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:31
> > +#include "WebCore/DNS.h"
> 
> Should be <WebCore/DNS.h> probably.
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:44
> > +    explicit NetworkRTCResolver(uint64_t identifier, CompletionHandler&&);
> 
> No need for explicit anymore.
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:48
> > +    virtual void stop();
> 
> Should use override.
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:35
> > +static void resolvedName(CFHostRef hostRef, CFHostInfoType typeInfo, const CFStreamError *error, void *info)
> 
> CFStreamError*, void*
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:60
> > +    resolver->completed(addresses);
> 
> Ditto for WTF::map.
> 

Right.

> > Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.h:36
> > +class NetworkRTCResolverCocoa : public NetworkRTCResolver {
> 
> Make class final.
> Maybe add a FIXME to remove it once DNSResolver is implemented as well.

Right. I'll try to check if I can remove it already.

Thanks again for the review.
Comment 35 Alejandro G. Castro 2018-02-19 03:44:44 PST
Created attachment 334149 [details]
Patch
Comment 36 Alejandro G. Castro 2018-02-19 04:10:16 PST
Created attachment 334150 [details]
Patch
Comment 37 Alejandro G. Castro 2018-02-19 04:29:38 PST
Created attachment 334152 [details]
Patch
Comment 38 Alejandro G. Castro 2018-02-19 04:38:55 PST
I added most of the comments, just avoided some of the changes in the Cocoa code and leave that for the moment where you can refactor that code because I do not have a working machine with that platform around to do bigger changes. At that point we can even try to get rid of the NetworkRTCResolver and call directly to the DNS.h APIs.
Comment 39 Alejandro G. Castro 2018-02-19 04:54:41 PST
Created attachment 334153 [details]
Patch
Comment 40 Alejandro G. Castro 2018-02-19 05:39:55 PST
Created attachment 334156 [details]
Patch
Comment 41 Alejandro G. Castro 2018-02-19 05:48:50 PST
Created attachment 334157 [details]
Patch
Comment 42 Alejandro G. Castro 2018-02-19 06:12:50 PST
Created attachment 334158 [details]
Patch
Comment 43 youenn fablet 2018-03-05 09:42:31 PST
Comment on attachment 334158 [details]
Patch

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

> Source/WebCore/platform/network/DNS.cpp:44
> +void resolveDNS(const String& hostname, uint64_t identifier, DNSCompletionHandler& completionHandler)

Probably a DNSCompletionHandler&&

> Source/WebCore/platform/network/DNS.cpp:50
> +    WebCore::DNSResolveQueue::singleton().resolve(hostname, identifier, completionHandler);

And WTFMove here.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:97
> +    auto* completionHandler = static_cast<DNSCompletionHandler*>(data);

It seems strange for DNSCompletionHandler to be passed directly here.
Usually when we pass a completion handler, we store it, so maybe DNSResolveQueueSoup could store it directly.
Or we would pass to DNSResolveQueueSoup the object owning the completion handler, which would be NetworkRTCResolver (that is in WebKit namespace now).

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:39
> +    void stopResolve(uint64_t identifier) final;

Can probably be made private at some point.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.h:47
> +    HashMap<uint64_t, GRefPtr<GCancellable>> m_cancelHandlers;

I would tend to augment this map to store pending completion handlers.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:180
> +            });

Remove 4 spaces?

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolver.h:49
> +    WebCore::DNSCompletionHandler m_completionHandler;

If we let the queue store the completion handler, NetworkRTCResolver would just store the identifier.
Comment 44 Alejandro G. Castro 2018-03-06 01:26:20 PST
Thanks for the review! :-) All the changes you propose are because the completion handler is still owned by the NetworkRTCResolver, the reason I did not change this was I had to modify the cocoa code more deeply as I said in comment 38.

My proposal for the final design or the code would be to remove the NetworkRTCResolver object and let the NetworkRTCProvider call directly to the DNS API that stores the handler in the Queue but to do that I had to modify the cocoa code completely, that is why I avoided it and proposed this as a first step, and later when we decide if we want to remove the NetworkRTCResolver we store the handlers in the queue.

My proposal to go ahead is to write the final solution for soup and add an ifdef for cocoa in the RTCPRovider code, that you can remove when you implement it for cocoa. Does that sound good?

Thanks again for the review.

(In reply to youenn fablet from comment #43)
> Comment on attachment 334158 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334158&action=review
> 
> > Source/WebCore/platform/network/DNS.cpp:44
> > +void resolveDNS(const String& hostname, uint64_t identifier, DNSCompletionHandler& completionHandler)
> 
> Probably a DNSCompletionHandler&&
>

...
Comment 45 Alejandro G. Castro 2018-03-06 06:03:07 PST
Created attachment 335087 [details]
Patch
Comment 46 Alejandro G. Castro 2018-03-06 06:12:54 PST
Created attachment 335088 [details]
Patch
Comment 47 Alejandro G. Castro 2018-03-06 06:27:04 PST
Created attachment 335090 [details]
Patch
Comment 48 Alejandro G. Castro 2018-03-06 06:29:05 PST
Created attachment 335091 [details]
Patch
Comment 49 Alejandro G. Castro 2018-03-06 06:58:22 PST
Created attachment 335094 [details]
Patch
Comment 50 Alejandro G. Castro 2018-03-06 07:05:07 PST
Created attachment 335095 [details]
Patch
Comment 51 youenn fablet 2018-03-06 09:29:00 PST
Comment on attachment 335095 [details]
Patch

LGTM!

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

> Source/WebCore/platform/network/DNS.h:40
> +    IPAddress(const struct sockaddr_in& address)

Should be explicit.

> Source/WebCore/platform/network/DNS.h:52
> +enum class DNSError { Unknown, CantResolve, Cancelled };

s/CantResolve/CannotResolve, not exactly sure what this means, is Timeout better?
Unknown means there is an error? Maybe change it to General?

> Source/WebCore/platform/network/DNS.h:54
> +using DNSAddressesOrError = Expected<std::reference_wrapper<Vector<WebCore::IPAddress>>, DNSError>;

I would go with Expected<Vector<WebCore::IPAddress>, DNSError>
We probably create a Vector anyway in our resolution and we can move it to create DNSAddressesOrError.

> Source/WebCore/platform/network/DNS.h:59
> +WEBCORE_EXPORT void stopResolveDNS(uint64_t identifier);

As a side note, we have ObjectIdentifier which allows to type identifiers without any runtime cost.
This allows ensuring we are not mixing identifiers at compile time.
See for instance ServiceWorkerTypes.h for some examples.
Maybe we could use this here as well, as a follow-up for instance.

> Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:36
> +    void stopResolve(uint64_t identifier) final;

Do you need these to be public?

> Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:39
> +    void updateIsUsingProxy() final;

Ditto here.

> Source/WebCore/platform/network/curl/DNSResolveQueueCurl.h:36
> +    void stopResolve(uint64_t identifier) final;

Ditto.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:35
> +#include <wtf/CompletionHandler.h>

Not sure this include is needed.

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:101
> +    auto completionHandler = resolveQueue->getCompletionHandler(identifier);

Maybe there is a way to have takeCompletionHandler() which would allow getting and removing it at the same time?

> Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:147
> +        return nullptr;

I would add ASSERT(completionAndCancelHandlers != m_completionAndCancelHandlers.end()) since there is probably something wrong in that case.
Or maybe add the assert at the call site?
Comment 52 Alejandro G. Castro 2018-03-07 04:11:37 PST
Thanks for the review! :-)

(In reply to youenn fablet from comment #51)
> Comment on attachment 335095 [details]
> Patch
> 
> LGTM!
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335095&action=review
> 
> > Source/WebCore/platform/network/DNS.h:40
> > +    IPAddress(const struct sockaddr_in& address)
> 
> Should be explicit.
> 

Right.

> > Source/WebCore/platform/network/DNS.h:52
> > +enum class DNSError { Unknown, CantResolve, Cancelled };
> 
> s/CantResolve/CannotResolve, not exactly sure what this means, is Timeout
> better?
> Unknown means there is an error? Maybe change it to General?
> 

It is the error we have in soup library SOUP_STATUS_CANT_RESOLVE, the documentation doesn't say much more as usual :-). I've checked the GIO library which is the one handling this and apparently there are 3 situations that are handled with that: the address is not found, network problem or similar, or unknown. It looks pretty general but all of them mean it can not resolve so I think it is not bad option for the name.

> > Source/WebCore/platform/network/DNS.h:54
> > +using DNSAddressesOrError = Expected<std::reference_wrapper<Vector<WebCore::IPAddress>>, DNSError>;
> 
> I would go with Expected<Vector<WebCore::IPAddress>, DNSError>
> We probably create a Vector anyway in our resolution and we can move it to
> create DNSAddressesOrError.
> 

Right, I'll change the Cocoa code accordingly.

> > Source/WebCore/platform/network/DNS.h:59
> > +WEBCORE_EXPORT void stopResolveDNS(uint64_t identifier);
> 
> As a side note, we have ObjectIdentifier which allows to type identifiers
> without any runtime cost.
> This allows ensuring we are not mixing identifiers at compile time.
> See for instance ServiceWorkerTypes.h for some examples.
> Maybe we could use this here as well, as a follow-up for instance.
> 

Sounds interesting option for a follow up refactor, the identifier comes from the WebProcess so probably we would need to modify all the IPC.

> > Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:36
> > +    void stopResolve(uint64_t identifier) final;
> 
> Do you need these to be public?
> 

Yes, this is used to delegate the action for the stopResolveDNS API in DNS.h.

> > Source/WebCore/platform/network/cf/DNSResolveQueueCFNet.h:39
> > +    void updateIsUsingProxy() final;
> 
> Ditto here.
> 

Right, this was was a leftover of one of the refactors.

> > Source/WebCore/platform/network/curl/DNSResolveQueueCurl.h:36
> > +    void stopResolve(uint64_t identifier) final;
> 
> Ditto.
> 

This needs to be public.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:35
> > +#include <wtf/CompletionHandler.h>
> 
> Not sure this include is needed.
> 

I've checked it is needed.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:101
> > +    auto completionHandler = resolveQueue->getCompletionHandler(identifier);
> 
> Maybe there is a way to have takeCompletionHandler() which would allow
> getting and removing it at the same time?
> 

Not sure, I'll check it.

> > Source/WebCore/platform/network/soup/DNSResolveQueueSoup.cpp:147
> > +        return nullptr;
> 
> I would add ASSERT(completionAndCancelHandlers !=
> m_completionAndCancelHandlers.end()) since there is probably something wrong
> in that case.
> Or maybe add the assert at the call site?

Good point, I'll use an assertion.

Thanks again for the review.
Comment 53 Alejandro G. Castro 2018-03-07 05:37:29 PST
Created attachment 335187 [details]
Patch
Comment 54 Alejandro G. Castro 2018-03-07 05:55:54 PST
Created attachment 335188 [details]
Patch
Comment 55 Alejandro G. Castro 2018-03-07 06:41:37 PST
Created attachment 335189 [details]
Patch for landing
Comment 56 WebKit Commit Bot 2018-03-07 07:21:29 PST
Comment on attachment 335189 [details]
Patch for landing

Rejecting attachment 335189 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335189, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
e too many unreachable loose objects; run 'git prune' to remove them.

Current branch master is up to date.
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
error: The last gc run reported the following. Please correct the root cause
and remove /Volumes/Data/EWS/WebKit/.git/gc.log.
Automatic cleanup will not be performed until the file is removed.

warning: There are too many unreachable loose objects; run 'git prune' to remove them.


Full output: http://webkit-queues.webkit.org/results/6839248
Comment 57 Alejandro G. Castro 2018-03-07 08:13:28 PST
commit-queue failed in a weird way, it landed the patch but failed to update, apparently some performance problem with git.

https://trac.webkit.org/changeset/229359/webkit

Anyway it seems everything worked so closing the bug.

Thanks everyone.
Comment 58 Radar WebKit Bug Importer 2018-03-07 08:14:22 PST
<rdar://problem/38222040>