Bug 214222 – ASSERT([filteredCookies.get() count] <= 1) on imported/w3c/web-platform-tests/websockets/cookies/third-party-cookie-accepted.https.html

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

imported/w3c/web-platform-tests/websockets/cookies/third-party-cookie-accepted.https.html is crashing consistently on iOS WK2 Debug configuration:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000011d578d0e WTFCrash + 14 (Assertions.cpp:295)
1   com.apple.WebCore             	0x0000000122d552eb WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x0000000124aa42a0 WebCore::NetworkStorageSession::setCookiesFromDOM(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::Optional<WTF::ObjectIdentifier<WebCore::FrameIdentifierType> >, WTF::Optional<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking) const + 1136 (NetworkStorageSessionCocoa.mm:467)
3   com.apple.WebKit              	0x000000010cb40b1b WebKit::NetworkConnectionToWebProcess::setCookiesFromDOM(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking) + 299 (NetworkConnectionToWebProcess.cpp:655)
4   com.apple.WebKit              	0x000000010c697615 void IPC::callMemberFunctionImpl<WebKit::NetworkConnectionToWebProcess, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking), std::__1::tuple<WTF::URL, WebCore::SameSiteInfo, WTF::URL, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String, WebCore::ShouldRelaxThirdPartyCookieBlocking>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul>(WebKit::NetworkConnectionToWebProcess*, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking), std::__1::tuple<WTF::URL, WebCore::SameSiteInfo, WTF::URL, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String, WebCore::ShouldRelaxThirdPartyCookieBlocking>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul>) + 437 (HandleMessage.h:42)
5   com.apple.WebKit              	0x000000010c690580 void IPC::callMemberFunction<WebKit::NetworkConnectionToWebProcess, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking), std::__1::tuple<WTF::URL, WebCore::SameSiteInfo, WTF::URL, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String, WebCore::ShouldRelaxThirdPartyCookieBlocking>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul> >(std::__1::tuple<WTF::URL, WebCore::SameSiteInfo, WTF::URL, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String, WebCore::ShouldRelaxThirdPartyCookieBlocking>&&, WebKit::NetworkConnectionToWebProcess*, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking)) + 112 (HandleMessage.h:48)
6   com.apple.WebKit              	0x000000010c66496e void IPC::handleMessage<Messages::NetworkConnectionToWebProcess::SetCookiesFromDOM, WebKit::NetworkConnectionToWebProcess, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking)>(IPC::Decoder&, WebKit::NetworkConnectionToWebProcess*, void (WebKit::NetworkConnectionToWebProcess::*)(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::ShouldAskITP, WTF::String const&, WebCore::ShouldRelaxThirdPartyCookieBlocking)) + 190 (HandleMessage.h:115)
7   com.apple.WebKit              	0x000000010c6630dc WebKit::NetworkConnectionToWebProcess::didReceiveNetworkConnectionToWebProcessMessage(IPC::Connection&, IPC::Decoder&) + 1116 (NetworkConnectionToWebProcessMessageReceiver.cpp:365)
8   com.apple.WebKit              	0x000000010cb3bd63 WebKit::NetworkConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 179 (NetworkConnectionToWebProcess.cpp:191)
9   com.apple.WebKit              	0x000000010c5a54ff IPC::Connection::dispatchMessage(IPC::Decoder&) + 431 (Connection.cpp:1002)
10  com.apple.WebKit              	0x000000010c5a5e30 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 528
11  com.apple.WebKit              	0x000000010c5a64c0 IPC::Connection::dispatchOneIncomingMessage() + 208 (Connection.cpp:1139)
12  com.apple.WebKit              	0x000000010c5c5278 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7::operator()() + 88 (Connection.cpp:979)
13  com.apple.WebKit              	0x000000010c5c518e WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call() + 30 (Function.h:52)
14  com.apple.JavaScriptCore      	0x000000011d5a4b02 WTF::Function<void ()>::operator()() const + 130 (Function.h:84)
15  com.apple.JavaScriptCore      	0x000000011d61d015 WTF::RunLoop::performWork() + 341 (RunLoop.cpp:120)
16  com.apple.JavaScriptCore      	0x000000011d6205b1 WTF::RunLoop::performWork(void*) + 33 (RunLoopCF.cpp:39)
17  com.apple.CoreFoundation      	0x0000000108189c71 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
18  com.apple.CoreFoundation      	0x0000000108189b9c __CFRunLoopDoSource0 + 76
19  com.apple.CoreFoundation      	0x0000000108189374 __CFRunLoopDoSources0 + 180
20  com.apple.CoreFoundation      	0x0000000108183f6e __CFRunLoopRun + 974
21  com.apple.CoreFoundation      	0x0000000108183884 CFRunLoopRunSpecific + 404
22  com.apple.Foundation          	0x0000000107a49831 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 211
23  com.apple.Foundation          	0x0000000107a49a45 -[NSRunLoop(NSRunLoop) run] + 76
24  libxpc.dylib                  	0x0000000109dd65ae _xpc_objc_main + 297
25  libxpc.dylib                  	0x0000000109dd8976 xpc_main + 132
26  com.apple.WebKit              	0x000000010cce509b WebKit::XPCServiceMain(int, char const**) + 1067 (XPCServiceMain.mm:171)
27  com.apple.WebKit              	0x000000010e20758b WKXPCServiceMain + 27 (WKMain.mm:33)
28  com.apple.WebKit.Networking   	0x00000001078e9da2 main + 34 (AuxiliaryProcessMain.cpp:30)
29  libdyld.dylib                 	0x0000000109aa210d start + 1

