Glib download API change request
Since the introduction of WKDownload in 2020, I’ve been trying to make the download object creation process asynchronous in WebKit. With 290510@main I’ve removed the last caller of WebProcessPool::download from the Cocoa platform, but I noticed that the glib APIs have 3 places where a WebKitDownload* object is returned synchronously from an API call: webkit_web_context_download_uri, webkit_web_view_download_uri, and webkit_network_session_download_uri. Would the maintainers of these APIs be willing to deprecate and remove them in favor of an API design closer to WKWebView’s startDownloadUsingRequest:completionHandler:? That function is given a callback which asynchronously provides the download object to the API client. It would also be nice if webkit_download_cancel could be reshaped to look more like WKDownload.cancel: and stop using legacyDidCancel.
Hm, we'll need to investigate this. Deprecating APIs is easy. Adding newer replacements is often easy. But removing APIs is extremely disruptive; we don't want to do that unless there is absolutely no viable alternative. It requires an API version bump, which requires changes in every application that depends on WebKitGTK/WPE even if they don't use the affected API, and new parallel packaging in every downstream. We should try to reimplement the sync APIs using the async APIs so we don't have to remove them. The challenge here is we have to do that *without* iterating the global RunLoop (otherwise, application callbacks will be dispatched at unexpected times, and all manner of bugs could occur, including memory corruption and other vulnerabilities). So we need to do this using a "private" RunLoop. In Linux-specific code, we would do this easily by using GMainContext directly; we would create a new GMainContext, push it as thread-default, iterate it until the async operation has completed, and then pop it, thereby creating a synchronous wrapper around an async function call. But I don't think it can be done without changes in WebKit's RunLoop. RunLoop does not have any equivalent concept to allow temporarily making a different RunLoop the "current" default RunLoop for a thread, which is what we need here. It's probably doable, though. Once before, sometime rather recently (maybe 6-12 months ago?), we gave up on doing this properly and landed a patch that iterates the global run loop behind the application's back in order to fix some bug. I don't remember what particular bug it was, but I was opposed to doing that; it's really not safe. If you're OK with keeping the existing synchronous code around, it would be less work to just maintain separate sync and async paths. We'll remove all deprecated APIs in the far distant future, next time we bump API version; we could add a comment to delete the sync versions then. Michael
I’d be fine with having a transition period where both old and new APIs work, then possibly another transition period where old APIs exist as deprecated stubs that don’t work before removing them. It’s also possible we could add glib-specific synchronous IPC to support the old APIs during this transition period. The point I’m trying to make is that we need to do IPC roundtrips before creating a DownloadProxy, and we need to have a path towards aligning our API shapes when it becomes problematic to support differently shaped APIs.
On Feb 18, 2025, at 9:00 AM, Michael Catanzaro <mcatanzaro@redhat.com> wrote:
Hm, we'll need to investigate this.
Deprecating APIs is easy. Adding newer replacements is often easy. But removing APIs is extremely disruptive; we don't want to do that unless there is absolutely no viable alternative. It requires an API version bump, which requires changes in every application that depends on WebKitGTK/WPE even if they don't use the affected API, and new parallel packaging in every downstream.
We should try to reimplement the sync APIs using the async APIs so we don't have to remove them. The challenge here is we have to do that *without* iterating the global RunLoop (otherwise, application callbacks will be dispatched at unexpected times, and all manner of bugs could occur, including memory corruption and other vulnerabilities). So we need to do this using a "private" RunLoop. In Linux-specific code, we would do this easily by using GMainContext directly; we would create a new GMainContext, push it as thread-default, iterate it until the async operation has completed, and then pop it, thereby creating a synchronous wrapper around an async function call. But I don't think it can be done without changes in WebKit's RunLoop. RunLoop does not have any equivalent concept to allow temporarily making a different RunLoop the "current" default RunLoop for a thread, which is what we need here. It's probably doable, though.
Once before, sometime rather recently (maybe 6-12 months ago?), we gave up on doing this properly and landed a patch that iterates the global run loop behind the application's back in order to fix some bug. I don't remember what particular bug it was, but I was opposed to doing that; it's really not safe.
If you're OK with keeping the existing synchronous code around, it would be less work to just maintain separate sync and async paths. We'll remove all deprecated APIs in the far distant future, next time we bump API version; we could add a comment to delete the sync versions then.
Michael
On Tue, Feb 18 2025 at 11:00:05 AM -06:00:00, Michael Catanzaro <mcatanzaro@redhat.com> wrote:
We should try to reimplement the sync APIs using the async APIs so we don't have to remove them. The challenge here is we have to do that *without* iterating the global RunLoop (otherwise, application callbacks will be dispatched at unexpected times, and all manner of bugs could occur, including memory corruption and other vulnerabilities). So we need to do this using a "private" RunLoop.
Sorry, I incorrectly assumed we would need to do this in cross-platform code, but looks like we can do it in Linux-specific code, where we can use GMainContext directly and easily avoid this problem. So I don't think we actually have any problem here; we can keep the sync codepath working as a wrapper around the async codepath.
Well I'm not sure if this would work or not. Left a comment at https://github.com/WebKit/WebKit/pull/42334#issuecomment-2723224554
On 2025-02-17 19:12, Alex Christensen via webkit-dev wrote:
Since the introduction of WKDownload in 2020, I’ve been trying to make the download object creation process asynchronous in WebKit. With 290510@main I’ve removed the last caller of WebProcessPool::download from the Cocoa platform, but I noticed that the glib APIs have 3 places where a WebKitDownload* object is returned synchronously from an API call: webkit_web_context_download_uri, webkit_web_view_download_uri, and webkit_network_session_download_uri. Would the maintainers of these APIs be willing to deprecate and remove them in favor of an API design closer to WKWebView’s startDownloadUsingRequest:completionHandler:? That function is given a callback which asynchronously provides the download object to the API client. It would also be nice if webkit_download_cancel could be reshaped to look more like WKDownload.cancel: and stop using legacyDidCancel.
Here is the first step, adding an async API to GLib: https://github.com/WebKit/WebKit/pull/42334
participants (3)
-
Alex Christensen
-
Michael Catanzaro
-
Patrick Griffis