Bug 147282 - WKWebsiteDataStore remove methods don't properly delete cookies
Summary: WKWebsiteDataStore remove methods don't properly delete cookies
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anders Carlsson
Depends on:
Reported: 2015-07-24 16:03 PDT by Anders Carlsson
Modified: 2015-07-26 20:53 PDT (History)
1 user (show)

See Also:

Patch (12.92 KB, patch)
2015-07-24 16:08 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (12.93 KB, patch)
2015-07-24 16:15 PDT, Anders Carlsson
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2015-07-24 16:03:48 PDT
WKWebsiteDataStore remove methods don't properly delete cookies
Comment 1 Anders Carlsson 2015-07-24 16:08:34 PDT
Created attachment 257488 [details]
Comment 2 Anders Carlsson 2015-07-24 16:15:05 PDT
Created attachment 257489 [details]
Comment 3 Anders Carlsson 2015-07-24 17:34:25 PDT
Committed r187375: <http://trac.webkit.org/changeset/187375>
Comment 4 Darin Adler 2015-07-26 20:53:30 PDT
Comment on attachment 257489 [details]

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

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:240
> +void deleteCookiesForHostnames(const NetworkStorageSession& session, const Vector<String>& hostnames)
> +{
> +}
> +
>  void deleteAllCookiesModifiedSince(const NetworkStorageSession&, std::chrono::system_clock::time_point)
>  {
>  }

Seems kind of lame that these don’t even have a comment about the fact that they are not implemented and do nothing! Also would be nice to leave out the unused argument names to make this build with unused argument warnings.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:230
> +    HashMap<String, RetainPtr<NSHTTPCookie>> cookiesByDomain;

Is there a need for RetainPtr here? Doesn’t the cookies array retains all the cookies in that array? Or maybe that array is an internal data structure that changes once you start calling wkDeleteHTTPCookie?

> Source/WebCore/platform/network/mac/CookieJarMac.mm:232
> +        cookiesByDomain.set(cookie.domain, cookie);

Why set instead of add?

> Source/WebCore/platform/network/mac/CookieJarMac.mm:234
> +    for (const auto& hostname : hostnames) {

Not sure that const here is all that useful.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:239
> +    [WebCore::cookieStorage(session) _saveCookies];

Seems to me that the use of CFHTTPCookieStorageRef to get the list of cookies but the use of NSHTTPCookieStorage for the call to _saveCookies is peculiar enough that it needs a motivating comment.