Changeset 151098 in webkit

Timestamp:

Jun 3, 2013 1:34:34 AM (11 years ago)

Author:

commit-queue@webkit.org

Message:

[EFL][WK2] Process launcher uses system() for wrapping the WebProcess when using WEB_PROCESS_CMD_PREFIX
​https://bugs.webkit.org/show_bug.cgi?id=105156

Patch by Sergio Correia <Sergio Correia> on 2013-06-03
Reviewed by Christophe Dumez.

When using WEB_PROCESS_CMD_PREFIX - which allows us for instance to analyze
WebProcess under tools like valgrind or gdb -, the ProcessLauncher would
spawn the new process using system(), which would, among other things, keep
an extra UIProcess waiting and executing the shell.

This patch handles the normal case and the case where we have something to
prefix WebProcess (i.e., by using WEB_PROCESS_CMD_PREFIX in a debug build)
the same way, through a call to execvp().

To achieve this a function was introduced to create an array with the given
arguments to the full command to be executed, to be used by execvp(). We use
a Vector<OwnArrayPtr<char>>, so that we can take advantage of the destructor
of OwnArrayPtr to handle the memory deallocation when it goes out of scope.

• UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:

(WebKit::createArgsArray): This function creates the array to be used by
execvp(), out of the Strings given as arguments.
(WebKit::ProcessLauncher::launchProcess): Rework the logic to accomodate
both the cases with and without WEB_PROCESS_CMD_PREFIX. The execl() call
was replaced with an execvp() call, since now we should deal with having
a variable number of arguments (WEB_PROCESS_CMD_PREFIX) as well.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/Launcher/efl/ProcessLauncherEfl.cpp (modified) (6 diffs)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2013-06-03  Sergio Correia  <sergio.correia@openbossa.org>
+
+        [EFL][WK2] Process launcher uses system() for wrapping the WebProcess when using WEB_PROCESS_CMD_PREFIX
+        https://bugs.webkit.org/show_bug.cgi?id=105156
+
+        Reviewed by Christophe Dumez.
+
+        When using WEB_PROCESS_CMD_PREFIX - which allows us for instance to analyze
+        WebProcess under tools like valgrind or gdb -, the ProcessLauncher would
+        spawn the new process using system(), which would, among other things, keep
+        an extra UIProcess waiting and executing the shell.
+
+        This patch handles the normal case and the case where we have something to
+        prefix WebProcess (i.e., by using WEB_PROCESS_CMD_PREFIX in a debug build)
+        the same way, through a call to execvp().
+
+        To achieve this a function was introduced to create an array with the given
+        arguments to the full command to be executed, to be used by execvp(). We use
+        a Vector<OwnArrayPtr<char>>, so that we can take advantage of the destructor
+        of OwnArrayPtr to handle the memory deallocation when it goes out of scope.
+
+        * UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:
+        (WebKit::createArgsArray):  This function creates the array to be used by
+        execvp(), out of the Strings given as arguments.
+        (WebKit::ProcessLauncher::launchProcess): Rework the logic to accomodate
+        both the cases with and without WEB_PROCESS_CMD_PREFIX. The execl() call
+        was replaced with an execvp() call, since now we should deal with having
+        a variable number of arguments (WEB_PROCESS_CMD_PREFIX) as well.
+
 2013-06-02  Arunprasad Rajkumar  <arurajku@cisco.com>
 

trunk/Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp

@@ -1 +1 @@
 /*
     Copyright (C) 2012 Samsung Electronics
+    Copyright (C) 2013 Nokia Corporation and/or its subsidiary(-ies).
 
     This library is free software; you can redistribute it and/or
@@ -28 +29 @@
 #include <WebCore/ResourceHandle.h>
 #include <WebCore/RunLoop.h>
+#include <wtf/OwnArrayPtr.h>
 #include <wtf/text/CString.h>
 #include <wtf/text/WTFString.h>
@@ -34 +36 @@
 
 namespace WebKit {
+
+static Vector<OwnArrayPtr<char>> createArgsArray(const String& prefix, const String& executablePath, const String& socket, const String& pluginPath)
+{
+    ASSERT(!executablePath.isEmpty());
+    ASSERT(!socket.isEmpty());
+
+    Vector<String> splitArgs;
+    prefix.split(' ', splitArgs);
+
+    splitArgs.append(executablePath);
+    splitArgs.append(socket);
+    if (!pluginPath.isEmpty())
+        splitArgs.append(pluginPath);
+
+    Vector<OwnArrayPtr<char>> args;
+    args.resize(splitArgs.size() + 1); // Extra room for null.
+
+    size_t numArgs = splitArgs.size();
+    for (size_t i = 0; i < numArgs; ++i) {
+        CString param = splitArgs[i].utf8();
+        args[i] = adoptArrayPtr(new char[param.length() + 1]); // Room for the terminating null coming from the CString.
+        strncpy(args[i].get(), param.data(), param.length() + 1); // +1 here so that strncpy copies the ending null.
+    }
+    // execvp() needs the pointers' array to be null-terminated.
+    args[numArgs] = nullptr;
+
+    return args;
+}
 
 void ProcessLauncher::launchProcess()
@@ -43 +73 @@
     }
 
-    CString executablePath, pluginPath;
+    String processCmdPrefix, executablePath, pluginPath;
     switch (m_launchOptions.processType) {
     case WebProcess:
-        executablePath = executablePathOfWebProcess().utf8();
+        executablePath = executablePathOfWebProcess();
         break;
 #if ENABLE(PLUGIN_PROCESS)
     case PluginProcess:
-        executablePath = executablePathOfPluginProcess().utf8();
-        pluginPath = m_launchOptions.extraInitializationData.get("plugin-path").utf8();
+        executablePath = executablePathOfPluginProcess();
+        pluginPath = m_launchOptions.extraInitializationData.get("plugin-path");
         break;
 #endif
@@ -59 +89 @@
     }
 
-    char socket[5];
-    snprintf(socket, sizeof(socket), "%d", sockets[0]);
-
 #ifndef NDEBUG
-    CString prefixedExecutablePath;
-    if (!m_launchOptions.processCmdPrefix.isEmpty()) {
-        String prefixedExecutablePathStr = m_launchOptions.processCmdPrefix + ' ' +
-            String::fromUTF8(executablePath.data()) + ' ' + socket + ' ' + String::fromUTF8(pluginPath.data());
-        prefixedExecutablePath = prefixedExecutablePathStr.utf8();
-    }
+    if (!m_launchOptions.processCmdPrefix.isEmpty())
+        processCmdPrefix = m_launchOptions.processCmdPrefix;
 #endif
+    Vector<OwnArrayPtr<char>> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath);
 
     // Do not perform memory allocation in the middle of the fork()
@@ -77 +101 @@
     if (!pid) { // Child process.
         close(sockets[1]);
-#ifndef NDEBUG
-        if (!prefixedExecutablePath.isNull()) {
-            // FIXME: This is not correct because it invokes the shell
-            // and keeps this process waiting. Should be changed to
-            // something like execvp().
-            if (system(prefixedExecutablePath.data()) == -1) {
-                ASSERT_NOT_REACHED();
-                exit(EXIT_FAILURE);
-            } else
-                exit(EXIT_SUCCESS);
-        }
-#endif
-        execl(executablePath.data(), executablePath.data(), socket, pluginPath.data(), static_cast<char*>(0));
+        execvp(args.data()[0].get(), reinterpret_cast<char* const*>(args.data()));
     } else if (pid > 0) { // parent process;
         close(sockets[0]);