Bug 123201 - [WK2] [GTK] Allow running the web process with an arbitrary prefix command
Summary: [WK2] [GTK] Allow running the web process with an arbitrary prefix command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alberto Garcia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-23 05:15 PDT by Alberto Garcia
Modified: 2013-10-28 03:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.05 KB, patch)
2013-10-23 05:25 PDT, Alberto Garcia
gustavo: review-
Details | Formatted Diff | Diff
Patch (3.89 KB, patch)
2013-10-23 14:54 PDT, Alberto Garcia
no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2013-10-28 01:17 PDT, Alberto Garcia
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alberto Garcia 2013-10-23 05:15:45 PDT
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
Comment 1 Alberto Garcia 2013-10-23 05:25:01 PDT
Created attachment 214946 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-10-23 09:49:36 PDT
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?
Comment 3 Sergio Correia (qrwteyrutiyoup) 2013-10-23 09:58:51 PDT
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?
Comment 4 Alberto Garcia 2013-10-23 10:11:37 PDT
(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).
Comment 5 Alberto Garcia 2013-10-23 10:21:24 PDT
(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.
Comment 6 Alberto Garcia 2013-10-23 14:54:54 PDT
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.
Comment 7 Alberto Garcia 2013-10-28 01:17:04 PDT
Created attachment 215287 [details]
Patch

The previous patch no longer applies, here's the rebased version.
Comment 8 Carlos Garcia Campos 2013-10-28 01:38:01 PDT
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
Comment 9 Alberto Garcia 2013-10-28 01:56:41 PDT
(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!
Comment 10 Alberto Garcia 2013-10-28 03:34:52 PDT
Committed r158105: <http://trac.webkit.org/changeset/158105>