Bug 222366 – [WPE][GTK] Move network session APIs to a new WebKitNetworkSession class

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

r267502 and r267534 moved WebKitWebContext APIs that are internally properties of the NetworkSession from WebKitWebContext to WebKitWebsiteDataManager. This was required since r267763 moved network process ownership from WebProcessPool to WebsiteDataStore. Problem is this doesn't make much sense from an API user perspective. Why should WebKitWebsiteDataManager be the place to configure seemingly-unrelated network properties? I realize it's really late to be reconsidering this now, but if we release these APIs in 2.32 we are stuck with them forever. We still have a bit of time left.

At first, I thought we could keep these APIs in WebKitWebContext and have them internally configure all associated WebsiteDataStores, but it doesn't work. The problem is that a WebKitWebsiteDataManager may be associated with a WebKitWebContext *or* a WebKitWebView. If the WebKitWebView does not have its own WebKitWebsiteDataManager, then it will use the WebKitWebsiteDataManager of its WebKitWebContext. This means it's now possible for two WebKitWebViews in the same WebKitWebContext to use *different* network processes and NetworkSessions. It also means it's possible for two WebKitWebViews in totally different WebKitWebContexts to use the *same* network process, which was previously not possible. So the APIs really cannot remain part of WebKitWebContext.

I think we can solve this by introducing a new API object, WebKitNetworkSession. It would need to be a property of both the WebKitWebContext and WebKitWebView, exactly the same as WebKitWebsiteDataManager is. The WebKitNetworkSession would have its own website-data-manager property, and would be the object that really owns the WebKitWebsiteDataManager. Then the WebKitWebContext and WebKitWebView website-data-manager properties and getters/setters would instead forward to their WebKitNetworkSession. This means everything would work the same as it does now, but we would effectively have a new API object with a nicer name so that we don't have to confusingly put seemingly-unrelated functions under WebKitWebsiteDataManager.

For now, WebKitNetworkSession would have three functions:

webkit_network_session_get_tls_errors_policy
webkit_network_session_set_tls_errors_policy
webkit_network_session_set_network_proxy_settings

The corresponding WebKitWebContext APIs would remain deprecated, and the corresponding WebKitWebsiteDataManager APIs would be removed (quick! before we release them in 2.32). We could *optionally* deprecate the website-data-manager properties and getters/setters of WebKitWebContext and WebKitWebsiteDataManager, since it might be confusing to have so many different places where you can set a WebKitWebsiteDataManager. But we don't have to do that. This scheme could work either way.

We don't have much time to decide whether to do this. Opinions?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> For now, WebKitNetworkSession would have three functions:
> 
> webkit_network_session_get_tls_errors_policy
> webkit_network_session_set_tls_errors_policy
> webkit_network_session_set_network_proxy_settings

Actually it's 2021, we don't need the TLS error policy functions anymore. IMO we can deprecate those without replacement.

That would leave only webkit_network_session_set_network_proxy_settings.

[Michael Catanzaro]

Carlos thinks WebKitNetworkSession is too much for 2.32 in the limited time we have left before release. So I have opened bug #222561 to remove webkit_website_data_manager_[get,set]_tls_errors_policy. We'll be stuck with the misplaced webkit_website_data_manager_set_network_proxy_settings for the lifetime of the GTK 3 API. I'll assign this bug to block the GTK 4 tracker so we can reconsider webkit_website_data_manager_set_network_proxy_settings() for GTK 4. We should also totally remove WebKitTLSErrorsPolicy from the GTK 4 API.

[Michael Catanzaro]

Even if we decide to keep things as they are now, we still need to document the network process implications carefully. Unless you're familiar with WebKit internals, it's unclear what's going on here, and too easy for a competent developer to accidentally create too many network processes by mistake, e.g.:

https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/1170#note_1498599

[Carlos Garcia Campos]

I've been looking at this. The good thing of not introducing thi new api in the old api is that we have more flixibility to change the behavior too. So, I propose to change the way it currently works and move to something similar to apple. This is my proposal:

1.- Only support one network process: I don't see any reason to use more than one, you can just have different sessions in the same process. With current API you can end up with two network processes accessing the same cookies database, for example, even if that's actually a programmer error.

2.- Make it possible to use a different session on every web view: in our current API we can only decide whether to use the web context network session or an ephemeral one. The API would be similar to the settings property, you can share the same settings or use a different object, but it doesn't depend on the web context.

The API would be something like:

 - WebKitNetworkSession
  + new(): receives the data directory and cache directory, both optional and creates a persistent WebKitWebsiteDataManager for the given directories.
  + new_epehmeral(): creates an ephemeral WebKitWebsiteDataManager
  + All network related APIs from WebKitWebsiteDataManager (get_cookie_manager, set|get_itp_enabled, set|get_persistent_credential_storage_enabled, set|get_tls_errors_policy, set_network_proxy_settings, set_memory_pressure_settings and get_itp_summary) will be moved to WebKitNetworkSession
  + All network related APIs from WebKitWebContext (prefetch_dns, allow_tls_certificate_for_host and downloads) will be moved to WebKitNetworkSession

 - WebKitWebsiteDataManager
  + Remove the constructors, so it can only be created by WebKitNetworkSession
  + It keeps only the API to fetch and remove website data.

 - WebKitWebContext
  + Remove clear_cache(): it can be done with WebKitWebsiteDataManager.
  + Remove the website-data-manager property.

 - WebKitWebView
  + Remove the is-ephemeral property
  + Add network-session construct only property

What do you think?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #4)
> 1.- Only support one network process: I don't see any reason to use more
> than one, you can just have different sessions in the same process. With
> current API you can end up with two network processes accessing the same
> cookies database, for example, even if that's actually a programmer error.

So you want to have one network process per UI process, matching Apple's process model? I think that would be fine. We would then want to switch to one NetworkSession per WebKitWebContext/WebProcessPool instead of using the default NetworkSession; otherwise, separate instances of WebKit could clash with each other.

Sounds much better than the status quo.

> 2.- Make it possible to use a different session on every web view: in our
> current API we can only decide whether to use the web context network
> session or an ephemeral one. The API would be similar to the settings
> property, you can share the same settings or use a different object, but it
> doesn't depend on the web context.

I like this proposal. It makes more sense than what we have now.

I think we can keep WebKitWebView:is-ephemeral and just make it read-only.

[Carlos Garcia Campos]

Created attachment 464556 [details]
WIP patch

This is what I have so far, uploading it now to get some initial feedback. What is left to do is:

 - API tests: make the existing ones build and add specific test for network session.
 - WPE minibrowser
 - Favicon database: I'm not sure what to do here, I'll elaborate in a follow up comment

[Carlos Garcia Campos]

WebKitFaviconDatabase is kind of a spacial case of website data, because it's not attached to a particular session, but global to a web context. 

 - If the web context is created with an ephemeral website data store, but a an existing database is used, the icons are still loaded, but nothing is written to the database.

 - If created with a persistent data store, the database is written, expect for icon requests made from an ephemeral web view.

My initial idea was to move the favicon database ownership from web context to website data manager, but then it can't be shared with ephemeral web views. Keeping it in the web context is problematic too, because we need to know whether to allow database writes when the database is created and in the new api the web context doesn't have a network session, it's a web view property. I think the easiest solution would be to leave the favicon database in the web context and change the way it's configured. Currently, to enable favicosn you have to call webkit_web_context_set_favicon_database_directory(), which I find a bit weird, and passing null as path doesn't disables it but uses the default path. We can add methods to enable/disable favicons instead (or open/close the database which what actually happens in set_favicon_database_directory). Then the open method would receive an optional path and the readonly mode.

[Carlos Garcia Campos]

I'm also considering to remove all (or most of) webkit_web_view_new_with constructors. With all the construct properties we currently have I think most people need to use g_object_new and pass properties. Maybe we can keep with_related_view and add optional parameters to new() for the properties that are expected to be passed (web context, network session and settings, for example)

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> My initial idea was to move the favicon database ownership from web context
> to website data manager, but then it can't be shared with ephemeral web
> views.

This actually seems ideal. I don't think it's desirable to share the favicon database with ephemeral web views because the favicon database will necessarily load remote web content, and a server could use the absence of a favicon load to determine whether the user has previously visited in non-ephemeral mode. It may be a small info leak, but I'd say the server is not supposed to be able to do that.

> I think the easiest solution would be to leave the favicon
> database in the web context and change the way it's configured.

If you don't want to move it to WebKitWebsiteDataManager, then OK.

