Summary: | [WPE][GTK] Move network session APIs to a new WebKitNetworkSession class | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||
Component: | WebKitGTK | Assignee: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, mcatanzaro | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=222561 | ||||||||||||||
Bug Depends on: | 250570, 250649 | ||||||||||||||
Bug Blocks: | 210100 | ||||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2021-02-24 10:05:20 PST
(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. 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. 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 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? (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. 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
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. 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) (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 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. (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. (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. 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
Created attachment 464577 [details]
WIP patch
Submitting the right patch this time.
(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. 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?
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. 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
Pull request: https://github.com/WebKit/WebKit/pull/9104 Committed 259433@main (2d1b53b211b4): <https://commits.webkit.org/259433@main> Reviewed commits have been landed. Closing PR #9104 and removing active labels. |