Bug 175265 - [GTK][WPE] webkit_cookie_manager_delete_all_cookies doesn't delete the cookies if called before a web process is running
Summary: [GTK][WPE] webkit_cookie_manager_delete_all_cookies doesn't delete the cookie...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-07 09:43 PDT by Debarshi Ray
Modified: 2017-11-20 00:19 PST (History)
13 users (show)

See Also:


Attachments
Reproducer: webkit_cookie_manager_delete_all_cookies doesn't delete the cookies (1.24 KB, text/plain)
2017-08-07 09:44 PDT, Debarshi Ray
no flags Details
Attempted workaround using webkit_website_data_manager_clear (3.00 KB, text/plain)
2017-08-07 09:45 PDT, Debarshi Ray
no flags Details
Patch (10.15 KB, patch)
2017-08-16 00:43 PDT, Carlos Garcia Campos
beidson: review-
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Updated patch (27.31 KB, patch)
2017-08-29 04:11 PDT, Carlos Garcia Campos
beidson: review-
Details | Formatted Diff | Diff
New patch (8.14 KB, patch)
2017-11-16 02:58 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Debarshi Ray 2017-08-07 09:43:23 PDT
For context, see:
https://bugzilla.gnome.org/show_bug.cgi?id=781005

webkit_cookie_manager_delete_all_cookies no longer deletes all the cookies before returning ever since it was re-written in terms of webkit_website_data_manager_clear in:

commit c968a9600329208662695273a6ce676f84fc657f
Author: carlosgc@webkit.org <carlosgc@webkit.org...>
Date:   Wed Feb 15 07:34:59 2017 +0000

    [GTK] Update cookie manager API to properly work with ephemeral sessions
    https://bugs.webkit.org/show_bug.cgi?id=168230

This isn't surprising because now webkit_cookie_manager_delete_all_cookies asynchronously calls webkit_website_data_manager_clear but doesn't wait for it to complete without returning.

I don't know how WebProcessPool and the network process works, so I am not sure if webkit_cookie_manager_delete_all_cookies was working merely by accident.

