Bug 123201 – [WK2] [GTK] Allow running the web process with an arbitrary prefix command

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

[Alberto Garcia]

I think it would be useful to have a way to pass an arbitrary prefix
to the command line when launching the web process in order to debug
webkit more easily. Similar to what the EFL or the (former) Qt port
do.

I wrote a patch that uses the WEB_PROCESS_CMD_PREFIX environment
variable for that purpose.

Example:

$ WEB_PROCESS_CMD_PREFIX='/usr/bin/gdbserver localhost:8080' WebKitBuild/Debug/Programs/MiniBrowser

and in a different terminal:

$ gdb WebKitBuild/Debug/Programs/.libs/WebKitWebProcess
(gdb) target remote localhost:8080

[Alberto Garcia]

Created attachment 214946 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 214946 [details]
Patch

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

Think the idea is a great one, would prefer the impl to be simplified a bit.

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:104
> +    if (!m_launchOptions.processCmdPrefix.isEmpty()) {
> +        Vector<String> splitted;
> +        m_launchOptions.processCmdPrefix.split(' ', splitted);
> +        for (auto it = splitted.begin(); it != splitted.end(); it++)
> +            prefixArgs.append(it->utf8());

Can we do away with this splitted intermediate vector? Are you doing this to make sure the CString is kept alive? I think it would be better to use g_strdup if that is the case - not a lot of overhead, way simpler code.

> Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:29
>  
> +#include <glib.h>

This should not have an empty line.

> Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:37
> +    if (webProcessCmdPrefix && *webProcessCmdPrefix)

Why do you do this check on the dereferenced pointer?

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 214946 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:123
>      if (!g_spawn_async(0, argv, 0, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, childSetupFunction, GINT_TO_POINTER(sockets[1]), &pid, &error.outPtr())) {

Maybe we could add G_SPAWN_SEARCH_PATH to the spawn flags as well, so that we would be able to pass something like "gdbserver localhost:8080" to the WEB_PROCESS_CMD_PREFIX variable and find gdbserver on the PATH?

[Alberto Garcia]

(In reply to comment #2)
> > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:104
> > +    if (!m_launchOptions.processCmdPrefix.isEmpty()) {
> > +        Vector<String> splitted;
> > +        m_launchOptions.processCmdPrefix.split(' ', splitted);
> > +        for (auto it = splitted.begin(); it != splitted.end(); it++)
> > +            prefixArgs.append(it->utf8());
>
> Can we do away with this splitted intermediate vector?

String::split() returns Strings, but we cannot get the raw data
directly from them.

Using only one vector<String> and something like

  argv[i++] = it->utf8().data();

won't work because the intermediate CStrings which hold the data are
immediately destroyed.

The problem of g_strdup() is that we have to free that memory manually
afterwards, but the rest of the contents of the argv vector are freed
automatically.

We can also duplicate all the strings in the argv vector, but I was
looking for a solution that doesn't add overhead to release builds.

> > Source/WebKit2/UIProcess/gtk/WebProcessProxyGtk.cpp:37
> > +    if (webProcessCmdPrefix && *webProcessCmdPrefix)
>
> Why do you do this check on the dereferenced pointer?

It just checks that the string is not empty (this part is actually
copied from the EFL code).

[Alberto Garcia]

(In reply to comment #3)
> Maybe we could add G_SPAWN_SEARCH_PATH to the spawn flags as well,
> so that we would be able to pass something like "gdbserver
> localhost:8080" to the WEB_PROCESS_CMD_PREFIX variable and find
> gdbserver on the PATH?

Dunno, I guess we're always using the full path to spawn processes so
it should be safe to do it, but I'm fine leaving it as it is now.

[Alberto Garcia]

Created attachment 215001 [details]
Patch

I fixed the style problem in WebProcessProxy.cpp but otherwise the
code is essentially the same.

I rewrote the ProcessLauncher::launchProcess() part so it's hopefully
a bit clearer now.

As I explained earlier I don't think it's easy to do without the
intermediate array. It's anyway necessary to split the string first in
order to know how much memory we need to allocate for argv.

In this patch I also put the prefixArgs vector inside the #ifndef NDEBUG
block, so for release builds the code is almost identical.

[Alberto Garcia]

Created attachment 215287 [details]
Patch

The previous patch no longer applies, here's the rebased version.

[Carlos Garcia Campos]

Comment on attachment 215287 [details]
Patch

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

This looks good to me. Thanks.

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:105
> +    if (!m_launchOptions.processCmdPrefix.isEmpty()) {

You are preventing this from being empty in WebProcessProxy::platformGetLaunchOptions, so we could probably check whether it's null instead.

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:106
> +        Vector<String> splitted;

I'm not an English expert at all, but I think splitted doesn't exists. We could call this something like splitArgs or splitPrefixArgs

[Alberto Garcia]

(In reply to comment #8)

> You are preventing this from being empty in
> WebProcessProxy::platformGetLaunchOptions, so we could probably
> check whether it's null instead.

> I'm not an English expert at all, but I think splitted doesn't
> exists. We could call this something like splitArgs or
> splitPrefixArgs

You're right in both cases, I'll fix these and commit. Thanks!

[Alberto Garcia]

Committed r158105: <http://trac.webkit.org/changeset/158105>