Bug 214880 – http/tests/cookies/document-cookie-multiple-cookies.html is failing on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

http/tests/cookies/document-cookie-multiple-cookies.html is failing on Windows:
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]

Created attachment 405368 [details]
Patch

[Chris Dumez]

Created attachment 405384 [details]
Patch

[Geoffrey Garen]

Comment on attachment 405384 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:201
> +    RetainPtr<CFStringRef> cookieStringCF = cookieString.createCFString();

Could be auto

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:231
> +    auto cookies = adoptCF(CFArrayCreate(kCFAllocatorDefault, 0, 0, &kCFTypeArrayCallBacks));

Don't you need CFArrayCreateMutable if you want to append? Seems like a bug.

Can we avoid my question about whether append is valid, and the cost of reallocation in append, by passing in { &cookie, 1 } (or syntactically valid equivalent) instead of { 0, 0 } here?

[Chris Dumez]

Created attachment 405390 [details]
Patch

[Geoffrey Garen]

Comment on attachment 405390 [details]
Patch

r=me

[Chris Dumez]

Created attachment 405398 [details]
Patch

[Chris Dumez]

Comment on attachment 405398 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:216
> +    return WTFMove(cookie);

Windows really does not like this return statement it seems. I tried without WTFMove() first and now with it. It is still failing. Not sure what's going on...

[Chris Dumez]

Created attachment 405417 [details]
Patch

[Chris Dumez]

Created attachment 405420 [details]
Patch

[Karl Rackler]

I have marked these tests as failing while this issue is investigated.
https://trac.webkit.org/changeset/265021/webkit

[Chris Dumez]

Created attachment 405429 [details]
Patch

[Chris Dumez]

(In reply to Karl Rackler from comment #10)
> I have marked these tests as failing while this issue is investigated.
> https://trac.webkit.org/changeset/265021/webkit

Thanks, I updated my patch to unskip it.

[Chris Dumez]

NetworkStorageSessionCFNetWin.obj : error LNK2019: unresolved external symbol __imp__CFHTTPCookieCreateWithStringAndPartition referenced in function "class WTF::RetainPtr<struct OpaqueCFHTTPCookie const *> __cdecl WebCore::parseDOMCookie(class WTF::String,struct __CFURL const *)" (?parseDOMCookie@WebCore@@YA?AV?$RetainPtr@PEBUOpaqueCFHTTPCookie@@@WTF@@VString@3@PEBU__CFURL@@@Z) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\bin64\TestWebCoreLib.dll : fatal error LNK1120: 1 unresolved externals [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj]

Looks like I may not be able to use __CFHTTPCookieCreateWithStringAndPartition() on Windows... I guess I'll keep using the old API but only keep the first cookie..

[Chris Dumez]

Created attachment 405433 [details]
Patch

[Darin Adler]

Comment on attachment 405433 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:223
> +    return cookie;

Getting a type mismatch on this line; maybe you need a cast somewhere to CFHTTPCookieRef?

[Chris Dumez]

Created attachment 405438 [details]
Patch

[Chris Dumez]

Created attachment 405465 [details]
Patch

[Darin Adler]

Comment on attachment 405465 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:211
> +    auto cookie = static_cast<CFHTTPCookieRef>(CFArrayGetValueAtIndex(parsedCookies.get(), 0));

Idle unimportant thought: I wonder if we can use checked_cf_cast instead of static_cast. I think it’s designed for uses like this one.

[Chris Dumez]

Created attachment 405467 [details]
Patch

[Darin Adler]

Comment on attachment 405467 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:229
> +    UNUSED_PARAM(frameID);
> +    UNUSED_PARAM(pageID);

Could use "omit the name" style for unused arguments instead of UNUSED_PARAM style.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:239
> +    auto cookies = adoptCF(CFArrayCreateMutable(0, 1, &kCFTypeArrayCallBacks));
> +    CFArrayAppendValue(cookies.get(), cookie.get());

Not important to optimize, but this could be done more efficiently with CFArrayCreate. Like we do above for CFDictionaryCreate, although it can be done with less type casting by defining local arrays.

[Chris Dumez]

Created attachment 405471 [details]
Patch

[EWS]

Committed r265048: <https://trac.webkit.org/changeset/265048>

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

[Radar WebKit Bug Importer]

<rdar://problem/66283757>