Bug 196094 – [GTK][WPE] Implement a basic DNS cache

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Patrick Griffis]

[GTK][WPE] Implement a basic DNS cache

[Patrick Griffis]

Created attachment 365568 [details]
Patch

[Patrick Griffis]

Created attachment 365570 [details]
Patch

[Patrick Griffis]

Created attachment 365571 [details]
Patch

[EWS Watchlist]

Comment on attachment 365571 [details]
Patch

Attachment 365571 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11603776

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365614 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

Comment on attachment 365571 [details]
Patch

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

Great! This patch is perfect for libsoup, but not for WebKit. We should follow the WebKit coding style, use smart pointers and our internal data structures from WTF whenever possible. I'll add more specific comments in the patch.

> Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:115
> +    webkit_soup_cached_resolver_ensure_default();

We know this is called only once so we could simply:

GRefPtr<GResolver> resolver = webkitCachedResolverNew(adoptGRef(g_resolver_get_default()));
g_resolver_set_default(resolver.get());

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:27
> +#include "WebKitSoupCachedResolver.h"

This is not soup specific art all, so I would avoid the Soup in the name, I would just call this WebKitCachedResolver.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:45
> + */

This is private object in WebKit, so we don't need documentation. If you want to keep a comment explaining what this object is for, remove the gtk-doc tags and use C++ style comment.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:48
> +/* This version introduces querying names for ipv4/ipv6
> + * separately which we cache separately */

