The new test case added in r250589 is failing: **FAIL** PublicSuffix.TopPrivatelyControlledDomain ../../Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:182 Expected equality of these values: String("test.com") Which is: test.com topPrivatelyControlledDomain(".test.com") Which is: This is because libsoup considers that hostnames starting with a dot are not valid and SOUP_TLD_ERROR_INVALID_HOSTNAME is returned. I don't know if we should just remove the leading dot before passing the hostname to soup_tld_get_base_domain().
libsoup since the TLD API was added considers domains starting with one or more dots not to be valid domains: /* Valid hostnames neither start with a dot nor have more than one * dot together. */ if (*hostname == '.') { g_set_error_literal (error, SOUP_TLD_ERROR, SOUP_TLD_ERROR_INVALID_HOSTNAME, _("Invalid hostname")); return NULL; } This is why the soup cookies API does a round of normalization before using the TLD API: normalized_cookie_domain = normalize_cookie_domain (cookie->domain); cookie_base_domain = soup_tld_get_base_domain (normalized_cookie_domain, NULL); I think that if we're dealing also with cookie domains, we need to remove the leading dot before using the soup API.
Created attachment 381670 [details] Patch
Comment on attachment 381670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381670&action=review > Source/WebCore/platform/soup/PublicSuffixSoup.cpp:59 > + unsigned pos = 0; pos -> position > Source/WebCore/platform/soup/PublicSuffixSoup.cpp:64 > + while (domainUTF8.data()[pos] == '.') > + pos++; > + > + GUniqueOutPtr<GError> error; > + if (const char* baseDomain = soup_tld_get_base_domain(domainUTF8.data() + pos, &error.outPtr())) We are returning early if domain is empty, it might become empty here if there are only dots.
Comment on attachment 381670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381670&action=review > Source/WebCore/platform/soup/PublicSuffixSoup.cpp:61 > + pos++; this might lead to an invalid access in the case of having only dots in the domain
(In reply to Sergio Villar Senin from comment #4) > Comment on attachment 381670 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381670&action=review > > > Source/WebCore/platform/soup/PublicSuffixSoup.cpp:61 > > + pos++; > > this might lead to an invalid access in the case of having only dots in the > domain CStringBuffer (the underlying type containing the data) is implicitly allocated with an extra 0 byte, so this shouldn't happen unless the domain was empty, but we return earlier if that's the case, so I think this is safe.
Created attachment 381902 [details] Patch
Comment on attachment 381902 [details] Patch Make sure the new test you added passes in all other platforms before landing, please.
The commit-queue encountered the following flaky tests while processing attachment 381902 [details]: imported/w3c/web-platform-tests/css/css-position/position-absolute-container-dynamic-002.html bug 203473 (author: simon.fraser@apple.com) imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html bug 203474 (author: simon.fraser@apple.com) imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: ysuzuki@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 381902 [details] Patch Clearing flags on attachment: 381902 Committed r251643: <https://trac.webkit.org/changeset/251643>
All reviewed patches have been landed. Closing bug.