[webkit-reviews] review granted: [Bug 179914] [GTK] Duplicated symbols in libjavascriptcoregtk and libwebkit2gtk can cause crashes in production builds : [Attachment 330061] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Michael Catanzaro
<mcatanzaro at igalia.com>'s request for review:
Bug 179914: [GTK] Duplicated symbols in libjavascriptcoregtk and libwebkit2gtk
can cause crashes in production builds
https://bugs.webkit.org/show_bug.cgi?id=179914

Attachment 330061: Patch

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




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

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitExtensionManager.h:23
> +#include "WebKitDefines.h"

Why are you including this API header here?

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitExtensionManager.h:42
> +    __attribute__((visibility("default"))) static WebKitExtensionManager&
singleton();

Could you use WTF_EXPORT instead? Whye do we need this now? This is in the
version script for production builds, and already exported in developer builds.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitExtensionManager.h:44
> +    __attribute__((visibility("default"))) void initialize(InjectedBundle*,
API::Object*);

Ditto.

> Source/cmake/OptionsGTK.cmake:111
> +	   set(JavaScriptCore_VERSION_SCRIPT
"-Wl,--version-script,${CMAKE_MODULE_PATH}/gtk/javascriptcoregtk-symbols.map")
> +	   set(WebKit_VERSION_SCRIPT
"-Wl,--version-script,${CMAKE_MODULE_PATH}/gtk/webkit2gtk-symbols.map")

I also thought about renaming the version script, but I would also move them to
a different place, I don't think they belong to CMAKE_MODULE_PATH. I would move
javascriptcoregtk-symbols.map to Source/JavaScriptCore which is where the pc
and git files are, for example and other similar files like
JavaScriptCore.order (or use a gtk subdir if you prefer). And
webkit2gtk-symbols.map to Source/WebKit/gtk for the same reasons. Also since
the condition is only developer mode, I don't think we need to check it here
and set these variables, we could simply check if developer mode is enabled in
jsc and wk makefiles directly to include the link flags.

> Source/cmake/wpe/wpewebkit-symbols.map:14
> +};

Does it make sense to keep this duplicated file? Maybe we can use a single file
using glib instead of gtk webkitglib-symbols.map.