[Webkit-unassigned] [Bug 177932] [GTK] New API to add, retrieve and delete cookies via WebKitCookieManager

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Nov 18 13:26:51 PST 2017


https://bugs.webkit.org/show_bug.cgi?id=177932

Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #326469|0                           |1
        is obsolete|                            |
 Attachment #327321|                            |review?
              Flags|                            |

--- Comment #20 from Mario Sanchez Prada <mario at webkit.org> ---
Created attachment 327321

  --> https://bugs.webkit.org/attachment.cgi?id=327321&action=review

Patch proposal

Thanks for the review Carlos. I see you marked the patch as r+ but, since you suggested quite some changes and I'm adding one more on top (see the end of this comment), I'm attaching yet one more version of the patch where I addressed all your comments and I'm now asking for one more final review, hope you don't mind.

Now see my comments below...

(In reply to Carlos Garcia Campos from comment #18)
> Comment on attachment 326469 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326469&action=review
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:43
> > +Cookie::Cookie(SoupCookie* cookie)
> > +{
> > +    name = String::fromUTF8(cookie->name);
> > +    value = String::fromUTF8(cookie->value);
> > +    domain = String::fromUTF8(cookie->domain);
> > +    path = String::fromUTF8(cookie->path);
> > +    expires = cookie->expires ? static_cast<double>(soup_date_to_time_t(cookie->expires)) * 1000 : 0;
> > +    httpOnly = cookie->http_only;
> > +    secure = cookie->secure;
> > +    session = !cookie->expires;
> 
> Use the member initialization instead

Fixed.

> Cookie::Cookie(SoupCookie* cookie)
>     : name(String::fromUTF8(cookie->name))
>     , value(String::fromUTF8(cookie->value))
> ....
> 
> > Source/WebCore/platform/network/soup/CookieSoup.cpp:60
> > +    if (name.isNull() || value.isNull() || domain.isNull() || path.isNull())
> > +        return nullptr;
> 
> Could we use isNull()?

Not really, since Cookie::isNull() returns `false` only when *all* the internal attributes (name, domain, path, value, expired...) are nullptr/0/false, and in here I want to return nullptr if *either* of these four things are null, since otherwise I could not create a SoupCookie object.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:246
> > + * Asynchronously add a #SoupCookie to the underlying storage, which is not persistent
> > + * unless you explicitly call webkit_cookie_manager_set_persistent_storage().
> 
> Asynchronously add @cookie to @cookie_manager ? I don't think we need to
> clarify the persistance here

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:250
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:284
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:296
> > + * @uri: the #SoupURI for the domain of the cookies to be retrieved
> 
> We don't expose SoupURI in our API, better use char* and create the soupURI
> internally if needed.

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:302
> > + * Asynchronously get a list of #SoupCookie from @cookie_manager associated with @uri, which
> > + * must be either and HTTP or an HTTPS URL.
> 
> and -> an

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:306
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:327
> > +        GList* cookies_list = nullptr;
> 
> cookies_list -> cookieList

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:346
> > + * Returns: (element-type SoupCookie) (transfer full): A #GSList of #SoupCookie instances
> > + *    or %NULL in case of error.
> 
> If NULL means error, how can we return an empty list?

Oops! Documentation bug, fixed!

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:347
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:361
> > + * @name: the name the cookie to be deleted
> > + * @domain: the domain of the cookie to be deleted
> > + * @path: the path of the cookie to be deleted
> 
> It's weird that this doesn't receive a SoupCookie, maybe we can document
> that name, domain and patch will be used to match the cookie.

If you think that the triplet {name, domain, path} is what acts as a primary key to identify the cookie, then it's not that weird. Besides, it's particularly convenient since soup_cookie_new() doesn't allow you to pass a null value, which would force you to construct a SoupCookie with some fake/dummy value here just to delete it, which is quite weird IMHO.

For that reason, I think that simply passing the three strings that are needed to identify the cookie to be deleted is simpler, cleaner and more convenient, so I've re-written a bit the documentation to try to make it more clear now:

"""
 Asynchronously delete a #SoupCookie from the current session, that will be uniquely                                
 identified in the internal cookie jar by its name, domain and path.  
"""

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:371
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:412
> > + * Returns: %TRUE if the cookie was added or %FALSE in case of error.
> 
> added -> deleted

Fixed.

> > Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:413
> > + */
> 
> Since: 2.20

Fixed.

> > Source/WebKit/UIProcess/API/wpe/WebKitCookieManager.h:131
> > +WEBKIT_API GList*
> 
> GList* -> GList *

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:122
> > +        test->m_cookieAdded = added;
> 
> Does this make sense? we are asserting before if there's an error which is
> the only case where the cookie isn't added no?

You're right, it's not needed. And the same applies to m_cookieDeleted. Fixed (by removing both things).

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:148
> > +        g_list_free_full(m_cookies, reinterpret_cast<GDestroyNotify>(soup_cookie_free));
> 
> You should free the cookies in the destructor too.

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:149
> > +        m_cookies = 0;
> 
> nullptr

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:287
> > +    test->setAcceptPolicy(WEBKIT_COOKIE_POLICY_ACCEPT_NO_THIRD_PARTY);
> 
> No third party is already the default.

Fixed (by removing this line).

> > Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:332
> > +    g_assert_nonnull(foundCookies);
> > +    g_assert_cmpint(g_list_length(foundCookies), ==, 2);
> 
> g_list_length returns 0 in case list is null, so we don't need the previous
> assert that isn't used in previous calls to getCookies() either.

Fixed (by removing the unnecessary asserts).


(In reply to Michael Catanzaro from comment #19)
> Comment on attachment 326469 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326469&action=review
> 
> > Source/WebCore/platform/Cookie.h:71
> > +    Cookie(SoupCookie*);
> 
> Also, though I think this conversion is OK, it'd probably be less confusing
> to make it explicit. I usually make single-parameter constructors explicit
> by default.

Fixed.


Last, I realized (thanks to one of the unit tests I had added, that was failing!) that I needed to change the implementation of NetworkStorageSession::deleteCookie() back to how I had it in my initial proposal, before this review from Michael:

> > Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:326
> > +    GUniquePtr<SoupCookie> targetCookie(cookie);
> > +    GUniquePtr<GSList> cookiesList(soup_cookie_jar_get_cookie_list(cookieStorage(), uri.get(), TRUE));
> > +    for (GSList* item = cookiesList.get(); item; item = g_slist_next(item)) {
> > +        GUniquePtr<SoupCookie> soupCookie(static_cast<SoupCookie*>(item->data));
> > +
> > +        // Use (name, domain, path) as the primary key to identify the cookie.
> > +        if (g_strcmp0(soup_cookie_get_name(soupCookie.get()), soup_cookie_get_name(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_domain(soupCookie.get()), soup_cookie_get_domain(targetCookie.get()))
> > +            || g_strcmp0(soup_cookie_get_path(soupCookie.get()), soup_cookie_get_path(targetCookie.get())))
> > +            continue;
> > +
> > +        soup_cookie_jar_delete_cookie(cookieStorage(), soupCookie.get());
> > +        break;
> 
> This is inefficient. Imagine deleting a list of cookies... it should be O(n)
> but now it's O(n^2) because you have to loop through roughly half the list
> each time. Fortunately, I don't think we need to store the SoupCookies in
> some other data structure to make this work, because
> soup_cookie_jar_delete_cookie() doesn't work by comparing pointer values, it
> works by checking the domain, name, value, and path of the cookie. So I
> think you don't need to get the SoupCookie from the cookie jar: you can just
> construct a new one yourself, and it should work. So you shouldn't need this
> loop at all.

Following this comment, I replaced all that code with a simple call to soup_cookie_jar_delete_cookie(targetCookie.get()), but the problem is that such a thing won't work because what  soup_cookie_jar_delete_cookie() does internally is to (1) retrieve a list of cookies for a given domain -out of a hash table indexed by domain- and then (2) iterate through that list using soup_cookie_equal() to decide which cookie to delete. And since soup_cookie_equal() uses **name, path & value** (curiously enough, it doesn't use the domain) to match the cookie in the jar with the one that needs to be deleted, this approach won't work with the API we are proposing because what we use to match the cookie is name, domain & path, but not the value (since we might not know it after all if all we want is to delete a cookie identifying it by what it constitutes the primary key).

So, this seems to me to be a good solution after all even if it's not ideal. I could create a hash table here to keep a parallel structure that we could check to use the actual SoupCookie we want to delete without having to do this loop but in my mind that's a bit overkill for a few reasons:
  1. It would require initializing that hash table when creating the cookie storage by adding every single cookie in the jar to it (which would consume memory), just in case we want to delete a cookie through this new API afterwards.
  2. It's not really true that this is O(n^2) where n == number of cookies stored: we call soup_cookie_jar_get_cookie_list() to retrieve a list of cookies *for the given domain*, and that's the list we iterate over, which doesn't necessarily have to be too big (or not as big as if it was iterating through ALL the cookies stored), so this is a fairly relative problem.
  3. Deleting a cookie is not a common use case (otherwise this API would have existed already), so I think I'd optimize for not adding extra artifacts here (such the proposed hash table) just to make this a bit more efficient at the expense of always using more memory, specially when the performance impact doesn't have to be that bad since it's O(n^2) where n == list of cookies for a given domain, not all of them.

So, for these reasons I simply brought the original code I had in place back for now, which gets the unit tests passing now again.

Now, another alternative I can think of that would remove this "burden" from WebKit2GTK itself (by requiring some extra work from the client) would be to actually have webkit_cookie_manager_delete_cookie() receive a SoupCookie* instead of the {name, domain, path} triplet. By doing that it would no longer be possible to conveniently remove a cookie by simple specifying those three values (ignoring the value), as it would require that the app first had retrieved the cookie that it wants to delete from the cookie storage. The advantage, however, would be that there would be no need to do this loop in NetworkStorageSessionSoup.cpp as simply calling soup_cookie_jar_delete_cookie() with the cookie passed would work, since this time soup_cookie_equal() would be able to match the right cookie as the value attribute would be set.

Carlos, Michael... what is your preference?:

  a. Keep the API as it is in the current patch (with webkit_cookie_manager_delete_cookie(name, domain, path)) + the funny loop in WebCore to match the cookie to be deleted
  b. Change the API so that webkit_cookie_manager_delete_cookie() receives a SoupCookie, simplifying the logic in WebKit + forcing apps to specify a full cookie (including "value") to delete it

I personally don't have any strong preference, as I think both could work fine, but I want to stress that I have no problem in changing from (a) to (b) if that's what looks better to you.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171118/cee42551/attachment-0001.html>


More information about the webkit-unassigned mailing list