Bug 55086 – WebKit2: Need a way to manage cookies from the web process

• Component: WebKit2
• Status: RESOLVED FIXED

[Brian Weinstein]

We need a way to manage cookies from the Web Process in WebKit2.

We need to be able to:
1) Get a list of origins that have cookies.
2) Remove all cookies for a given origin.
3) Remove all cookies.

<rdar://problem/8971738>

[Brian Weinstein]

Created attachment 83553 [details]
[PATCH] Fix

[WebKit Review Bot]

Attachment 83553 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebCookieManagerProxy.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 83553 [details]
[PATCH] Fix

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

You should probably let the EWS chew on this a bit. Looks nice!

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:63
> +    if (!m_webContext->hasValidProcess()) {

Seems like you need to null-check m_webContext here and everywhere else you use it, since clearContext could have been called. Or, if it couldn't have been called, you should assert that m_webContext is non-null.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:79
> +void WebCookieManagerProxy::deleteCookiesForOrigin(WebSecurityOrigin* origin)

Can that be a const WebSecurityOrigin*?

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:35
> +#include "APIObject.h"
> +#include "GenericCallback.h"
> +#include "ImmutableArray.h"
> +
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefPtr.h>
> +#include <wtf/Vector.h>

We don't normally separate #includes like this.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:47
> +struct SecurityOriginData;
> +class WebContext;
> +class WebSecurityOrigin;

I personally like to put struct and class declarations in separate paragraphs, but I don't know if that's our prevailing style.

> Source/WebKit2/UIProcess/API/C/WKCookieManager.cpp:52
> +void WKCookieManagerGetCookieOrigins(WKCookieManagerRef cookieManagerRef, void* context, WKCookieManagerGetCookieOriginsFunction callback)
> +{
> +    toImpl(cookieManagerRef)->getCookieOrigins(ArrayCallback::create(context, callback));
> +}
> +
> +void WKCookieManagerDeleteCookiesForOrigin(WKCookieManagerRef cookieManagerRef, WKSecurityOriginRef originRef)
> +{
> +    toImpl(cookieManagerRef)->deleteCookiesForOrigin(toImpl(originRef));
> +}
> +
> +void WKCookieManagerDeleteAllCookies(WKCookieManagerRef cookieManagerRef)
> +{
> +    toImpl(cookieManagerRef)->deleteAllCookies();
> +}

I don't really see the point of all the "Ref" suffixes, though I know that's what we do elsewhere.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:42
> +    static WebCookieManager& shared = *new WebCookieManager;

DEFINE_STATIC_LOCAL would be better.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:59
> +    // FIXME <rdar://problem/8971738>: Implement.

Huh? This function looks pretty implemented to the uninitiated. You should say what is still missing here.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:65
> +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>::iterator i = origins.begin();

It's weird to use "i" as the name of an iterator. We normally use "it".

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:67
> +        RefPtr<SecurityOrigin> origin = *i;

This could be a const RefPtr& to save an extra ref/deref.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:74
> +        SecurityOriginData originData;
> +        originData.protocol = origin->protocol();
> +        originData.host = origin->host();
> +        originData.port = origin->port();
> +
> +        identifiers.uncheckedAppend(originData);

If we had a SecurityOriginData constructor that took a single const RefPtr<SecurityOrigin>& argument, you could just use copyKeysToVector instead of writing this loop manually. Such a constructor would need to be marked "explicit", and would probably warrant a comment about why we have it (since we would normally just take a bare SecurityOrigin*). You might want to post a new patch if you do this, or do it separately.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:82
> +    // FIXME <rdar://problem/8971738>: Implement.

A notImplemented() call would be nice. It would also be nice if we had a Bugzilla bug to reference instead of a silly private Radar.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:87
> +    // FIXME <rdar://problem/8971738>: Implement.

Ditto.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:45
> +    WTF_MAKE_NONCOPYABLE(WebCookieManager);
> +
> +public:

No need for the blank line here.

[Brian Weinstein]

I'm going to send out a new patch that addresses these comments + switches the passing of cookies across the wire to use hostnames instead of SecurityOrigins.

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 83553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83553&action=review
> 
> You should probably let the EWS chew on this a bit. Looks nice!
> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:63
> > +    if (!m_webContext->hasValidProcess()) {
> 
> Seems like you need to null-check m_webContext here and everywhere else you use it, since clearContext could have been called. Or, if it couldn't have been called, you should assert that m_webContext is non-null.

Added ASSERTS. We have this code style a lot, and asserts are valuable here, because if we hit an assert here (which I doubt), it would be useful information to have.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:79
> > +void WebCookieManagerProxy::deleteCookiesForOrigin(WebSecurityOrigin* origin)
> 
> Can that be a const WebSecurityOrigin*?

Made it a string.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.h:35
> > +#include "APIObject.h"
> > +#include "GenericCallback.h"
> > +#include "ImmutableArray.h"
> > +
> > +#include <wtf/PassRefPtr.h>
> > +#include <wtf/RefPtr.h>
> > +#include <wtf/Vector.h>
> 
> We don't normally separate #includes like this.

Fixed.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.h:47
> > +struct SecurityOriginData;
> > +class WebContext;
> > +class WebSecurityOrigin;
> 
> I personally like to put struct and class declarations in separate paragraphs, but I don't know if that's our prevailing style.

Removed the SecurityOrigin information.

> 
> > Source/WebKit2/UIProcess/API/C/WKCookieManager.cpp:52
> > +void WKCookieManagerGetCookieOrigins(WKCookieManagerRef cookieManagerRef, void* context, WKCookieManagerGetCookieOriginsFunction callback)
> > +{
> > +    toImpl(cookieManagerRef)->getCookieOrigins(ArrayCallback::create(context, callback));
> > +}
> > +
> > +void WKCookieManagerDeleteCookiesForOrigin(WKCookieManagerRef cookieManagerRef, WKSecurityOriginRef originRef)
> > +{
> > +    toImpl(cookieManagerRef)->deleteCookiesForOrigin(toImpl(originRef));
> > +}
> > +
> > +void WKCookieManagerDeleteAllCookies(WKCookieManagerRef cookieManagerRef)
> > +{
> > +    toImpl(cookieManagerRef)->deleteAllCookies();
> > +}
> 
> I don't really see the point of all the "Ref" suffixes, though I know that's what we do elsewhere.

I'll keep it like this for now.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:42
> > +    static WebCookieManager& shared = *new WebCookieManager;
> 
> DEFINE_STATIC_LOCAL would be better.

Fixed.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:59
> > +    // FIXME <rdar://problem/8971738>: Implement.
> 
> Huh? This function looks pretty implemented to the uninitiated. You should say what is still missing here.
> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:65
> > +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>::iterator i = origins.begin();
> 
> It's weird to use "i" as the name of an iterator. We normally use "it".
> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:67
> > +        RefPtr<SecurityOrigin> origin = *i;
> 
> This could be a const RefPtr& to save an extra ref/deref.
> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:74
> > +        SecurityOriginData originData;
> > +        originData.protocol = origin->protocol();
> > +        originData.host = origin->host();
> > +        originData.port = origin->port();
> > +
> > +        identifiers.uncheckedAppend(originData);
> 
> If we had a SecurityOriginData constructor that took a single const RefPtr<SecurityOrigin>& argument, you could just use copyKeysToVector instead of writing this loop manually. Such a constructor would need to be marked "explicit", and would probably warrant a comment about why we have it (since we would normally just take a bare SecurityOrigin*). You might want to post a new patch if you do this, or do it separately.


I cleaned this all up since I am using Strings, now it is just copyKeysToVector.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:82
> > +    // FIXME <rdar://problem/8971738>: Implement.
> 
> A notImplemented() call would be nice. It would also be nice if we had a Bugzilla bug to reference instead of a silly private Radar.

Fixed.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:87
> > +    // FIXME <rdar://problem/8971738>: Implement.
> 
> Ditto.

Ditto.

> 
> > Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:45
> > +    WTF_MAKE_NONCOPYABLE(WebCookieManager);
> > +
> > +public:
> 
> No need for the blank line here.

Fixed.

[Brian Weinstein]

Created attachment 83575 [details]
[PATCH] Fix v2

[WebKit Review Bot]

Attachment 83575 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebCookieManagerProxy.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jessie Berlin]

(In reply to comment #6)
> Created an attachment (id=83575) [details]
> [PATCH] Fix v2

I am not thrilled with getCookieHostnames. It implies that it is getting the hostnames for a single cookie. Maybe something like "getHostnamesWithCookies" would be better?

[Brian Weinstein]

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=83575) [details] [details]
> > [PATCH] Fix v2
> 
> I am not thrilled with getCookieHostnames. It implies that it is getting the hostnames for a single cookie. Maybe something like "getHostnamesWithCookies" would be better?

Sure, fixed.

[Brian Weinstein]

Created attachment 83585 [details]
[PATCH] Fix v3 (Renaming)

[WebKit Review Bot]

Attachment 83585 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebCookieManagerProxy.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 83585 [details]
[PATCH] Fix v3 (Renaming)

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

Looks great. Maybe Jessie should take a look before you land, since she caught the origin vs. hostname issue earlier.

> Source/WebKit2/ChangeLog:56
> +        (WebKit::WebCookieManagerProxy::getHostnamesWithCookies): Call through to the web process to get origins with cookies.

s/origins/hostnames/

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:79
> +        // FIXME: Log error or assert.

Why leave this as a FIXME? Why not just do it?

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:88
> +    Vector<RefPtr<APIObject> > hostnames;
> +    hostnames.reserveCapacity(hostnameCount);
> +
> +    for (size_t i = 0; i < hostnameCount; ++i)
> +        hostnames.uncheckedAppend(WebString::create(hostnameList[i]));

Another nearly-equivalent option is:

Vector<RefPtr<APIObject> > hostnames(hostnameCount);
for (size_t i = 0; i < hostnameCount; ++i)
    hostnames[i] = WebString::create(hostnameList[i]);

I think that would be ever-so-slightly clearer. Maybe ever-so-slightly more efficient, too.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:59
> +    void deleteCookiesForHostname(String hostname);

Should be a const String&.

[Brian Weinstein]

(In reply to comment #12)
> (From update of attachment 83585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83585&action=review
> 
> Looks great. Maybe Jessie should take a look before you land, since she caught the origin vs. hostname issue earlier.
> 
> > Source/WebKit2/ChangeLog:56
> > +        (WebKit::WebCookieManagerProxy::getHostnamesWithCookies): Call through to the web process to get origins with cookies.
> 
> s/origins/hostnames/
> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:79
> > +        // FIXME: Log error or assert.
> 
> Why leave this as a FIXME? Why not just do it?

This FIXME has been in a lot of places, and I'm not sure what kind of logging or asserting should be there. I will follow-up with Sam or Anders and ask then what we should do there, and fix all of the FIXMEs.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:88
> > +    Vector<RefPtr<APIObject> > hostnames;
> > +    hostnames.reserveCapacity(hostnameCount);
> > +
> > +    for (size_t i = 0; i < hostnameCount; ++i)
> > +        hostnames.uncheckedAppend(WebString::create(hostnameList[i]));
> 
> Another nearly-equivalent option is:
> 
> Vector<RefPtr<APIObject> > hostnames(hostnameCount);
> for (size_t i = 0; i < hostnameCount; ++i)
>     hostnames[i] = WebString::create(hostnameList[i]);
> 
> I think that would be ever-so-slightly clearer. Maybe ever-so-slightly more efficient, too.

When I had the uncheckedAppend, it was using SecurityOrigins which had the possibility of being null. With strings that doesn't make much sense. Fixed.

> 
> > Source/WebKit2/UIProcess/WebCookieManagerProxy.h:59
> > +    void deleteCookiesForHostname(String hostname);
> 
> Should be a const String&.

Fixed.

[Jessie Berlin]

The latest patch looks good to me.

[Brian Weinstein]

Landed WebCookieManager (that doesn't do anything yet) in r79585. More functionality will be added to it as part of this bug.

[Brian Weinstein]

Created attachment 83859 [details]
[PATCH] Cookie Managing

[Jessie Berlin]

Comment on attachment 83859 [details]
[PATCH] Cookie Managing

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

> Source/WebCore/platform/mac/CookieJar.mm:218
> +    

Did you mean to implement this?

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:243
> +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;

I think it is better to have an early return here if !cookiesCF

> Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:258
> +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;

Ditto.

[Jessie Berlin]

(In reply to comment #17)
> (From update of attachment 83859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83859&action=review
> 
> > Source/WebCore/platform/mac/CookieJar.mm:218
> > +    
> 
> Did you mean to implement this?
> 
> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:243
> > +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;
> 
> I think it is better to have an early return here if !cookiesCF
> 
> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:258
> > +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;
> 
> Ditto.

Note: I don't have review privileges.

[Brian Weinstein]

(In reply to comment #17)
> (From update of attachment 83859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83859&action=review
> 
> > Source/WebCore/platform/mac/CookieJar.mm:218
> > +    
> 
> Did you mean to implement this?

I did. Oops.

> 
> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:243
> > +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;
> 
> I think it is better to have an early return here if !cookiesCF

I agree. Fixed.

> 
> > Source/WebCore/platform/network/cf/CookieJarCFNet.cpp:258
> > +    CFIndex count = cookiesCF ? CFArrayGetCount(cookiesCF.get()) : 0;
> 
> Ditto.

Fixed.

New patch coming soon!

[Brian Weinstein]

Created attachment 83863 [details]
[PATCH] Cookie Managing v2

[Jessie Berlin]

Comment on attachment 83863 [details]
[PATCH] Cookie Managing v2

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

Looks good to me overall. Again, I don't have review privileges.

> Source/WebCore/ChangeLog:12
> +        No change in behavior.

It took me a moment to realize this referred to not needing new tests. You should mention tests here, maybe something like "No change in behavior needing tests".

You probably could write WebKit2 API tests for this. Maybe file a bug about adding those tests?

> Source/WebCore/platform/mac/CookieJar.mm:222
> +    

Extra space.

[Brady Eidson]

Comment on attachment 83863 [details]
[PATCH] Cookie Managing v2

r+ but fix the space Jessie mentioned.

[Brian Weinstein]

Landed the rest in r79722.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79722 might have broken GTK Linux 32-bit Debug