Bug 198635 - [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
Summary: [WKHTTPCookieStore getAllCookies:] may return duplicate cookies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-06 17:22 PDT by Chris Dumez
Modified: 2019-06-10 09:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.50 KB, patch)
2019-06-06 17:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (14.26 MB, application/zip)
2019-06-06 20:56 PDT, Build Bot
no flags Details
Patch (8.16 KB, patch)
2019-06-07 13:42 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2019-06-07 19:36 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2019-06-07 19:40 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (7.77 KB, patch)
2019-06-10 08:57 PDT, Sihui Liu
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 2019-06-06 17:22:45 PDT
[WKHTTPCookieStore getAllCookies:] may return duplicate cookies.
Comment 1 Chris Dumez 2019-06-06 17:22:59 PDT
<rdar://problem/46010232>
Comment 2 Chris Dumez 2019-06-06 17:29:30 PDT
Created attachment 371543 [details]
Patch
Comment 3 Chris Dumez 2019-06-06 17:30:04 PDT
Reported at WWDC today.
Comment 4 Build Bot 2019-06-06 20:56:29 PDT
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
Comment 5 Build Bot 2019-06-06 20:56:32 PDT
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
Comment 6 Chris Dumez 2019-06-06 21:02:03 PDT
(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 7 Sihui Liu 2019-06-07 13:12:20 PDT
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.
Comment 8 Sihui Liu 2019-06-07 13:42:07 PDT
Created attachment 371606 [details]
Patch
Comment 9 Chris Dumez 2019-06-07 13:51:50 PDT
(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 10 Ryosuke Niwa 2019-06-07 19:24:22 PDT
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.
Comment 11 Sihui Liu 2019-06-07 19:36:14 PDT
Created attachment 371641 [details]
Patch
Comment 12 Sihui Liu 2019-06-07 19:40:21 PDT
Created attachment 371643 [details]
Patch
Comment 13 Ryosuke Niwa 2019-06-07 19:45:29 PDT
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?
Comment 14 Sihui Liu 2019-06-10 08:38:01 PDT
(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.
Comment 15 Sihui Liu 2019-06-10 08:57:59 PDT
Created attachment 371749 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2019-06-10 09:16:58 PDT
Comment on attachment 371749 [details]
Patch for landing

Clearing flags on attachment: 371749

Committed r246264: <https://trac.webkit.org/changeset/246264>
Comment 17 WebKit Commit Bot 2019-06-10 09:17:00 PDT
All reviewed patches have been landed.  Closing bug.