| Summary: | Implement Same-Site cookies | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Enhancement | CC: | achristensen, ap, beidson, berto, bfulgham, cdumez, cgarcia, commit-queue, craig+webkit, dbates, esprehn+autocc, ews-watchlist, galpeter, gustavo, Hironori.Fujii, japhet, kangil.han, mcatanzaro, m.kurz+webkitbugs, rniwa, ryanhaddad, steffen.weber, teppeis, webkit-bug-importer, wilander, youennf | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=184955 https://bugs.webkit.org/show_bug.cgi?id=204742 |
||||||||||||||||||
| Bug Depends on: | 184903 | ||||||||||||||||||
| Bug Blocks: | 184894, 184897, 187197 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Mike West
2016-07-06 05:29:41 PDT
John, Brent: I know cookies are outside your purview, but perhaps you could triage this to the right folks? I've started putting together a test suite at http://rfc6265.biz/tests/, which might be helpful if y'all start taking a closer look at this. (In reply to Mike West from comment #3) > I've started putting together a test suite at http://rfc6265.biz/tests/, > which might be helpful if y'all start taking a closer look at this. The tests seem to be broken. The first four tests either all fail or timeout when run in Chrome Canary 68.0.3398.0. I did not continue to test the other tests linked off of this site given the success of the first four. (In reply to John Wilander from comment #2) > rdar://problem/27196358 This was duped to... Created attachment 338163 [details]
Patch and layout tests
Comment on attachment 338163 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338163&action=review > Source/WebCore/loader/CookieJar.cpp:65 > + const auto& request = document.loader()->request(); > + return SameSiteInfo::create(request); Will inline line 64 into line 65 such that this reads: return SameSiteInfo::create(document.loader()->request()); Attachment 338163 [details] did not pass style-queue:
ERROR: LayoutTests/TestExpectations:776: Path does not exist. [test/expectations] [5]
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/TestExpectations:776: Path does not exist. [test/expectations] [5]
Total errors found: 3 in 105 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338166 [details]
Patch and layout tests
Attachment 338166 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 105 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338174 [details]
Patch and layout tests
Attachment 338174 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 107 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338175 [details]
Patch and layout tests
Attachment 338175 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 107 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338191 [details]
Patch and layout tests
Attachment 338191 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 111 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 338197 [details]
Patch and layout tests
Attachment 338197 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4]
Total errors found: 1 in 111 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 338197 [details] Patch and layout tests Attachment 338197 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7355865 New failing tests: http/tests/preload/onload_event.html Created attachment 338209 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 338197 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review This looks reasonable. Is it safe to land for internal machines? > Source/WebCore/loader/CookieJar.cpp:64 > + return SameSiteInfo::create(document.loader()->request()); If we are inlining, might this be better? if (auto loader = document.loader()) return SameSiteInfo::create(loader->request()); return { }] > Source/WebCore/platform/network/mac/CookieJarMac.mm:-151 > - }]; Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue? Comment on attachment 338197 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review >> Source/WebCore/loader/CookieJar.cpp:64 >> + return SameSiteInfo::create(document.loader()->request()); > > If we are inlining, might this be better? > > if (auto loader = document.loader()) > return SameSiteInfo::create(loader->request()); > > return { }] Will change code before landing to match your suggestion excluding the empty line above "return { }" as I do not feel this improves the readability of the code, adding a * after auto for clarity, and substituting a ';' for ']': if (auto* loader = document.loader()) return SameSiteInfo::create(loader->request()); return { }; >> Source/WebCore/platform/network/mac/CookieJarMac.mm:-151 >> - }]; > > Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue? There is no behavior change. Notice that cookiesForURL is overloaded and I moved this code to cookiesForURL(NSHTTPCookieStorage *, NSURL *, NSURL *, const std::optional<SameSiteInfo>&, NSString *partition). Maybe your remark indicates confusion with having two cookiesForURL()s? I am open to naming suggestions (or other ideas) to improve the clarity of the code and remove the need to overload cookiesForURL(). (In reply to Brent Fulgham from comment #22) > Comment on attachment 338197 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338197&action=review > > [...] Is it safe to land for internal machines? > Builds and runs on my Apple Internal machine :) Committed r230921: <https://trac.webkit.org/changeset/230921> This change broke 32-bit macOS builds: ./platform/network/mac/CookieJarMac.mm:101:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertySiteForCookies' ./platform/network/mac/CookieJarMac.mm:102:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertyIsTopLevelNavigation' https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%2832-bit%20Build%29/builds/5399 (In reply to Ryan Haddad from comment #26) > This change broke 32-bit macOS builds: Correction: It broke all the builds. (In reply to Ryan Haddad from comment #27) > (In reply to Ryan Haddad from comment #26) > > This change broke 32-bit macOS builds: > > Correction: It broke all the builds. Committed build fixes in <https://trac.webkit.org/changeset/230923> and <https://trac.webkit.org/changeset/230924> Re-opened since this is blocked by bug 184903 Here were the remaining failures after the last build fix attempt: https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/10535 https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/8989 https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/8584 Committed r230944: <https://trac.webkit.org/changeset/230944> Committed ChangeLog fix in <https://trac.webkit.org/changeset/230945>. Committed build fix in <https://trac.webkit.org/changeset/230946> Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230947>. Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230948>. Committed r230981: <https://trac.webkit.org/changeset/230981> |