Bug 184938 – -[WKHTTPCookieStore deleteCookie:completionHandler:] doesn't delete cookies

• Component: WebKit API
• Status: RESOLVED FIXED

[Sihui Liu]

There is a method "deleteCookie:completionHandler:" which should delete the specified cookie, but it's not reliable and cookies are not being deleted.
https://developer.apple.com/documentation/webkit/wkhttpcookiestore/2882009-deletecookie

[Sihui Liu]

<rdar://problem/34737395>

[Sihui Liu]

Created attachment 338680 [details]
Patch

[Sam Weinig]

Seems like this should have an API test.

[Daniel Bates]

Comment on attachment 338680 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        converted to NSHTTPCooike, this property information was lost, and delete function cannot 

NSHTTPCooike => NSHTTPCookie

> Source/WebCore/ChangeLog:14
> +        As existing APIs do not provide a way to create a HTTPOnly cookie, I tested it manually.

Are there any SPIs that we can use to create such a cookie? Maybe this is a dumb question, can we create a custom protocol handler for HTTP and then fabricate a response that includes a Set-Cookie header?

[Daniel Bates]

(In reply to Daniel Bates from comment #4)
> Maybe this is a dumb question, can we create a custom protocol handler for HTTP and then fabricate a response that includes a Set-Cookie header?

This will not work according to Sihui Liu as custom protocol responses are not processed the same way as HTTP responses by CFNetwork.

[Sihui Liu]

Created attachment 338837 [details]
Patch

[Sihui Liu]

Created attachment 338846 [details]
Patch

[EWS Watchlist]

Comment on attachment 338846 [details]
Patch

Attachment 338846 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7466115

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 338867 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Geoffrey Garen]

Comment on attachment 338846 [details]
Patch

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

r=me

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:70
> +    NSArray *nsCookies = [nsCookieStorage() cookies];
> +    for (NSHTTPCookie *nsCookie in nsCookies) {
> +        if (Cookie(nsCookie) == cookie) {
> +            [nsCookieStorage() deleteCookie:nsCookie];
> +            break;
> +        }
> +    }

Unlike the change to operator== above, which is a nice improvement that we would do intentionally, this change is a purely a workaround, which we'd rather not preserve. So, please do follow up and fix the Cookie => NSHTTPCookie conversion path, and when you do, you can remove this workaround.

[WebKit Commit Bot]

Comment on attachment 338846 [details]
Patch

Clearing flags on attachment: 338846

Committed r231067: <https://trac.webkit.org/changeset/231067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 338846 [details]
Patch

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

> Source/WebCore/platform/network/cocoa/CookieCocoa.mm:146
> +    return name == other.name
> +    && value == other.value
> +    && domain == other.domain
> +    && path == other.path
> +    && created == other.created
> +    && expires == other.expires
> +    && httpOnly == other.httpOnly
> +    && secure == other.secure
> +    && session == other.session
> +    && comment == other.comment
> +    && commentURL == other.commentURL
> +    && ports == other.ports;

This change makes equality testing asymmetric with the hash algorithm as we perform equality testing ourself and ask NSHTTPCookie to compute a hash. The test case included in this patch demonstrates that we know how to set the HttpOnly bit for an NSHTTPCookie; => we can fix the FIXME operator NSHTTPCookie *(). Would it be sufficient to fix this fix me and then revert the change in this function to fix this bug? The benefit of fixing the FIXME and then using -[NSHTTPCookie equal:] is that it makes this class effectively a wrapper for NSHTTPCookie (*) and means there is one less function to modify whenever we need to add a new cookie field.

(*) The class exists for the purpose of exposing a platform-independent interface to a cookie.