[webkit-reviews] review denied: [Bug 177052] [WPE][GTK] Further cleanups in ProcessLauncherGLib.cpp : [Attachment 321051] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 177052: [WPE][GTK] Further cleanups in ProcessLauncherGLib.cpp
https://bugs.webkit.org/show_bug.cgi?id=177052

Attachment 321051: Patch

https://bugs.webkit.org/attachment.cgi?id=321051&action=review




--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 321051
  --> https://bugs.webkit.org/attachment.cgi?id=321051
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321051&action=review

I think we are complicating the code to avoid just a few copies/allocations. I
think it would be easier to take the opposite approach, and make the argv take
the ownership of the strings. We could use a GPtrArray instead of a vector.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:61
> +    // Footgun warning: the argv vector does not own the strings it
contains, so
> +    // the owning containers need to stay in scope. Seems safest to declare
them
> +    // all way up here.

This could happen if argv takes the ownership of strings.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:107
>	   m_launchOptions.processCmdPrefix.split(' ', splitArgs);

Here we could use g_strsplit and pass the strings to the array, only freeing
the container.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:109
> +	       argv.append(arg.utf8().data());

This is still super-wrong. utf8() returns a temporary, so you can't save the
data() here, it will be freed right after the append.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:112
> +    argv.reserveInitialCapacity(4);

If we keep using a Vector, it's better to pre-allocate this in the stack i
think. Vector<const char*, 4>. If we use a GPtrArray we could use
g_ptr_array_new_full() to provide this as initial size and g_free as free func.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:114
> +    realExecutablePath = fileSystemRepresentation(executablePath).data();

This is the only copy we would do, we can either keep using
fileSystemRepresentation() adn g_strdup() the .data(), or use
g_uri_unescape_string() directly, which is what fileSystemRepresentation() does
in the end.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:117
> +    webkitSocket.reset(g_strdup_printf("%d", socketPair.client));

This would be added to the array directly.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:121
> +	   wpeSocket.reset(g_strdup_printf("%d",
wpe_renderer_host_create_client()));

Ditto.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:126
> +    argv.append(realPluginPath.data());

And here again we could dup or use g_uri_unescape_string().