Bug 214880 - http/tests/cookies/document-cookie-multiple-cookies.html is failing on Windows
Summary: http/tests/cookies/document-cookie-multiple-cookies.html is failing on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-28 09:47 PDT by Chris Dumez
Modified: 2020-07-29 11:31 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.24 KB, patch)
2020-07-28 09:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2020-07-28 11:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2020-07-28 12:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2020-07-28 13:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.56 KB, patch)
2020-07-28 14:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.36 KB, patch)
2020-07-28 15:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.98 KB, patch)
2020-07-28 17:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.69 KB, patch)
2020-07-28 18:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2020-07-28 22:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2020-07-29 09:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2020-07-29 09:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2020-07-29 10:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-07-28 09:47:01 PDT
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.
Comment 1 Chris Dumez 2020-07-28 09:59:15 PDT
Created attachment 405368 [details]
Patch
Comment 2 Chris Dumez 2020-07-28 11:46:49 PDT
Created attachment 405384 [details]
Patch
Comment 3 Geoffrey Garen 2020-07-28 12:07:11 PDT
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?
Comment 4 Chris Dumez 2020-07-28 12:19:42 PDT
Created attachment 405390 [details]
Patch
Comment 5 Geoffrey Garen 2020-07-28 13:05:16 PDT
Comment on attachment 405390 [details]
Patch

r=me
Comment 6 Chris Dumez 2020-07-28 13:13:28 PDT
Created attachment 405398 [details]
Patch
Comment 7 Chris Dumez 2020-07-28 14:35:43 PDT
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...
Comment 8 Chris Dumez 2020-07-28 14:42:59 PDT
Created attachment 405417 [details]
Patch
Comment 9 Chris Dumez 2020-07-28 15:13:55 PDT
Created attachment 405420 [details]
Patch
Comment 10 Karl Rackler 2020-07-28 17:12:25 PDT
I have marked these tests as failing while this issue is investigated.
https://trac.webkit.org/changeset/265021/webkit
Comment 11 Chris Dumez 2020-07-28 17:14:20 PDT
Created attachment 405429 [details]
Patch
Comment 12 Chris Dumez 2020-07-28 17:15:00 PDT
(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.
Comment 13 Chris Dumez 2020-07-28 18:19:40 PDT
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..
Comment 14 Chris Dumez 2020-07-28 18:48:29 PDT
Created attachment 405433 [details]
Patch
Comment 15 Darin Adler 2020-07-28 19:05:04 PDT
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?
Comment 16 Chris Dumez 2020-07-28 22:06:51 PDT
Created attachment 405438 [details]
Patch
Comment 17 Chris Dumez 2020-07-29 09:39:02 PDT
Created attachment 405465 [details]
Patch
Comment 18 Darin Adler 2020-07-29 09:42:01 PDT
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.
Comment 19 Chris Dumez 2020-07-29 09:45:46 PDT
Created attachment 405467 [details]
Patch
Comment 20 Darin Adler 2020-07-29 10:07:57 PDT
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.
Comment 21 Chris Dumez 2020-07-29 10:16:44 PDT
Created attachment 405471 [details]
Patch
Comment 22 EWS 2020-07-29 11:30:19 PDT
Committed r265048: <https://trac.webkit.org/changeset/265048>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405471 [details].
Comment 23 Radar WebKit Bug Importer 2020-07-29 11:31:16 PDT
<rdar://problem/66283757>