Bug 227819

Created attachment 433187 [details] Patch

Comment on attachment 433187 [details] Patch ChangeLog of LayoutTests is missing.

Committed r279778 (239545@main): <https://commits.webkit.org/239545@main>

Created attachment 433221 [details] Example of test actual vs expectation diff FYI this caused a number of tests under http/tests/storageAccess/ to no longer match expectations, generally along the lines of the example, but the tests still pass.

(In reply to Arcady Goldmints-Orlov from comment #4) > Created attachment 433221 [details] > Example of test actual vs expectation diff > > FYI this caused a number of tests under http/tests/storageAccess/ to no > longer match expectations, generally along the lines of the example, but the > tests still pass. Thanks for noticing this Arcady. I can confirm that after this patch those tests have different expectations. Also i'm re-reading the doc of soup_cookie_jar_get_cookie_list_with_same_site_info() and according to it <https://libsoup.org/SoupCookieJar.html#soup-cookie-jar-get-cookie-list-with-same-site-info> null may be passed as value for the site_for_cookies argument (cookieURI). So I will revert this, it seems it is not the right fix.

Reverted r279778 for reason: It caused unexpected text diffs on http/tests/storageAccess tests Committed r279825 (239586@main): <https://commits.webkit.org/239586@main>

This is the trace from the debug bot: Thread 1 (Thread 0x7f6e62fd4ec0 (LWP 46893)): #0 g_logv (log_domain=0x7f6e643df973 "libsoup", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=<optimized out>) at ../glib/gmessages.c:1413 #1 0x00007f6e656fb973 in g_log (log_domain=log_domain@entry=0x7f6e643df973 "libsoup", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL, format=format@entry=0x7f6e65753ad0 "%s: assertion '%s' failed") at ../glib/gmessages.c:1451 #2 0x00007f6e656fc19d in g_return_if_fail_warning (log_domain=log_domain@entry=0x7f6e643df973 "libsoup", pretty_function=pretty_function@entry=0x7f6e643ea9f0 <__func__.3> "soup_host_matches_host", expression=expression@entry=0x7f6e643e1893 "host != NULL") at ../glib/gmessages.c:2883 #3 0x00007f6e643d2992 in soup_host_matches_host (host=<optimized out>, compare_with=compare_with@entry=0x55c8e1ef7340 "localhost") at ../libsoup/soup-misc.c:189 #4 0x00007f6e643a5e92 in cookie_is_valid_for_same_site_policy (for_http=0, is_top_level_navigation=0, cookie_uri=0x0, top_level=0x55c8e1e8e7d0, uri=0x55c8e253ab40, is_safe_method=1, cookie=0x7f6e18004b30) at ../libsoup/cookies/soup-cookie-jar.c:306 #5 get_cookies (jar=jar@entry=0x55c8e1ef8520 [SoupCookieJar], uri=uri@entry=0x55c8e253ab40, top_level=top_level@entry=0x55c8e1e8e7d0, site_for_cookies=site_for_cookies@entry=0x0, is_safe_method=is_safe_method@entry=1, for_http=for_http@entry=0, is_top_level_navigation=0, copy_cookies=1) at ../libsoup/cookies/soup-cookie-jar.c:358 #6 0x00007f6e643a654e in soup_cookie_jar_get_cookie_list_with_same_site_info (jar=0x55c8e1ef8520 [SoupCookieJar], uri=0x55c8e253ab40, top_level=0x55c8e1e8e7d0, site_for_cookies=0x0, for_http=0, is_safe_method=1, is_top_level_navigation=0) at ../libsoup/cookies/soup-cookie-jar.c:493 #7 0x00007f6e713e6a53 in WebCore::cookiesForSession(WebCore::NetworkStorageSession const&, WTF::URL const&, WTF::URL const&, WebCore::SameSiteInfo const&, std::optional<WTF::ObjectIdentifier<WebCore::FrameIdentifierType> >, std::optional<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, bool, WebCore::IncludeSecureCookies, WebCore::ShouldAskITP, WebCore::ShouldRelaxThirdPartyCookieBlocking) (session=..., firstParty=..., url=..., sameSiteInfo=..., frameID=std::optional<class WTF::ObjectIdentifier<WebCore::FrameIdentifierType>> = {...}, pageID=std::optional<class WTF::ObjectIdentifier<WebCore::PageIdentifierType>> = {...}, forHTTPHeader=false, includeSecureCookies=WebCore::IncludeSecureCookies::Yes, shouldAskITP=WebCore::ShouldAskITP::No, relaxThirdPartyCookieBlocking=WebCore::ShouldRelaxThirdPartyCookieBlocking::No) at ../../Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:595 It seems that on #3 it ends calling soup_host_matches_host() with host=NULL (and that causes the issue) But according to that trace it reach that point on #4, where it does: <https://gitlab.gnome.org/GNOME/libsoup/-/blob/2.99.9/libsoup/cookies/soup-cookie-jar.c#L306> return soup_host_matches_host (g_uri_get_host (cookie_uri ? cookie_uri : top_level), g_uri_get_host (uri)); cookie_uri is NULL, but it is checked so it should end calling soup_host_matches_host() with host=top_level, which is not NULL. I'm puzzled :? Any ideas of why this happens?

(In reply to Carlos Alberto Lopez Perez from comment #7) > I'm puzzled :? > Any ideas of why this happens? I suspect it would likely become clear if we had a printf showing the values of cookie_uri and top_level. Note the return value of g_uri_get_host() is nullable, so libsoup might be wrong to assume it can never return NULL there.

(In reply to Michael Catanzaro from comment #8) > (In reply to Carlos Alberto Lopez Perez from comment #7) > > I'm puzzled :? > > Any ideas of why this happens? > > I suspect it would likely become clear if we had a printf showing the values > of cookie_uri and top_level. > According to the trace above: cookie_uri=0x0 top_level=0x55c8e1e8e7d0

My guess is (a) top_level just doesn't contain a host component, e.g. it might be a file:// URI, although those should never set cookies and so shouldn't reach this point, or else (b) something wrong with the GUri parser that causes it to think the URI has no host component.

(In reply to Michael Catanzaro from comment #10) > My guess is (a) top_level just doesn't contain a host component, e.g. it > might be a file:// URI, although those should never set cookies and so > shouldn't reach this point This is what is happening. I put some printf's and the value of top_level is "about:blank" so it doesn't have a host component and therefore g_uri_get_host() returns null. Seems this is a bug on libsoup? It should nullcheck what g_uri_get_host() returns, shouldn't it?

Yes, I agree this is a libsoup bug. This is a great test. I would never have thought to test a corner-case like this. Page spawns an about:blank window, then gives it a frame that tries to set cookies. Huh.

This appears to have been fixed by r291741. Closing as duplicate. *** This bug has been marked as a duplicate of bug 238202 ***