Bug 204137

Created attachment 383419 [details] Patch

Note that this requires libsoup to be merged: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/36 Once merged do we build the development release in JHBuild?

Created attachment 383420 [details] Patch

(In reply to Patrick Griffis from comment #2) > Once merged do we build the development release in JHBuild? You'll want to update the jhbuild moduleset in your patch here to ensure you don't break the test expectations with your expectations changes.

*** Bug 184955 has been marked as a duplicate of this bug. ***

Comment on attachment 383420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383420&action=review r- because tests will fail, we need to update the libsoup in jhbuild too. > Source/WebCore/ChangeLog:11 > + Added an httpMethod argument to cookie lookup functions for the > + soup API to use. Modified other backends to simply ignore this. Do you know why other ports don't need the http method? > Source/WebCore/loader/CookieJar.cpp:84 > + result = session->cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, includeSecureCookies, httpMethod); We only need the http method to determine whether it's safe, right? and it's only used by same site cookies, right? If that's the case maybe it would be easier to add bool isSafeHTTPMethod to SameSitoInfo and we don't need to add a new parameter. We still have time to change the libsoup API or we can use any safe HTTP method when it's safe, since it's only used for that. > Source/WebCore/platform/network/soup/CookieSoup.cpp:35 > +#if SOUP_CHECK_VERSION(2, 69, 0) Better use the first version including the new api, even if it's an unstable release. That way we don't need to wait until the next stable release to test this. > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:486 > + httpMethodStr = httpMethod->ascii().data(); This is a temporary, either cache the CString returned by ascii() or do httpMethod ? httpMethod->ascii().data() : nullptr directly in the call to soup_cookie_jar_get_cookie_list_with_same_site_info(). > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:187 > + URL cookieURL = soupURIToURL(siteForCookies); > + setIsSameSite(areRegistrableDomainsEqual(cookieURL, m_url)); This could be one line. > LayoutTests/ChangeLog:9 > + Updated GTK/WPE test expectations to pass most same-site cookie tests > + matching the Apple ports. To do this, we need to wait for the next libsoup unstable release and use it in out jhbuild, otherwise the tests will fail in the bots that use a previous version.

Created attachment 389653 [details] Patch

GNOME ftp doesn't have the tarball yet, not sure how long that is cached for.

Created attachment 389654 [details] Patch

Comment on attachment 389654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389654&action=review > Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:189 > + else > +#endif > + m_sameSiteDisposition = SameSiteDisposition::Unspecified; I can tell you Carlos doesn't like to mix #if #endif and runtime conditionals like this. -Wmisleading-indentation won't like it either. Look how much cleaner it is this way, with just one line of code duplication: #if SOUP_CHECK_VERSION(2, 69, 90) setIsTopSite(soup_message_get_is_top_level_navigation(soupMessage)); if (SoupURI* siteForCookies = soup_message_get_site_for_cookies(soupMessage)) setIsSameSite(areRegistrableDomainsEqual(soupURIToURL(siteForCookies), m_url)); else m_sameSiteDisposition = SameSiteDisposition::Unspecified; #else m_sameSiteDisposition = SameSiteDisposition::Unspecified; #endif > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:688 > + if (isTopLevelNavigation()) { > + request.setFirstPartyForCookies(request.url()); > +#if SOUP_CHECK_VERSION(2, 69, 90) > + soup_message_set_is_top_level_navigation(m_soupMessage.get(), true); > + } > + > + if (request.isSameSite()) { > + GUniquePtr<SoupURI> requestURI = urlToSoupURI(request.url()); > + soup_message_set_site_for_cookies(m_soupMessage.get(), requestURI.get()); > +#endif > + } if (isTopLevelNavigation()) { request.setFirstPartyForCookies(request.url()); #if SOUP_CHECK_VERSION(2, 69, 90) soup_message_set_is_top_level_navigation(m_soupMessage.get(), true); #endif } #if SOUP_CHECK_VERSION(2, 69, 90) if (request.isSameSite()) { GUniquePtr<SoupURI> requestURI = urlToSoupURI(request.url()); soup_message_set_site_for_cookies(m_soupMessage.get(), requestURI.get()); } #endif > Tools/gtk/jhbuild.modules:56 > <repository type="tarball" name="ftp.gnome.org" > href="http://ftp.gnome.org"/> Future work: everything should switch to use download.gnome.org.

Comment on attachment 389654 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389654&action=review > Source/WebCore/platform/network/SameSiteInfo.cpp:36 > +static bool httpMethodIsSafe(const String& method) > +{ > + return method == "GET" || method == "HEAD" || method == "OPTIONS" || method == "TRACE"; > +} I think this could be moved to HTTPParsers.cpp as isSafeMethod(). We should also use equalLettersIgnoringASCIICase() instead of checking the uppercase method names directly. > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:459 > + GUniquePtr<SoupURI> cookieURI = sameSiteInfo.isSameSite ? urlToSoupURI(url) : nullptr; Move this after the if (!firstPartyURI) because this is not needed if we return early.

Created attachment 389666 [details] Patch

Comment on attachment 389666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389666&action=review LGTM > Source/WebCore/platform/network/HTTPParsers.cpp:988 > +bool isSafeMethod(const String& method) Add a comment linking to https://tools.ietf.org/html/rfc7231#section-4.2.1

Created attachment 389855 [details] Patch

Has libsoup 2.69.90 been released? the tarball doesn't exist, at least in the url https://download.gnome.org/libsoup/2.69/libsoup-2.69.90.tar.xz

Ok, the right url is https://download.gnome.org/sources/libsoup/2.69/libsoup-2.69.90.tar.xz so the repo in the jhbuild moduleset is wrong

Comment on attachment 389855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389855&action=review > Tools/gtk/jhbuild.modules:58 > + <repository type="tarball" name="download.gnome.org" > + href="https://download.gnome.org/sources"/> Maybe thi needs a trailing slash? https://download.gnome.org/sources/

Created attachment 389945 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 389945 [details]: imported/w3c/web-platform-tests/web-animations/timing-model/timelines/update-and-send-events-replacement.html bug 207335 (author: graouts@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 389945 [details] Patch Clearing flags on attachment: 389945 Committed r256013: <https://trac.webkit.org/changeset/256013>

All reviewed patches have been landed. Closing bug.