Bug 245784

Summary:	REGRESSION(254232@main): Causes process launching to use fork + exec instead of posix_spawn
Product:	WebKit	Reporter:	Michael Catanzaro <mcatanzaro>
Component:	WebKitGTK	Assignee:	Michael Catanzaro <mcatanzaro>
Status:	RESOLVED FIXED
Severity:	Normal	CC:	bugs-noreply, mcatanzaro, webkit-bug-importer
Priority:	P2
Version:	WebKit Nightly Build
Hardware:	PC
OS:	Linux
See Also:	https://bugzilla.redhat.com/show_bug.cgi?id=2130294 https://bugs.webkit.org/show_bug.cgi?id=221489 https://bugzilla.redhat.com/show_bug.cgi?id=2132776

Description Michael Catanzaro 2022-09-28 08:19:57 PDT

254232@main removed use of G_SUBPROCESS_FLAGS_INHERIT_FDS since we actually do not ever want to inherit fds into our child processes, except for those that we explicitly pass along using g_subprocess_launcher_take_fd(). This was a good change since it makes WebKit more robust to file descriptor leaks, but there is an unfortunate side effect: it prevents gspawn from using posix_spawn() to launch subprocesses, so we wind up falling back to fork() and exec() instead. I had left an insufficient warning comment about this:

     // Warning: do not set a child setup function, because we want GIO to be able to spawn with
     // posix_spawn() rather than fork()/exec(), in order to better accommodate applications that use
     // a huge amount of memory or address space in the UI process, like Eclipse.
     GRefPtr<GSubprocessLauncher> launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_INHERIT_FDS));

but I only mentioned one of the many things that could cause us to fall back to posix_spawn(), and didn't mention that G_SUBPROCESS_FLAGS_INHERIT_FDS was important.

How unfortunate. :(

Comment 1 Michael Catanzaro 2022-09-30 07:40:30 PDT

So I've been thinking about this... it really needs to be more robust, less fragile. We could add a new flag in both GSpawnFlags and GSubprocessFlags to cause process launching to fail if fork()/exec() is required. The flags would do nothing on Windows. That way, we can't mess up anymore.

Sadly I can't think of any way to keep the nice behavior of closing all leaked file descriptors. Carlos, are you OK with reverting this commit and allowing leaked descriptors to actually leak again? It is unfortunate, but seems we have to choose between this or posix_spawn(), because there is no race-free way to close descriptors with posix_spawn() except to ensure they all have CLOEXEC flags set. It's possible to do with fork() only because no secondary threads exist after fork() is called. Anyway, I think this is OK because CLOEXEC is really the proper solution to fd leaks anyway: it's nice that gspawn can close anything we miss, but not mandatory IMO. Do you agree?

Comment 2 Michael Catanzaro 2022-09-30 08:06:14 PDT

Oh, and I think we can avoid reintroducing bug #221489 by just always setting CLOEXEC on the socket and relying on g_subprocess_launcher_take_fd() to unset it. (Except if the new WPE process launching API is used, in which case we can either not set CLOEXEC, or else change the API to deal with it.)

Comment 3 Michael Catanzaro 2022-09-30 10:05:22 PDT

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

Comment 4 EWS 2022-10-09 06:43:01 PDT

Committed 255325@main (9d195cb5d885): <https://commits.webkit.org/255325@main>

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