Bug 203193

Summary: [GTK][WPE] Test PublicSuffix.TopPrivatelyControlledDomain is failing since r250589
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKitGTKAssignee: Claudio Saavedra <csaavedra>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, bugs-noreply, commit-queue, csaavedra, pgriffis, svillar, wilander
Priority: P2 Keywords: Gtk
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Carlos Garcia Campos
Reported 2019-10-21 04:30:50 PDT
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().
Attachments
Patch (2.02 KB, patch)
2019-10-23 03:18 PDT, Claudio Saavedra 		no flags
DetailsFormatted DiffDiff
Patch (3.50 KB, patch)
2019-10-25 03:13 PDT, Claudio Saavedra 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Claudio Saavedra
Comment 1 2019-10-21 04:40:34 PDT
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.
Claudio Saavedra
Comment 2 2019-10-23 03:18:56 PDT
Created attachment 381670 [details] Patch
Carlos Garcia Campos
Comment 3 2019-10-23 03:30:59 PDT
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.
Sergio Villar Senin
Comment 4 2019-10-23 03:43:17 PDT
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
Claudio Saavedra
Comment 5 2019-10-23 08:15:05 PDT
(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.
Claudio Saavedra
Comment 6 2019-10-25 03:13:47 PDT
Created attachment 381902 [details] Patch
Carlos Garcia Campos
Comment 7 2019-10-25 06:14:44 PDT
Comment on attachment 381902 [details] Patch Make sure the new test you added passes in all other platforms before landing, please.
WebKit Commit Bot
Comment 8 2019-10-27 13:23:49 PDT
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.
WebKit Commit Bot
Comment 9 2019-10-27 13:24:36 PDT
Comment on attachment 381902 [details] Patch Clearing flags on attachment: 381902 Committed r251643: <https://trac.webkit.org/changeset/251643>
WebKit Commit Bot
Comment 10 2019-10-27 13:24:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.