WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
123201
[WK2] [GTK] Allow running the web process with an arbitrary prefix command
https://bugs.webkit.org/show_bug.cgi?id=123201
Summary
[WK2] [GTK] Allow running the web process with an arbitrary prefix command
Alberto Garcia
Reported
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alberto Garcia
Comment 1
2013-10-23 05:25:01 PDT
Created
attachment 214946
[details]
Patch
Gustavo Noronha (kov)
Comment 2
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?
Sergio Correia (qrwteyrutiyoup)
Comment 3
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?
Alberto Garcia
Comment 4
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).
Alberto Garcia
Comment 5
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.
Alberto Garcia
Comment 6
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.
Alberto Garcia
Comment 7
2013-10-28 01:17:04 PDT
Created
attachment 215287
[details]
Patch The previous patch no longer applies, here's the rebased version.
Carlos Garcia Campos
Comment 8
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
Alberto Garcia
Comment 9
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!
Alberto Garcia
Comment 10
2013-10-28 03:34:52 PDT
Committed
r158105
: <
http://trac.webkit.org/changeset/158105
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug