Bug 245784 – REGRESSION(254232@main): Causes process launching to use fork + exec instead of posix_spawn

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

[Michael Catanzaro]

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

[Michael Catanzaro]

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?

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[EWS]

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

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