Bug 220090 – [WPE][GTK] Remove unnecessary child setup function from process launcher

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

The process launcher currently uses a child setup function to close the server end of its IPC socket. But this is totally unnecessary because this socket always uses CLOEXEC. So the child setup function is totally redundant.

This is step one towards making Eclipse not crash when its UI process JVM is using a huge amount of memory. In such conditions, fork() will fail due to OOM, but posix_spawn() will still succeed. A child setup function forces GLib to use fork() instead of posix_spawn(). However, this commit is not sufficient to fix Eclipse, because GSubprocess itself sets its own child setup function.

[Michael Catanzaro]

Created attachment 416674 [details]
Patch

[Michael Catanzaro]

Comment on attachment 416674 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:163
>      GRefPtr<GSubprocessLauncher> launcher = adoptGRef(g_subprocess_launcher_new(G_SUBPROCESS_FLAGS_INHERIT_FDS));
> -    g_subprocess_launcher_set_child_setup(launcher.get(), childSetupFunction, GINT_TO_POINTER(socketPair.server), nullptr);
>      g_subprocess_launcher_take_fd(launcher.get(), socketPair.client, socketPair.client);

Note that g_subprocess_launcher_take_fd() is not actually doing anything when G_SUBPROCESS_FLAGS_INHERIT_FDS is set.

Technically, I think the best solution is to get rid of G_SUBPROCESS_FLAGS_INHERIT_FDS if it doesn't break anything, and rely on g_subprocess_launcher_take_fd() to expose the IPC socket. That would subvert the goal of fixing Eclipse, but it's probably the right thing for WebKit. I will investigate.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> Technically, I think the best solution is to get rid of
> G_SUBPROCESS_FLAGS_INHERIT_FDS if it doesn't break anything, and rely on
> g_subprocess_launcher_take_fd() to expose the IPC socket. That would subvert
> the goal of fixing Eclipse, but it's probably the right thing for WebKit. I
> will investigate.

In fact, I'm almost certain we have a fd leak somewhere. Each time we spawn a new web process, the number of open fds in the new process is higher than in previous processes. That doesn't make sense: it should be about the same. No clue where it's coming from; probably something somewhere should be using CLOEXEC but is failing to do so.

If we get rid of G_SUBPROCESS_FLAGS_INHERIT_FDS, this leak goes away, and I don't notice any behavior changes, so I think we should do it. Applications that use outrageous amounts of UI process memory will just need to find some other solution for their fork() woes.

[Michael Catanzaro]

OK, ping reviewers. I think we can commit this as-is.

(In reply to Michael Catanzaro from comment #2)
> Note that g_subprocess_launcher_take_fd() is not actually doing anything
> when G_SUBPROCESS_FLAGS_INHERIT_FDS is set.

Oops, not quite. It ensures that the client socket is closed in the parent process when the GSubprocessLauncher is finalized. Might be more clear to do that manually, of course.

(In reply to Michael Catanzaro from comment #3)
> If we get rid of G_SUBPROCESS_FLAGS_INHERIT_FDS, this leak goes away, and I
> don't notice any behavior changes, so I think we should do it. Applications
> that use outrageous amounts of UI process memory will just need to find some
> other solution for their fork() woes.

Well although it works for GTK, I'm sure it will break WPE, since there we need to inherit the WPE socket in addition to the WebKit IPC socket as well. And if we get rid of G_SUBPROCESS_FLAGS_INHERIT_FDS, then this seems hard to do, unless we are willing to revert r241816.

Reported bug #220335 for our fd leak.

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Ping reviewers

[Carlos Garcia Campos]

Comment on attachment 416674 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:-50
> -    int socket = GPOINTER_TO_INT(userData);
> -    close(socket);

So, does this fail because it's already closed?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> So, does this fail because it's already closed?

No, it will succeed, because exec() has not yet been called. But it's also not needed because CLOEXEC is set here:

IPC::Connection::SocketPair socketPair = IPC::Connection::createPlatformConnection(IPC::Connection::ConnectionOptions::SetCloexecOnServer);

so it will be closed automatically regardless of whether we call close() manually.

[EWS]

commit-queue failed to commit attachment 416674 [details] to WebKit repository.

[EWS]

commit-queue failed to commit attachment 416674 [details] to WebKit repository.

[EWS]

Committed r271610: <https://trac.webkit.org/changeset/271610>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416674 [details].