(In reply to Carlos Garcia Campos from comment #8)
> I'm also considering to remove all (or most of) webkit_web_view_new_with
> constructors. With all the construct properties we currently have I think
> most people need to use g_object_new and pass properties. Maybe we can keep
> with_related_view and add optional parameters to new() for the properties
> that are expected to be passed (web context, network session and settings,
> for example)

Bug #250835

[Michael Catanzaro]

Comment on attachment 464556 [details]
WIP patch

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

I looked over the first half of the patch so far. My opinion is: very good.

> Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp:536
> +void webkit_network_session_set_memory_pressure_settings(WebKitMemoryPressureSettings* settings)
> +{
> +    std::optional<MemoryPressureHandler::Configuration> config = settings ? std::make_optional(webkitMemoryPressureSettingsGetMemoryPressureHandlerConfiguration(settings)) : std::nullopt;
> +    WebProcessPool::setNetworkProcessMemoryPressureHandlerConfiguration(config);
> +}

This one looks weird. Probably a preexisting issue, but I don't like that the setting works globally instead of on a particular WebProcessPool/WebKitWebContext or WebKitNetworkSession.

> Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.h.in:61
> +struct _WebKitNetworkSession {
> +    GObject parent;
> +
> +    /*< private >*/
> +    WebKitNetworkSessionPrivate *priv;
> +};
> +
> +struct _WebKitNetworkSessionClass {
> +    GObjectClass parent_class;
> +
> +    /*< private >*/
> +    void (*_webkit_reserved0) (void);
> +    void (*_webkit_reserved1) (void);
> +    void (*_webkit_reserved2) (void);
> +    void (*_webkit_reserved3) (void);
> +};

This might depend on how exactly Adrian decides to handle bug #243663, but I think it's time to start using G_DECLARE_FINAL_TYPE. Then we would have the instance struct in the source file instead of the header (after all, everything there is private), and the class struct would be hidden behind the marco. We wouldn't maintain vtable padding unless we make it derivable in the future (which for this class we'd never do).

Let's talk to Adrian and see what he's planning.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Garcia Campos from comment #7)
> > My initial idea was to move the favicon database ownership from web context
> > to website data manager, but then it can't be shared with ephemeral web
> > views.
> 
> This actually seems ideal. I don't think it's desirable to share the favicon
> database with ephemeral web views because the favicon database will
> necessarily load remote web content, and a server could use the absence of a
> favicon load to determine whether the user has previously visited in
> non-ephemeral mode. It may be a small info leak, but I'd say the server is
> not supposed to be able to do that.
> 

That's a good point, I think we can try to move it then.

> > I think the easiest solution would be to leave the favicon
> > database in the web context and change the way it's configured.
> 
> If you don't want to move it to WebKitWebsiteDataManager, then OK.
> 
> (In reply to Carlos Garcia Campos from comment #8)
> > I'm also considering to remove all (or most of) webkit_web_view_new_with
> > constructors. With all the construct properties we currently have I think
> > most people need to use g_object_new and pass properties. Maybe we can keep
> > with_related_view and add optional parameters to new() for the properties
> > that are expected to be passed (web context, network session and settings,
> > for example)
> 
> Bug #250835

Perfect, let's discuss it there.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> Comment on attachment 464556 [details]
> WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464556&action=review
> 
> I looked over the first half of the patch so far. My opinion is: very good.
> 

Great, thanks!

> > Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.cpp:536
> > +void webkit_network_session_set_memory_pressure_settings(WebKitMemoryPressureSettings* settings)
> > +{
> > +    std::optional<MemoryPressureHandler::Configuration> config = settings ? std::make_optional(webkitMemoryPressureSettingsGetMemoryPressureHandlerConfiguration(settings)) : std::nullopt;
> > +    WebProcessPool::setNetworkProcessMemoryPressureHandlerConfiguration(config);
> > +}
> 
> This one looks weird. Probably a preexisting issue, but I don't like that
> the setting works globally instead of on a particular
> WebProcessPool/WebKitWebContext or WebKitNetworkSession.
> 

That's because we want to apply the settings to any network process spawned by the website data store. Now there's only one network process, but still, you can't set different settings per network session, because it's global to the network process. So, this is like a static method of WebKitNetworkSession.

> > Source/WebKit/UIProcess/API/glib/WebKitNetworkSession.h.in:61
> > +struct _WebKitNetworkSession {
> > +    GObject parent;
> > +
> > +    /*< private >*/
> > +    WebKitNetworkSessionPrivate *priv;
> > +};
> > +
> > +struct _WebKitNetworkSessionClass {
> > +    GObjectClass parent_class;
> > +
> > +    /*< private >*/
> > +    void (*_webkit_reserved0) (void);
> > +    void (*_webkit_reserved1) (void);
> > +    void (*_webkit_reserved2) (void);
> > +    void (*_webkit_reserved3) (void);
> > +};
> 
> This might depend on how exactly Adrian decides to handle bug #243663, but I
> think it's time to start using G_DECLARE_FINAL_TYPE. Then we would have the
> instance struct in the source file instead of the header (after all,
> everything there is private), and the class struct would be hidden behind
> the marco. We wouldn't maintain vtable padding unless we make it derivable
> in the future (which for this class we'd never do).
> 
> Let's talk to Adrian and see what he's planning.

I guess the tricky part is that we use the new placement syntax for the private struct to be able to use smart pointers, that's why we have our own macros to define types. G_DECLARE_FINAL_TYPE doesn't use a private struct, I think.

[Carlos Garcia Campos]

Created attachment 464576 [details]
WIP patch

Updated patch. 

Done:
 - Ported most of the API tests.
 - Renamed webkit_network_session_set_network_proxy_settings() as webkit_network_session_set_proxy_settings()
 - Add WebKitNetworkSession to autocleanups
 - Fix webkit_network_session_download_uri()

Todo:
 - Move favicon database to WebKitWebsiteDataManager.
 - Enable TestCookieManager (I don't know why this is disabled)
 - Port WPE MiniBrowser
 - Port cog

[Carlos Garcia Campos]

Created attachment 464577 [details]
WIP patch

Submitting the right patch this time.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #12)
> That's because we want to apply the settings to any network process spawned
> by the website data store. Now there's only one network process, but still,
> you can't set different settings per network session, because it's global to
> the network process. So, this is like a static method of
> WebKitNetworkSession.

OK, I see that it really *has* to be this way because it wouldn't make sense to have separate memory pressure settings per network session when all the network sessions use the same network process. So OK.

> I guess the tricky part is that we use the new placement syntax for the
> private struct to be able to use smart pointers, that's why we have our own
> macros to define types. G_DECLARE_FINAL_TYPE doesn't use a private struct, I
> think.

Let's discuss in bug #243663.

[Carlos Garcia Campos]

Created attachment 464608 [details]
WIP patch

Another update.

Done:
 - Enabled and ported cookie manager test.
 - Moved favicon database to WebKitWebsiteDataManager.

Todo:
 - Fix favicon database unit test (it's mostly ported but there are changes in behavior)
 - Add WebKitNetworkSession API test.
 - Port WPE MiniBrowser
 - Port cog

So, in the end I moved the favicons to website data manager, which means ephemeral sessions always load icons when they are enabled. Since it's part of website data manager, there's no api to set the directory, but instead set|get_favicons_enabled. When enabled, the database is created and opened, using the website data manger configuration (base cache directory). Would it make sense to change the default and have favicos enabled by default instead?

[Michael Catanzaro]

I guess disabling the favicon database makes sense for apps that are not web browsers, but our default settings are mostly designed to be good for browsers, so I'd say favicon database should be enabled by default. What would be really nice would be to have two sets of defaults, with an API to express intent. For example, WebKitSettings could have a construct property to indicate whether the application should receive web browser or non-browser defaults. The closest we have to this currently is WebKitCacheModel (WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER/WEBKIT_CACHE_MODEL_WEB_BROWSER/WEBKIT_CACHE_MODEL_DOCUMENT_BROWSER), but of course that's not a good match for settings.

P.S. Please also update migrating-to-webkitgtk-6.0.md.

[Carlos Garcia Campos]

Created attachment 464630 [details]
WIP patch

Another update:

Done:
 - Changed favicons to be enabled by default.
 - Fixed the favicons database unit test.
 - Added WebKitNetworkSession api test.
 - Ported WPE MiniBrowser

Todo:
 - Port cog
 - Update migrating-to-webkitgtk-6.0.md

[Carlos Garcia Campos]

Pull request: https://github.com/WebKit/WebKit/pull/9104

[EWS]

Committed 259433@main (2d1b53b211b4): <https://commits.webkit.org/259433@main>

Reviewed commits have been landed. Closing PR #9104 and removing active labels.