Bug 289129 – REGRESSION(289693@main): [SOUP] Internal Server Error on reddit.com

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Since 289693@main "[SOUP] Add support for the Cookie Store API" it's no longer possible to interact with reddit.com. Trying to upvote/downvote, comment, or even just clear notifications all fails with error message "ClientGQLNetworkError: Internal Server Error".

[Michael Catanzaro]

The problem is specifically caused by HAVE(COOKIE_CHANGE_LISTENER_API). Sabotaging that and fixing the build with it disabled fixes the problem on reddit.com:

diff --git a/Source/WTF/wtf/PlatformHave.h b/Source/WTF/wtf/PlatformHave.h
index 27188089ab5d..a804142733b7 100644
--- a/Source/WTF/wtf/PlatformHave.h
+++ b/Source/WTF/wtf/PlatformHave.h
@@ -552,7 +552,9 @@
 #define HAVE_DESIGN_SYSTEM_UI_FONTS 1
 #endif
 
-#if PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE)
+#if PLATFORM(COCOA)
+// FIXME: Should reenable for || PLATFORM(GTK) || PLATFORM(WPE).
+// https://webkit.org/b/289129
 #define HAVE_COOKIE_CHANGE_LISTENER_API 1
 #endif
 
diff --git a/Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp b/Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp
index c191cdc19301..15f683fd803f 100644
--- a/Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp
+++ b/Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp
@@ -137,10 +137,12 @@ void NetworkStorageSession::setCookieStorage(GRefPtr<SoupCookieJar>&& jar)
     m_cookieStorage = WTFMove(jar);
     g_signal_connect_swapped(m_cookieStorage.get(), "changed", G_CALLBACK(cookiesDidChange), this);
 
+#if HAVE(COOKIE_CHANGE_LISTENER_API)
     for (auto& [host, observers] : m_cookieChangeObservers) {
         for (auto& observer : observers)
             observer.allCookiesDeleted();
     }
+#endif
 }
 
 void NetworkStorageSession::setCookieObserverHandler(Function<void ()>&& handler)
@@ -567,10 +569,12 @@ void NetworkStorageSession::deleteAllCookies(CompletionHandler<void()>&& complet
         soup_cookie_jar_delete_cookie(cookieJar, cookie);
     }
 
+#if HAVE(COOKIE_CHANGE_LISTENER_API)
     for (auto& [host, observers] : m_cookieChangeObservers) {
         for (auto& observer : observers)
             observer.allCookiesDeleted();
     }
+#endif
 
     completionHandler();
 }

[Michael Catanzaro]

I'm landing this workaround in the 2.48 branch now, so we can release 2.48 without worrying about this regression. I won't do this on main, though.

[Michael Catanzaro]

I've just begun investigating. Workaround is to never retrieve cookies from the WebCookieCache, so something is wrong with the cache:

diff --git a/Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp b/Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp
index 70a6c3c9ed0f..a994a44b9c38 100644
--- a/Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp
+++ b/Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp
@@ -150,8 +150,8 @@ String WebCookieJar::cookies(WebCore::Document& document, const URL& url) const
     auto pageID = page->identifier();
     auto webPageProxyID = page->webPageProxyIdentifier();
 
-    if (isEligibleForCache(*webFrame, document.firstPartyForCookies(), url))
-        return m_cache.cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo, url, frameID, pageID, webPageProxyID, includeSecureCookies);
+    //if (isEligibleForCache(*webFrame, document.firstPartyForCookies(), url))
+    //    return m_cache.cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo, url, frameID, pageID, webPageProxyID, includeSecureCookies);
 
     auto sendResult = WebProcess::singleton().ensureNetworkProcessConnection().protectedConnection()->sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(document.firstPartyForCookies(), sameSiteInfo, url, frameID, pageID, includeSecureCookies, webPageProxyID), 0);
     auto [cookieString, secureCookiesAccessed] = sendResult.takeReplyOr(String { }, false);

[Michael Catanzaro]

Problem is NetworkStorageSession::setCookieStorage does not notify observers about added cookies, so all cookies that existed on disk before creating the network process are missing from WebCookieCache.

[Michael Catanzaro]

I think there's two problems here:

 * NetworkStorageSession::setCookieStorage does not notify observers about added cookies, so all cookies that existed on disk before creating the network process are missing from WebCookieCache
 * NetworkStorageSessionSoup is working with hosts, but NetworkStorageSessionCocoa is working with domains

The SoupCookie documentation defines the difference between host and domain, which matches web terminology:

> domain and path give the host or domain, and path within that host domain, to restrict this cookie to. If domain starts with “.”, that indicates a domain (which matches the string after the “.”, or any hostname that has domain as a suffix). Otherwise, it is a hostname and must match exactly.