I tried to work around this by wrapping webkit_website_data_manager_clear inside a new thread-default GMainContext (using g_main_context_push/pop_thread_default), but that doesn't work either. The GAsyncReadyCallback doesn't get invoked. Doesn't WTF::RunLoop, etc. not work with thread-default GMainContexts?
Comment 1 Debarshi Ray 2017-08-07 09:44:35 PDT
Created attachment 317425 [details]
Reproducer: webkit_cookie_manager_delete_all_cookies doesn't delete the cookies
Comment 2 Debarshi Ray 2017-08-07 09:45:07 PDT
Created attachment 317427 [details]
Attempted workaround using webkit_website_data_manager_clear
Comment 3 Carlos Garcia Campos 2017-08-10 01:33:11 PDT
webkit_cookie_manager_delete_all_cookies() has never been sync, it has always sent an async message to the network process to delete the cookies. However, it's expected that a subsequent load always happens after the cookies has been deleted, because the load message is sent after the delete cookies ones. I don't know why this is not happening now with the data manager, though.
Comment 4 Debarshi Ray 2017-08-10 02:20:57 PDT
(In reply to Carlos Garcia Campos from comment #3)
> webkit_cookie_manager_delete_all_cookies() has never been sync, it has
> always sent an async message to the network process to delete the cookies.
> However, it's expected that a subsequent load always happens after the
> cookies has been deleted, because the load message is sent after the delete
> cookies ones. I don't know why this is not happening now with the data
> manager, though.

I see. Thanks for looking into it, Carlos.
Comment 5 Carlos Garcia Campos 2017-08-10 03:46:11 PDT
The problem is that we are not deleting the cookies at all. This is what happens:

1- We create our WebKitWebContext that creates its WebProcessPool
2- We set a persistent cookies storage so a new network process is spawned for the process pool.
3- We ask the website data store to delete all cookies, but since website data store is a web process observer and we haven't spwaned any web process yet, it creates a new WebProcessPool with the default configuration (no persistent cookies) and sends the message to delete the cookies there.
4- The network process of the second process pool does nothing because it doesn't have cookies at all.

I don't know why website data store only uses process pools that already have a web process launched, Brady, Alex, any idea?

I know Apple fixed this by adding WebsiteDataStore::processPoolForCookieStorageOperations() that doesn't create a process pool if there isn't one with a web process launched, and returns nullptr in that case. Then the website data store saves the pending cookie operations, so that when the new process pool is created from the website data store, the pending cookies are sent to the newly created network process as parameters of the message AddWebsiteDataStore. We could try something similar, but I still don't understand why we want to create a secondary network process when we already have one running that is configured.
Comment 6 Michael Catanzaro 2017-08-10 10:34:18 PDT
Wow that's way too complicated....
Comment 7 Carlos Garcia Campos 2017-08-16 00:43:42 PDT
Created attachment 318242 [details]
Patch
Comment 8 Build Bot 2017-08-16 00:45:35 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 9 Michael Catanzaro 2017-08-16 10:29:58 PDT
Comment on attachment 318242 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1191
> -    return &processPool.websiteDataStore().websiteDataStore() == this;
> +    return &processPool.websiteDataStore().websiteDataStore() == this || m_associatedProcessPools.contains(&processPool);

This is the magic line that makes everything work, right? How does this avoid the network process being unnecessarily spawned?
Comment 10 Carlos Garcia Campos 2017-08-16 10:56:10 PDT
Comment on attachment 318242 [details]
Patch

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

We need a wk2 owner for this, Alex?

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1191
>> +    return &processPool.websiteDataStore().websiteDataStore() == this || m_associatedProcessPools.contains(&processPool);
> 
> This is the magic line that makes everything work, right? How does this avoid the network process being unnecessarily spawned?

It doesn't avoid the network process from being spawnede, it avoids that a new process pool with a default configuration is spawned to do nothing. This ensures that our process pool, already configured, is used and the network process with the right configuration  is spawned only if needed, to do the right thing.
Comment 11 Michael Catanzaro 2017-08-26 10:02:26 PDT
Hm, I have another question about this. What about bug #169797? It looks like Brady already fixed this in a different way? Perhaps we just need to implement the unimplemented cookie storage functions in NetworkStorageSessionSoup.cpp?
Comment 12 Carlos Garcia Campos 2017-08-28 02:40:47 PDT
(In reply to Michael Catanzaro from comment #11)
> Hm, I have another question about this. What about bug #169797? It looks
> like Brady already fixed this in a different way? Perhaps we just need to
> implement the unimplemented cookie storage functions in
> NetworkStorageSessionSoup.cpp?

I don't think that "workaround" would fix this issue, it seems to allow setting cookies while the network process hasn't been spawned yet, but not to delete them or any other operation. It doesn't avoid spawning a network process for nothing either.
Comment 13 Michael Catanzaro 2017-08-28 04:26:02 PDT
OK. I would ask Brady to do the owner review. Maybe he'll want to update the Mac to take this approach.
Comment 14 Brady Eidson 2017-08-28 09:34:53 PDT
Comment on attachment 318242 [details]
Patch

If a WebsiteDataStore is going to keep a set of "associated WebProcessPools" like this, then WebProcessPool itself has to do the association in its constructor and destructor. It's folly to mix API objects and IMPL objects like this.
Comment 15 Brady Eidson 2017-08-28 09:35:59 PDT
(In reply to Brady Eidson from comment #14)
> Comment on attachment 318242 [details]
> Patch
> 
> If a WebsiteDataStore is going to keep a set of "associated WebProcessPools"
> like this, then WebProcessPool itself has to do the association in its
> constructor and destructor. It's folly to mix API objects and IMPL objects
> like this.

We need to do better about this "web process pool vs website data store" thing in general. This patch is a wallpaper job for one specific API for one specific use case.
Comment 16 Carlos Garcia Campos 2017-08-29 04:11:32 PDT
Created attachment 319250 [details]
Updated patch

This patch moves the process pool tracking from the GLib API to the cross-platform code. This way WebProcessPool and WebPageProxy are the ones adding/removing associated process pools to WebsiteDataStore.
Comment 17 Michael Catanzaro 2017-09-01 17:44:36 PDT
Brady, is this version OK with you?
Comment 18 Michael Catanzaro 2017-09-12 07:21:51 PDT
Hi Brady, we need to land a fix for this issue and you didn't like the last version of this patch... can you review the new version please?
Comment 19 Brady Eidson 2017-09-13 11:00:20 PDT
(In reply to Michael Catanzaro from comment #18)
> Hi Brady, we need to land a fix for this issue and you didn't like the last
> version of this patch... can you review the new version please?

Sure, will look in a bit.
Comment 20 Alex Christensen 2017-09-13 11:01:37 PDT
Comment on attachment 319250 [details]
Updated patch

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

> Source/WebKit/ChangeLog:24
> +        (API::ProcessPoolConfiguration::copy): Aslo copy the default WebsiteDataStore.

Aslo

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:183
> +    Vector<WebProcessPool*> m_associatedProcessPools;

This can probably be a Vector<std::reference_wrapper<WebProcessPool>>
Comment 21 Alex Christensen 2017-09-13 11:04:15 PDT
Comment on attachment 319250 [details]
Updated patch

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

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:157
> +    RefPtr<WebsiteDataStore> m_defaultWebsiteDataStore;

It seems strange to have a member reference to the default website data store.  Couldn't defaultWebsiteDataStore be a static method?
Comment 22 Brady Eidson 2017-09-13 11:27:06 PDT
Comment on attachment 319250 [details]
Updated patch

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

Note that this patch is obsoleted by https://trac.webkit.org/changeset/221834/webkit, and will need to be rethought.

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:68
> +    WebsiteDataStore* defaultWebsiteDataStore() const { return m_defaultWebsiteDataStore.get(); }
> +    void setDefaultWebsiteDataStore(WebsiteDataStore& websiteDataStore) { m_defaultWebsiteDataStore = &websiteDataStore; }

This name is problematic:
"default website data store" is already an established concept in the APIWebsiteDataStore API.

But also, please note that we need to move *away* from the concept of a process pool having a default datastore and this patch further cements the concept.
Comment 23 Brady Eidson 2017-09-13 11:29:29 PDT
I'm sorry if I didn't make this explicitly clear in my earlier comment a few weeks ago.

It is a problem that we rely on a WebProcessPool having an implicit WebsiteDataStore. It should be a goal to move *away* from that, not further cement it.
Comment 24 Carlos Garcia Campos 2017-09-14 05:26:18 PDT
Is it just a naming issue? WebProcessPool has always a data store, at construction time it creates a legacy one or gets the default one (there's indeed a default one, I'm just allowing the api layer to set it up in my patch).
Comment 25 Carlos Garcia Campos 2017-09-22 00:26:16 PDT
Brady?
Comment 26 Brady Eidson 2017-09-22 14:00:34 PDT
(In reply to Carlos Garcia Campos from comment #24)
> WebProcessPool has always a data store, at
> construction time it creates a legacy one or gets the default one 

I agree that is what exists in the tree today.

What I am saying is that this is a mistake and we need to move away from it.

If you're insistent that you need to expose this as API, I'm giving you fair warning that future WebKit refactoring patches will break the API and it will be up to you to maintain support at that point up in your API layer.
Comment 27 Carlos Garcia Campos 2017-09-22 23:05:51 PDT
(In reply to Brady Eidson from comment #26)
> (In reply to Carlos Garcia Campos from comment #24)
> > WebProcessPool has always a data store, at
> > construction time it creates a legacy one or gets the default one 
> 
> I agree that is what exists in the tree today.
> 
> What I am saying is that this is a mistake and we need to move away from it.

Ok, what should we do instead?

> If you're insistent that you need to expose this as API, I'm giving you fair
> warning that future WebKit refactoring patches will break the API and it
> will be up to you to maintain support at that point up in your API layer.

I'm not insistent to do this, I'm insistent to fix this bug. If this is not the right way, let's do it right, but I need to know what the right way is. I don't mind doing any refactorings needed.
Comment 28 Brady Eidson 2017-09-27 10:43:06 PDT
(In reply to Carlos Garcia Campos from comment #27)
> (In reply to Brady Eidson from comment #26)
> > (In reply to Carlos Garcia Campos from comment #24)
> > > WebProcessPool has always a data store, at
> > > construction time it creates a legacy one or gets the default one 
> > 
> > I agree that is what exists in the tree today.
> > 
> > What I am saying is that this is a mistake and we need to move away from it.
> 
> Ok, what should we do instead?

It's already wrong in that web views created from one process pool already can have difference data stores from one another. Additionally, we'll soon be entering a world where one web view might use more than one data store.

So, WebProcessPool should not have a data store member. 

Everything it does that uses the data store member should instead take a data store argument.

With those principles in place, recompiling and fixing up call sites, etc etc.
Comment 29 Carlos Garcia Campos 2017-11-16 02:58:30 PST
Created attachment 327054 [details]
New patch

In r221834, WebProcessPool::setPrimaryDataStore() was added and the default one is no longer created until the very last moment. Using WebProcessPool::setPrimaryDataStore() when creating the web context fixes this issue.
Comment 30 Michael Catanzaro 2017-11-17 08:59:32 PST
Comment on attachment 327054 [details]
New patch

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

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestCookieManager.cpp:313
> +    // Changed signal is emittend for every deleted cookie, twice in this case.

emitted
Comment 31 Carlos Garcia Campos 2017-11-20 00:11:37 PST
Committed r225043: <https://trac.webkit.org/changeset/225043>
Comment 32 Debarshi Ray 2017-11-20 00:19:24 PST
Thanks for seeing this through, folks. Much appreciated.