Use C++ style comment. It could be one line and finish with period.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:55
> +struct _WebKitSoupCachedResolver {

It's fine to keep the instance and class structs private here but we also need to use a private instance to be able to use smart pointers and C++ objects.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:56
> +    GResolver parent_instance;

parentInstance

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:57
> +    GResolver* wrapped_resolver;

GRefPtr<GResolver> wrappedResolver;

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:58
> +    GHashTable* dns_caches[N_CACHES];

Use HashMap from WTF instead of GHashTable. It could be

HashMap<CString, CachedResponse>

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:59
> +    GMutex dns_cache_lock;

Use WTF Lock

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:63
> +    GResolverClass parent_class;

parentClass

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:66
> +G_DEFINE_TYPE(WebKitSoupCachedResolver, webkit_soup_cached_resolver, G_TYPE_RESOLVER)

Use WebKitDefineType

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:68
> +/* These match Firefox */

C++ style comments

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:70
> +#define DNS_CACHE_EXPIRE_SECONDS 60
> +#define DNS_CACHE_MAX_SIZE 400

static const Seconds dnsCachedExpireSeconds = 60_s;
static const unsigned dnsCacheMaxSize = 400;

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:88
> +/**
> + * webkit_soup_cached_resolver_new:
> + * @wrapped_resolver: Underlying #GResolver to be cached
> + * 
> + * Note that @wrapped_resolver must not be a #WebKitSoupCachedResolver.
> + *
> + * You should generally use webkit_soup_cached_resolver_ensure_default()
> + * rather than this API directly.
> + *
> + * Returns:(transfer full): A new #WebKitSoupCachedResolver
> + */

Same comment here about the api docs.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:90
> +static WebKitSoupCachedResolver* 
> +webkit_soup_cached_resolver_new(GResolver* wrapped_resolver)

This should be one line. I would make this public instead of webkit_soup_cached_resolver_ensure_default().

GResolver* webkitCachedResolverNew(GRefPtr<GResolver>&& wrappedResolver)

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:92
> +    g_return_val_if_fail(wrapped_resolver, nullptr);

G_IS_RESOLVER(wrapperResolver.get(), nullptr);

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:93
> +    g_return_val_if_fail(!WEBKIT_IS_SOUP_CACHED_RESOLVER(wrapped_resolver), nullptr);

We wouldn't need this check, replace it with ASSERT if you really want to check this.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:97
> +    return static_cast<WebKitSoupCachedResolver*>(g_object_new(WEBKIT_TYPE_SOUP_CACHED_RESOLVER,
> +        "wrapped-resolver", wrapped_resolver,
> +        nullptr));

Since this is private object and won't be used by any binding, we can get rid of the construct property and assign the wrapped object here after g_object_new call.

auto* resolver = static_cast<WebKitCachedResolver*>(g_object_new(WEBKIT_TYPE_CACHED_RESOLVER, nullptr));
resolver->priv->wrappedResolver = WTFMove(wrappedResolver);

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:123
> +typedef struct {
> +    GList* addresses; /* owned */
> +    gint64 expiration;
> +} CachedResponse;

struct CachedResponse {
    Vector<GRefPtr<GInetAddress>> addresses;
    WallTime expiration;
};

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:130
> +static void
> +cached_response_free(CachedResponse* cache)
> +{
> +    g_resolver_free_addresses(cache->addresses);
> +    g_free(cache);
> +}

Then you don't need this.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:170
> +static gpointer
> +copy_object(gconstpointer obj, gpointer user_data)
> +{
> +    return g_object_ref(G_OBJECT(obj));
> +}
> +
> +static GList* 
> +copy_addresses(GList* addresses)
> +{
> +    return g_list_copy_deep(addresses, copy_object, nullptr);
> +}

You shouldn't need this either.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:177
> +    gint64 now = g_get_monotonic_time();

Seconds now = WallTime::now();

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:184
> +        if (cached->expiration <= now || size > DNS_CACHE_MAX_SIZE)

How can be the current cache bigger than the maximum size? Shouldn't we check the expiration time to decide what response to remove when the cache is full?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:208
> +    /* Cleanup while we are at it. */
> +    cleanup_dns_cache(self, cache);

I think we could split the case of cache being full from the case of expired responses. Should we use a timer to remove expired resources instead of doing it only when adding new entries?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:234
> +    if (cached && cached->expiration > now)
> +        addresses = copy_addresses(cached->addresses);

Shouldn't we remove the entry from the cache if it has expired?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:240
> +

I think we can add a private C++ class here to handle the cache and make the GObject use it.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:257
> +typedef struct {
> +    char* hostname;
> +    GResolverNameLookupFlags flags;
> +} LookupData;

struct LookupAsyncData {
    GUniquePtr<char> hostname;
    GResolverNameLookupFlags flags;
};
WEBKIT_DEFINE_ASYNC_DATA_STRUCT(LookupAsyncData);

That will define createLookupAsyncData() and destroyLookupAsyncData() function using fast malloc and allowing to use smart pointers.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:325
> +    GTask* task = g_task_new(self, cancellable, callback, user_data);

GRefPtr<GTask> task = adoptGRef()

> Source/WebKit/SourcesGTK.txt:92
> +Shared/glib/WebKitSoupCachedResolver.cpp

This doesn't belong to Shared. I Shared we have things shared by multiple processes, but the dns cache is only used in the network process, so this should be in NetworkProcess/glib or NetworkProcess/soup

[Carlos Garcia Campos]

Created attachment 372453 [details]
Patch

Just addressed my own review comments

[Michael Catanzaro]

Comment on attachment 372453 [details]
Patch

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

I have a question about the threadsafety semantics. I was originally going to ask why you are using a lock in the DNSCache implementation, since I would have expected that would only ever be called from the main thread. But I see that is wrong, because GResolver is a global singleton, and global singletons in GLib are (probably) expected to be threadsafe (even though I've never seen any documentation indicate this clearly). OK then, the DNSCache is used by your WebKitCachedResolver, so makes sense for DNSCache to be threadsafe.

But WebKitCachedResolver is not itself threadsafe. So that doesn't make sense to me.

I have a suspicion that these will both only ever be used by WebKit on its main thread, so the DNSCache lock could be removed...? Is that not correct? If it's not correct, then I think WebKitCachedResolver needs its own lock. It just seems really suspicious that one has a lock but not the other.

> Source/WebKit/NetworkProcess/glib/WebKitCachedResolver.cpp:74
> +WEBKIT_DEFINE_ASYNC_DATA_STRUCT(LookupAsyncData)

Huh, I didn't know we had this.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 372453 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372453&action=review
> 
> I have a question about the threadsafety semantics. I was originally going
> to ask why you are using a lock in the DNSCache implementation, since I
> would have expected that would only ever be called from the main thread. But
> I see that is wrong, because GResolver is a global singleton, and global
> singletons in GLib are (probably) expected to be threadsafe (even though
> I've never seen any documentation indicate this clearly). OK then, the
> DNSCache is used by your WebKitCachedResolver, so makes sense for DNSCache
> to be threadsafe.

I have no idea. Patrick's patch was using a lock only to protect the cache itself, and that's what I've done here. 

> But WebKitCachedResolver is not itself threadsafe. So that doesn't make
> sense to me.

Why it's not? I think it is as long as the wrapped resolver is, and the default resolver is AFAIK.

> I have a suspicion that these will both only ever be used by WebKit on its
> main thread, so the DNSCache lock could be removed...? Is that not correct?
> If it's not correct, then I think WebKitCachedResolver needs its own lock.
> It just seems really suspicious that one has a lock but not the other.

I thought the same, but I'm just addressing my review comments here.

> > Source/WebKit/NetworkProcess/glib/WebKitCachedResolver.cpp:74
> > +WEBKIT_DEFINE_ASYNC_DATA_STRUCT(LookupAsyncData)
> 
> Huh, I didn't know we had this.

It uses fast malloc and allows to use C++ objects as members.

[Patrick Griffis]

(In reply to Carlos Garcia Campos from comment #9)
> (In reply to Michael Catanzaro from comment #8)
> > Comment on attachment 372453 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=372453&action=review
> > 
> > I have a question about the threadsafety semantics. I was originally going
> > to ask why you are using a lock in the DNSCache implementation, since I
> > would have expected that would only ever be called from the main thread. But
> > I see that is wrong, because GResolver is a global singleton, and global
> > singletons in GLib are (probably) expected to be threadsafe (even though
> > I've never seen any documentation indicate this clearly). OK then, the
> > DNSCache is used by your WebKitCachedResolver, so makes sense for DNSCache
> > to be threadsafe.
> 
> I have no idea. Patrick's patch was using a lock only to protect the cache
> itself, and that's what I've done here. 
> 

All calls in my impl were directly redirected to the the existing Resolver which was threadsafe. The only thing the cached resolver did was put results in a cache so that had to be threadsafe.

[Michael Catanzaro]

Comment on attachment 372453 [details]
Patch

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

> Source/WebKit/NetworkProcess/glib/WebKitCachedResolver.cpp:38
> +typedef struct {
> +    GRefPtr<GResolver> wrappedResolver;
> +    DNSCache cache;
> +} WebKitCachedResolverPrivate;

OK you're right, I suppose WebKitCachedResolver is threadsafe because both its data members are.

I recommend adding a comment here to warn about this, otherwise it would be really really easy to mess this up in the future by adding a non-threadsafe member here without a lock.

[WebKit Commit Bot]

Comment on attachment 372453 [details]
Patch

Clearing flags on attachment: 372453

Committed r246671: <https://trac.webkit.org/changeset/246671>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> I recommend adding a comment here to warn about this, otherwise it would be
> really really easy to mess this up in the future by adding a non-threadsafe
> member here without a lock.

I can all but promise this will break in the future, since you didn't add a comment and there's no indication whatsoever in the WebKitCachedResolver file that it is supposed to be threadsafe.

[Claudio Saavedra]

About the time this landed, several tests started having crashes in the network process, not very consistently but consistently enough to think that the underlying cause is probably this. Here a sample:

https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r247200%20(10899)/imported/w3c/web-platform-tests/cors/redirect-preflight-2-crash-log.txt

With crash history:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fcors%2Fredirect-preflight-2.htm

[Claudio Saavedra]

Crashes reported here https://bugs.webkit.org/show_bug.cgi?id=199572