[Chris Dumez]

Updated expectations in <https://trac.webkit.org/changeset/264268>.

[Alexey Proskuryakov]

Line 467 is this, which is a bit surprising.

    ASSERT([filteredCookies.get() count] <= 1);

[Radar WebKit Bug Importer]

<rdar://problem/65587120>

[Chris Dumez]

Looks like we use a different API for parsing cookies on macOS and iOS:
#if PLATFORM(MAC)
    NSArray *unfilteredCookies = [NSHTTPCookie _parsedCookiesWithResponseHeaderFields:headerFields forURL:cookieURL];
#else
    NSArray *unfilteredCookies = [NSHTTPCookie cookiesWithResponseHeaderFields:headerFields forURL:cookieURL];
#endif

[Chris Dumez]

The cookie string is:

samesite-unspecified=0.2755897489430851; Path=/, samesite-lax=0.2755897489430851; Path=/; SameSite=Lax, samesite-strict=0.2755897489430851; Path=/; SameSite=Strict, samesite-none=0.2755897489430851; Path=/; SameSite=None; Secure

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> The cookie string is:
> 
> samesite-unspecified=0.2755897489430851; Path=/,
> samesite-lax=0.2755897489430851; Path=/; SameSite=Lax,
> samesite-strict=0.2755897489430851; Path=/; SameSite=Strict,
> samesite-none=0.2755897489430851; Path=/; SameSite=None; Secure

the HTML spec says to treat the string set to document.cookie as a Set-Cookie header. Set-Cookie header is defined here:
https://tools.ietf.org/html/rfc6265#section-4.1

It should only support one cookie indeed.

[Chris Dumez]

Created attachment 404995 [details]
Patch

[Chris Dumez]

Created attachment 404996 [details]
Patch

[Alex Christensen]

Comment on attachment 404996 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404996&action=review

I think this is good, but this is a bad time to make a change like this.  Could this wait until after the next branch?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424
> +            RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]);

auto

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459
> +    setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo);

@[cookie]

[Chris Dumez]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 404996 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404996&action=review
> 
> I think this is good, but this is a bad time to make a change like this. 
> Could this wait until after the next branch?

I guess it can but it is merely aligning iOS with shipping macOS. 

> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424
> > +            RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]);
> 
> auto
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459
> > +    setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo);
> 
> @[cookie]

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> (In reply to Alex Christensen from comment #9)
> > Comment on attachment 404996 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=404996&action=review
> > 
> > I think this is good, but this is a bad time to make a change like this. 
> > Could this wait until after the next branch?
> 
> I guess it can but it is merely aligning iOS with shipping macOS. 

It also only impact document.cookie setting.

> 
> > 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424
> > > +            RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]);
> > 
> > auto
> > 
> > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459
> > > +    setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo);
> > 
> > @[cookie]

[Chris Dumez]

Created attachment 405042 [details]
Patch

[EWS]

Committed r264943: <https://trac.webkit.org/changeset/264943>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405042 [details].

[Aakash Jain]

(In reply to EWS from comment #13)
> Committed r264943: <https://trac.webkit.org/changeset/264943>
http/tests/cookies/document-cookie-multiple-cookies.html is failing consistently since this commit, as also warned by EWS.

History: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fcookies%2Fdocument-cookie-multiple-cookies.html

[Aakash Jain]

e.g.: https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean-tree/results.html

-PASS document.cookie is "samesite-unspecified=0"
+FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1; samesite-strict=2; samesite-unspecified=0.

[Chris Dumez]

(In reply to Aakash Jain from comment #15)
> e.g.:
> https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean-
> tree/results.html
> 
> -PASS document.cookie is "samesite-unspecified=0"
> +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1;
> samesite-strict=2; samesite-unspecified=0.

Ok. Windows still has the bug. I only fixed it for iOS.

[Chris Dumez]

(In reply to Chris Dumez from comment #16)
> (In reply to Aakash Jain from comment #15)
> > e.g.:
> > https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean-
> > tree/results.html
> > 
> > -PASS document.cookie is "samesite-unspecified=0"
> > +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1;
> > samesite-strict=2; samesite-unspecified=0.
> 
> Ok. Windows still has the bug. I only fixed it for iOS.

Fixing the same bug on Windows via Bug 214880.