Using system() will keep one extra UIProcess (in the example bellow, MiniBrowser) waiting and execute the shell. It waste memory that we sometimes cannot afford when debugging in a mobile environment. WEB_PROCESS_CMD_PREFIX="xterm -title WebProcess -e gdb --args" bin/MiniBrowser 1000 7681 10.2 1.1 286572 92612 pts/0 SLl+ 12:12 0:01 bin/MiniBrowser 1000 7685 0.0 0.5 242876 47124 pts/0 S+ 12:12 0:00 bin/MiniBrowser 1000 7686 0.0 0.0 2240 544 pts/0 S+ 12:12 0:00 sh -c xterm -title WebProcess -e gdb --args /opt/tmpsantos/projects/webkit-efl-2/ 1000 7687 0.1 0.0 11496 5776 pts/0 S+ 12:12 0:00 xterm -title WebProcess -e gdb --args /opt/tmpsantos/projects/webkit-efl-2/WebKit 1000 7695 87.3 5.9 1345076 491764 pts/4 Rs+ 12:12 0:09 gdb --args /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Debug/bin/WebProcess 1000 7698 0.4 0.3 168344 32672 pts/4 t 12:12 0:00 /opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Debug/bin/WebProcess 27
Created attachment 200299 [details] Patch Proposed patch
(In reply to comment #1) > Created an attachment (id=200299) [details] > Patch > > Proposed patch Oops, messed up the changelog. Will fix it when adjusting the patch as per the reviewers requests. Sorry.
Created attachment 200383 [details] Patch Fixed changelog mess (I had previously removed trailing whitespaces in messages not related to this patch), and fixed also a typo in the changelog/commit message.
Comment on attachment 200383 [details] Patch Attachment 200383 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/268349 New failing tests: fast/frames/crash-remove-iframe-during-object-beforeload.html
Created attachment 200386 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.2
Comment on attachment 200383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200383&action=review > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] ProcessLauncher: do not handle WEB_PROCESS_CMD_PREFIX with system() Seems that the actual bug has another title. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40 > + ProcessExecArgs(String cmd) const String& > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:46 > + cmd.split(' ', args); so you've composed this string before in ProcessLauncher::launchProcess(), and now you're decomposing it again.. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 > + char* const* args() { return const_cast<char* const*>(m_argv); } why not const char* ? > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71 > + Vector<CString> m_splitArgs; why is that needed? > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:98 > + String fullCmd = String::fromUTF8(executablePath.data()) + ' ' + String::number(sockets[0]) + ' ' + String::fromUTF8(pluginPath.data()); I'd consider using of StringBuilder > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:103 > + ProcessExecArgs execArgs(fullCmd); why do you need a class here? seems a function would be enough
Thanks for the review, Mikhail. (In reply to comment #6) > (From update of attachment 200383 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200383&action=review > > > Source/WebKit2/ChangeLog:3 > > + [EFL][WK2] ProcessLauncher: do not handle WEB_PROCESS_CMD_PREFIX with system() > > Seems that the actual bug has another title. > Yeah, I thought I could change it and ask someone to rename the actual bug, but I will just make the title in the patch to be the same as the bug, instead. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40 > > + ProcessExecArgs(String cmd) > > const String& > > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:46 > > + cmd.split(' ', args); > > so you've composed this string before in ProcessLauncher::launchProcess(), and now you're decomposing it again.. > Indeed. I will pass the possible prefix as a separate parameter and break only it. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 > > + char* const* args() { return const_cast<char* const*>(m_argv); } > > why not const char* ? > I was matching the prototype of execvp(): int execvp(const char *file, char *const argv[]); > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71 > > + Vector<CString> m_splitArgs; > > why is that needed? > Because I was making the items in argv point to them, rather than copying them with something like strdup() - which would require me to free() them in the destructor -, and hence they should exist while argv itself exist. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:98 > > + String fullCmd = String::fromUTF8(executablePath.data()) + ' ' + String::number(sockets[0]) + ' ' + String::fromUTF8(pluginPath.data()); > > I'd consider using of StringBuilder > I am going to take a look at StringBuilder, but probably if I send them as separate arguments, I will be able to avoid composing it here to decompose it in the class. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:103 > > + ProcessExecArgs execArgs(fullCmd); > > why do you need a class here? seems a function would be enough > It seemed simpler to handle the deallocation logic in a destructor, rather than having to do it manually.
Created attachment 200414 [details] Patch Addressing Mikhail's comments: 1) fixed bug title; 2) not composing/decomposing command string; now we decompose the prefix only, and have the other parameters passed as different arguments; 3) not keeping another Vector<String>; now we copy the strings to m_argv with strdup and release them with free in the destructor.
Comment on attachment 200414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200414&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38 > +class ProcessExecArgs { Still think it should not be class, we never need having an object instance. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53 > + for (i = 0, numArgs = prefixArgs.size(); i < numArgs; i++) I'd put numArgs = prefixArgs.size() out of 'for()' > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:101 > + String processCmdPrefix; Seems this var is not needed, m_launchOptions.processCmdPrefix can be passed directly.
Created attachment 201073 [details] Patch Not using a class anymore; we now have a function to create the array and another to deallocate it
(In reply to comment #9) > (From update of attachment 200414 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=200414&action=review > > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38 > > +class ProcessExecArgs { > > Still think it should not be class, we never need having an object instance. > Okay, I removed the class in the new patch. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53 > > + for (i = 0, numArgs = prefixArgs.size(); i < numArgs; i++) > > I'd put numArgs = prefixArgs.size() out of 'for()' > Done. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:101 > > + String processCmdPrefix; > > Seems this var is not needed, m_launchOptions.processCmdPrefix can be passed directly. m_launchOptions.processCmdPrefix only exists under #ifndef NDEBUG, so I think we need this variable, just like we need pluginPath, for instance. Anyway, please take a look at the updated version and let me know if you still think this variable is not needed.
Created attachment 201345 [details] Patch Addressing feedback from Mikhail given on IRC: using a struct as a scope guard to handle the deallocation of the arguments array allocated to be used by execvp().
Comment on attachment 201345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201345&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53 > + for (i = 0; i < numArgs; i++) { Nit: prefixed increment for style > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:54 > + args[i] = new char[strlen(splitArgs[i].utf8().data()) + 1]; We should not call strlen here as the returned CString stores the size. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55 > + strcpy(args[i], splitArgs[i].utf8().data()); You are calling utf8() twice, you should probably store the CString in a local variable > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71 > + for (size_t i = 0; m_args[i]; i++) ++i > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109 > + ArgsScopeGuard guard(args); Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it.
Comment on attachment 201345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201345&action=review >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53 >> + for (i = 0; i < numArgs; i++) { > > Nit: prefixed increment for style Ah, okay, didn't know it was the preferred style. Thanks. >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55 >> + strcpy(args[i], splitArgs[i].utf8().data()); > > You are calling utf8() twice, you should probably store the CString in a local variable Good point, it makes sense to store the CString and use it here and for the length. I will do that. >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109 >> + ArgsScopeGuard guard(args); > > Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it. Good question. I will look into OwnArrayPtr and see if I can find a way to use it here. Thanks for your review and suggestions.
(In reply to comment #14) > (From update of attachment 201345 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201345&action=review > > >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:53 > >> + for (i = 0; i < numArgs; i++) { > > > > Nit: prefixed increment for style > > Ah, okay, didn't know it was the preferred style. Thanks. > > >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:55 > >> + strcpy(args[i], splitArgs[i].utf8().data()); > > > > You are calling utf8() twice, you should probably store the CString in a local variable > > Good point, it makes sense to store the CString and use it here and for the length. I will do that. > > >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:109 > >> + ArgsScopeGuard guard(args); > > > > Isn't there a better way to do that? Maybe using OwnArrayPtr or similar? I need to think a bit more about it. > > Good question. I will look into OwnArrayPtr and see if I can find a way to use it here. Thanks for your review and suggestions. I guess in order to use WTF OwnPtr, you just need to define deleteOwnedPtr(char* const* args), then createArgsArray could return PassOwnPtr.
Created attachment 201462 [details] Patch 1) using prefixed increment style; 2) storing the CString to avoid calling utf8() twice; 3) using OwnArrayPtr + deleteOwnedArrayPtr to take care of the deallocating the memory when the args variable goes out of scope.
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:38 > +template <> void deleteOwnedArrayPtr<char* const>(char* const* args) Someone should confirm but shouldn't this be in wtf/ instead? > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40 > + if (args) { We usually do the opposite in WebKit: Return early. if (!arg) return; > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63 > + char** args = new char*[splitArgs.size() + 1]; // extra room for null We should probably adopt here instead of the return statement. Comment should start with a capital letter and end with a '.'. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:64 > + size_t i, numArgs = splitArgs.size(); Having several statements on the same line is uncommon in WebKit. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:65 > + for (i = 0; i < numArgs; ++i) { i can be declared here: size_t i = 0 > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68 > + strcpy(args[i], param.data()); strncpy? > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:104 > + OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath); Looks much better indeed.
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 > + args[i] = new char[param.length() + 1]; why '+1' ?
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 >> + args[i] = new char[param.length() + 1]; > > why '+1' ? For the ending '\0' character I assume. "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')." strlen() is called inside CString.
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review >>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 >>> + args[i] = new char[param.length() + 1]; >> >> why '+1' ? > > For the ending '\0' character I assume. > "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')." > > strlen() is called inside CString. than we should probably set last byte to null.
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review >>>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 >>>> + args[i] = new char[param.length() + 1]; >>> >>> why '+1' ? >> >> For the ending '\0' character I assume. >> "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')." >> >> strlen() is called inside CString. > > than we should probably set last byte to null. Agreed, would be safer indeed.
Comment on attachment 201462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201462&action=review >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40 >> + if (args) { > > We usually do the opposite in WebKit: Return early. > > if (!arg) > return; Noted. >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:63 >> + char** args = new char*[splitArgs.size() + 1]; // extra room for null > > We should probably adopt here instead of the return statement. Comment should start with a capital letter and end with a '.'. Okay, I will update it to adopt here. >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:65 >> + for (i = 0; i < numArgs; ++i) { > > i can be declared here: size_t i = 0 Will do. >>> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67 >>> + args[i] = new char[param.length() + 1]; >> >> why '+1' ? > > For the ending '\0' character I assume. > "The strlen() function calculates the length of the string s, excluding the terminating null byte ('\0')." > > strlen() is called inside CString. Yes, it's for the ending '\0'. From CString.h: // The data is implicitly allocated 1 character longer than length(), as it is zero-terminated. >> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68 >> + strcpy(args[i], param.data()); > > strncpy? Agreed.
Created attachment 201709 [details] Patch Updated as per Christophe's comments
Comment on attachment 201709 [details] Patch lgtm
Comment on attachment 201709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201709&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:37 > +// FIXME: Should be moved to wtf? Benjamin, what do you think about this one? I think this is generic and potentially reusable.
Comment on attachment 201709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201709&action=review > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:49 > +namespace WTF { > +template <> void deleteOwnedArrayPtr<char* const>(char* const* args) > +{ > + if (!args) > + return; > + > + for (size_t i = 0; args[i]; ++i) > + delete[] args[i]; > + delete[] args; > +} > +} // namespace WTF > + This should not be moved to WTF, this should not exist at all. You can change behaviors of WebKit ADTs with traits and template arguments. But certainly _NOT_ by doing template specialization of internal structure. By doing this, you are breaking the semantic of OwnArrayPtr. Anyone who modify this file later (and any reviewer checking patch) would assume the common well defined behavior. This would introduce subtle bugs and make reviews impossible. > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52 > +static PassOwnArrayPtr<char* const> createArgsArray(const String& prefix, const String& executablePath, const String& socket, const String& pluginPath) Why not just return a Vector<OwnArrayPtr<char* const>> here? > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:106 > + OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath); It looks like this should be in the if (!pid) branch.
(In reply to comment #26) > (From update of attachment 201709 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201709&action=review > > > This should not be moved to WTF, this should not exist at all. > > You can change behaviors of WebKit ADTs with traits and template arguments. But certainly _NOT_ by doing template specialization of internal structure. > > By doing this, you are breaking the semantic of OwnArrayPtr. Anyone who modify this file later (and any reviewer checking patch) would assume the common well defined behavior. This would introduce subtle bugs and make reviews impossible. It seemed to make sense to do this specialization for char*, but I see your point here. Indeed, things like this could introduce near-impossible to debug problems. > > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52 > > +static PassOwnArrayPtr<char* const> createArgsArray(const String& prefix, const String& executablePath, const String& socket, const String& pluginPath) > > Why not just return a Vector<OwnArrayPtr<char* const>> here? > After your suggestion I did some experiments with it and I am soon going to post a patch for review. > > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:106 > > + OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix, executablePath, String::number(sockets[0]), pluginPath); > > It looks like this should be in the if (!pid) branch. Ideally, yes, since it's only used there. However, there is a comment below warning not to perform memory allocation in the middle of the fork()/exec(), due to potential deadlock risks, and hence the allocation part is done in the parent process. Thanks for the your time, Benjamin!
Created attachment 202775 [details] Patch Addressing Benjamin's feedback. Now using a Vector<OwnArrayPtr<char>> to store the arguments to be passed to execvp().
Comment on attachment 202775 [details] Patch Attachment 202775 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/684012
(In reply to comment #29) > (From update of attachment 202775 [details]) > Attachment 202775 [details] did not pass efl-wk2-ews (efl-wk2): > Output: http://webkit-queues.appspot.com/results/684012 I am not able to reproduce this linking problem with either a debug or release build. Does anyone have a clue on what caused it?
I think Gyuyoung is fighting with the EFL bots, so it may not be related to your patch :/
Comment on attachment 202775 [details] Patch I like that new approach. This is EFL code, so it is better if Christophe r+.
Comment on attachment 202775 [details] Patch LGTM.r=me.
Could you please reupload for landing to make sure the ews likes it? I believe Gyuyoung fixed the bots.
(In reply to comment #34) > Could you please reupload for landing to make sure the ews likes it? I believe Gyuyoung fixed the bots. Yes, EFL buildbot and ews are fixed now. Please check this patch on ews again.
Created attachment 203443 [details] Patch Reuploading to make sure EWS is fine with the patch.
Comment on attachment 203443 [details] Patch Attachment 203443 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/662405
(In reply to comment #37) > (From update of attachment 203443 [details]) > Attachment 203443 [details] did not pass efl-wk2-ews (efl-wk2): > Output: http://webkit-queues.appspot.com/results/662405 There is below build error. I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit. There is no same usage in WebKit. Why do you need to use this use ? make[1]: *** [Tools/MiniBrowser/efl/CMakeFiles/MiniBrowser.dir/all] Error 2 ../../lib/libewebkit2.so.0.1.0: undefined reference to `WTF::OwnArrayPtr<char>::OwnArrayPtr(WTF::OwnArrayPtr<char> const&)'
(In reply to comment #38) > I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit. We can use Vector<OwnArrayPtr> if we add an OwnArrayPtr section to the wtf/VectorTraits.h source file.
(In reply to comment #39) > (In reply to comment #38) > > I'm not sure if we can use Vector<OwnArrayPtr<char>> in WebKit. > > We can use Vector<OwnArrayPtr> if we add an OwnArrayPtr section to the wtf/VectorTraits.h source file. Thank you for pointing it out. I file a bug for this(Bug 117131). Could you please review it ?
I think this patch can pass efl-wk2 ews now. Please re-upload one more time.
Created attachment 203558 [details] Patch Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131.
(In reply to comment #42) > Created an attachment (id=203558) [details] > Patch > > Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131. r=me again. FYI, in the future you can simply update the "Reviewed By" lines in the Changelogs and simply set cq? flag since the patch already got r+.
Comment on attachment 203558 [details] Patch Clearing flags on attachment: 203558 Committed r151098: <http://trac.webkit.org/changeset/151098>
All reviewed patches have been landed. Closing bug.
(In reply to comment #43) > (In reply to comment #42) > > Created an attachment (id=203558) [details] [details] > > Patch > > > > Uploading once more; now it might pass EWS, with the change Gyuyoung made in https://bugs.webkit.org/show_bug.cgi?id=117131. > > r=me again. FYI, in the future you can simply update the "Reviewed By" lines in the Changelogs and simply set cq? flag since the patch already got r+. Ah, thanks for this info. I was wondering whether there was some way to "keep" the r+. Thanks for your review again, and thanks to everyone else involved :)