Summary: | [WKHTTPCookieStore getAllCookies:] may return duplicate cookies | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, commit-queue, ews-watchlist, ggaren, rniwa, sihui_liu, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2019-06-06 17:22:45 PDT
Created attachment 371543 [details]
Patch
Reported at WWDC today. Comment on attachment 371543 [details] Patch Attachment 371543 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12401401 New failing tests: fast/shadow-dom/svg-use-href-change-in-shadow-tree.html storage/indexeddb/index-cursor.html imported/blink/fast/canvas/bug382588.html Created attachment 371554 [details]
Archive of layout-test-results from ews211 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Build Bot from comment #4) > Comment on attachment 371543 [details] > Patch > > Attachment 371543 [details] did not pass win-ews (win): > Output: https://webkit-queues.webkit.org/results/12401401 > > New failing tests: > fast/shadow-dom/svg-use-href-change-in-shadow-tree.html > storage/indexeddb/index-cursor.html > imported/blink/fast/canvas/bug382588.html No way these are related to this patch as it only impacts WK2. Comment on attachment 371543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371543&action=review > Source/WebKit/ChangeLog:12 > + the same name, as long as at least of of their properties (e.g. value) differs. This seems not quite right. Cookies with the same name but on different domains should be treated as different cookies. Created attachment 371606 [details]
Patch
(In reply to Sihui Liu from comment #7) > Comment on attachment 371543 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371543&action=review > > > Source/WebKit/ChangeLog:12 > > + the same name, as long as at least of of their properties (e.g. value) differs. > > This seems not quite right. Cookies with the same name but on different > domains should be treated as different cookies. That is a very good point :D Comment on attachment 371606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371606&action=review > Source/WebCore/ChangeLog:16 > +2019-06-07 Sihui Liu <sihui_liu@apple.com> Change log duplicate! > Source/WebCore/platform/network/cocoa/CookieCocoa.mm:-198 > - ASSERT(!name.isHashTableDeletedValue()); I don't think this is right. Created attachment 371641 [details]
Patch
Created attachment 371643 [details]
Patch
Comment on attachment 371643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371643&action=review > Source/WebKit/ChangeLog:15 > + You should explain that the other case where we need to do a different comparison check. > Source/WebCore/platform/Cookie.h:181 > + static const bool emptyValueIsZero = false; > + static const bool hasIsEmptyValueFunction = true; Do we really need to define both? I thought defining hasIsEmptyValueFunction was enough. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1944 > + for (auto& pendingCookie : pendingCookies()) { Just use m_pendingCookies.removeIf instead of creating a temporary Vector? (In reply to Ryosuke Niwa from comment #13) > Comment on attachment 371643 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371643&action=review > > > Source/WebKit/ChangeLog:15 > > + > > You should explain that the other case where we need to do a different > comparison check. > Will add. > > Source/WebCore/platform/Cookie.h:181 > > + static const bool emptyValueIsZero = false; > > + static const bool hasIsEmptyValueFunction = true; > > Do we really need to define both? I thought defining hasIsEmptyValueFunction > was enough. > Yeees. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1944 > > + for (auto& pendingCookie : pendingCookies()) { > > Just use m_pendingCookies.removeIf instead of creating a temporary Vector? Okay. Created attachment 371749 [details]
Patch for landing
Comment on attachment 371749 [details] Patch for landing Clearing flags on attachment: 371749 Committed r246264: <https://trac.webkit.org/changeset/246264> All reviewed patches have been landed. Closing bug. |