[GTK][WPE] webkit_cookie_manager_delete_all_cookies doesn't delete the cookies if called before a web process is running

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Debarshi Ray]

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: [e-mail] <[e-mail]...>
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?

[Debarshi Ray]

Created attachment 317425 [details]
Reproducer: webkit_cookie_manager_delete_all_cookies doesn't delete the cookies

[Debarshi Ray]

Created attachment 317427 [details]
Attempted workaround using webkit_website_data_manager_clear

[Carlos Garcia Campos]

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.

[Debarshi Ray]

(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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

Wow that's way too complicated....

[Carlos Garcia Campos]

Created attachment 318242 [details]
Patch

[Build Bot]

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

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

OK. I would ask Brady to do the owner review. Maybe he'll want to update the Mac to take this approach.

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

Brady, is this version OK with you?

[Michael Catanzaro]

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?

[Brady Eidson]

(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.

[Alex Christensen]

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>>

[Alex Christensen]

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?

[Brady Eidson]

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.

[Brady Eidson]

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.

[Carlos Garcia Campos]

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).

[Carlos Garcia Campos]

Brady?

[Brady Eidson]

(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.

[Carlos Garcia Campos]

(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.

[Brady Eidson]

(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.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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

[Carlos Garcia Campos]

Committed r225043: <https://trac.webkit.org/changeset/225043>

[Debarshi Ray]

Thanks for seeing this through, folks. Much appreciated.