[WKHTTPCookieStore getAllCookies:] may return duplicate cookies

[Chris Dumez]

[WKHTTPCookieStore getAllCookies:] may return duplicate cookies.

[Chris Dumez]

<rdar://problem/46010232>

[Chris Dumez]

Created attachment 371543 [details]
Patch

[Chris Dumez]

Reported at WWDC today.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

(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.

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 371606 [details]
Patch

[Chris Dumez]

(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

[Ryosuke Niwa]

Comment on attachment 371606 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> [phone]  Sihui Liu  <[e-mail]>

Change log duplicate!

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:-198
> -    ASSERT(!name.isHashTableDeletedValue());

I don't think this is right.

[Sihui Liu]

Created attachment 371641 [details]
Patch

[Sihui Liu]

Created attachment 371643 [details]
Patch

[Ryosuke Niwa]

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?

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 371749 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 371749 [details]
Patch for landing

Clearing flags on attachment: 371749

Committed r246264: <https://trac.webkit.org/changeset/246264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.