reddit sets the domain of most of its cookies to ".reddit.com" but the CookieChangeObserver exactly matches only "www.reddit.com" so the cookies never get added to any WebCookieCache. But in WebCookieJar::cookies, isEligibleForCache returns true, and so the cookie is looked up exclusively from the cache. Since it's not in the cache, it effectively does not exist.

[Michael Catanzaro]

Created attachment 474597 [details]
WIP patch

Here is a WIP patch that tackles the first problem, just so I don't lose the work. This is insufficient and not even ready for a draft pull request.

[Michael Catanzaro]

Also, warning, I accidentally left some debugging in that patch:

	    if (host.startsWith('.'))
	        host = host.substring(1, host.length());

This is an insufficient attempt to address the host vs. domain mismatch, which should not land.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
>  * NetworkStorageSessionSoup is working with hosts, but
> NetworkStorageSessionCocoa is working with domains

Well, no sorry, I misread once again. Both the NetworkStorageSessionSoup and NetworkStorageSessionCocoa paths sure look equivalent. Example:

void NetworkStorageSession::registerCookieChangeListenersIfNecessary()
{
    // ...

    [nsCookieStorage() _setCookiesChangedHandler:makeBlockPtr([this, weakThis = WeakPtr { *this }](NSArray<NSHTTPCookie *> *addedCookies, NSString *domainForChangedCookie) {
        if (!weakThis)
            return;
        String host = domainForChangedCookie;
        auto it = m_cookieChangeObservers.find(host);
        if (it == m_cookieChangeObservers.end())
            return;

Here it sure looks like domainForChangedCookie is a domain rather than a host. But it's used exactly as a key in the observers table, so only exact matches will be considered. I don't know how it works. The actual domain of the cookies is ".reddit.com" so it's supposed to match any hostname with ".reddit.com" as a suffix. I assume the value of domainForChangedCookie provided by the NSHTTPCookieStorage must be "reddit.com" without the period. But NetworkStorageSessionCocoa is not actually searching for all observers that match the suffix; just like NetworkStorageSessionSoup, it looks for an exact match. I don't see how it could ever work, because the only observer we will ever have is for www.reddit.com, not for reddit.com. For example:

startListeningForCookieChangeNotifications: url=https://www.reddit.com/r/linux/ host=www.reddit.com

So I don't see how the attempt to find a matching observer in m_cookieChangeObservers would ever succeed.

Solution is to iterate through all the keys in the map to see if they match:

+    auto host = String::fromUTF8(soup_cookie_get_domain(newCookie));
+    for (auto& host : m_cookieChangeObservers.keys()) {
+        if (soup_cookie_domain_matches(newCookie, host.utf8().data())) {
+            auto observers = m_cookieChangeObservers.getOptional(host);
+            if (!observers)
+                break;
+
+            for (auto& observer : *observers)
+                observer.cookiesAdded(host, { Cookie(newCookie) });
+        }
+    }

This does not match NetworkStorageSessionCocoa.mm, which means we would introduce a potentially-dangerous behavior difference between platforms, but we have to or cookies don't work. It really looks like the Cocoa code should be broken, but this seems unlikely because the code has not been touched in 5 years and surely somebody would have noticed if cookies were broken in safari. So I'm stumped there.

I'll submit a pull request later today.

[Michael Catanzaro]

I'm quite nervous about this behavior difference. Would be great if somebody with macOS could investigate.

(In reply to Michael Catanzaro from comment #5)
> I think there's two problems here:
> 
>  * NetworkStorageSession::setCookieStorage does not notify observers about
> added cookies, so all cookies that existed on disk before creating the
> network process are missing from WebCookieCache

This turns out to be not a problem for reddit.com because the cookie jar's changed signal is always emitted after the cookie jar is registered with the NetworkStorageSessionSoup, so in practice the cookies get added to the observer anyway. I'm not actually sure whether it ever matters, because I think this function is possibly always called before any observers are added. I'll fix it for correctness regardless.

[Michael Catanzaro]

We have a guess. The Cocoa code seems to be using a private API of NSHTTPCookieStorage that allows subscribing to cookie change notification for a particular domain. If NetworkStorageSessionCocoa subscribes to changes for "www.reddit.com" then possibly the NSHTTPCookieStorage would fire the change notification with domain "www.reddit.com" whenever a cookie with domain ".reddit.com" changes. i.e. we think that NSHTTPCookieStorage might be handling the soup_cookie_domain_matches() part itself. Possibly. There doesn't appear to be any documentation of how it works; the corresponding public API https://developer.apple.com/documentation/foundation/nshttpcookiemanagercookieschangednotification is different and seems to be a simple boolean notification where you have to figure out what changed yourself.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/42640

[EWS]

Committed 292428@main (b3028af5d2a4): <https://commits.webkit.org/292428@main>

Reviewed commits have been landed. Closing PR #42640 and removing active labels.