Summary: | [WPE][GTK] Merge ProcessLauncher[WPE,GTK] | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||
Component: | WPE WebKit | Assignee: | Michael Catanzaro <mcatanzaro> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, mcatanzaro, webkit-bug-importer | ||||
Priority: | P2 | ||||||
Version: | Other | ||||||
Hardware: | PC | ||||||
OS: | Linux | ||||||
Attachments: |
|
Description
Michael Catanzaro
2017-09-16 14:54:29 PDT
Created attachment 321014 [details]
Patch
Comment on attachment 321014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321014&action=review > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:104 > + nargs = 5; nargs++ here would be safer in case we add a new one > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:113 > + for (auto it = splitArgs.begin(); it != splitArgs.end(); it++) I know this was existing code, but we could take advantage of this patch to modernize it a bit. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:123 > + for (auto it = prefixArgs.begin(); it != prefixArgs.end(); it++) Ditto. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:132 > + argv[i++] = const_cast<char*>(realPluginPath.data()); I think this need to be protected by a ENABLE(NETSCAPE_PLUGIN_API) ifdef. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:136 > + if (!g_spawn_async(0, argv, 0, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, childSetupFunction, GINT_TO_POINTER(socketPair.server), &pid, &error.outPtr())) { 0 -> nullptr > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137 > + g_printerr("Unable to fork a new WebProcess: %s.\n", error->message); WTFLogAlways? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:151 > + RefPtr<ProcessLauncher> protectedThis(this); > + IPC::Connection::Identifier serverSocket = socketPair.server; > + RunLoop::main().dispatch([protectedThis, pid, serverSocket] { RunLoop::main().dispatch([protectedThis = makeRef(*this), pid, serverSocket = socketPair.server] { > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:152 > + protectedThis->didFinishLaunchingProcess(pid, serverSocket); You can also capture this for convenience and then you don't need to capture pid didFinishLaunchingProcess(m_processIdentifier, serverSocket); I know all this is just copied code, not yours, I don't like making refactorings or other changes in behavior in this kind of patches, but coding style and modernization is always welcome, I think :-) (In reply to Carlos Garcia Campos from comment #2) > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:132 > > + argv[i++] = const_cast<char*>(realPluginPath.data()); > > I think this need to be protected by a ENABLE(NETSCAPE_PLUGIN_API) ifdef. Yes, I broke this by moving realPluginPath inside the ifdef. I'll do this: #if ENABLE(NETSCAPE_PLUGIN_API) argv[i++] = const_cast<char*>(realPluginPath.data()); #else argv[i++] = nullptr; #endif argv[i++] = nullptr; This will avoid leaving uninitialized stack memory in that index. It's awkward. This messiness should all be replaced with a Vector, but that will be a follow-up patch. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137 > > + g_printerr("Unable to fork a new WebProcess: %s.\n", error->message); > > WTFLogAlways? This should be a fatal error, so I'll change it to g_error(). I'll also change the error message to say "child process" rather than "WebProcess" since that has been on my TODO list for at least two years now. Also, "g_printerr() should not be used from within libraries" according to its documentation. Committed r222133: <http://trac.webkit.org/changeset/222133> Committed r222134: <http://trac.webkit.org/changeset/222134> |