Bug 210366 – [CMake] Determine correct visibility for linked frameworks

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

Each port can set ${framework}_LIBRARY_TYPE to whatever value it wants. This leads to issues with linking frameworks within the WebKit code. For example JavaScriptCore can be compiled as a shared library with the contents of WTF and bmalloc exposed through it.

The CMake source should be able to handle cases like this.

[Don Olmstead]

Created attachment 396142 [details]
WIP Patch

[Don Olmstead]

Created attachment 396143 [details]
WIP Patch

[Don Olmstead]

Created attachment 396153 [details]
WIP Patch

[Don Olmstead]

Created attachment 396154 [details]
WIP Patch

[Don Olmstead]

Created attachment 396171 [details]
Patch

[Don Olmstead]

Created attachment 396172 [details]
WIP Patch

[Don Olmstead]

Created attachment 396180 [details]
WIP Patch

[Don Olmstead]

Created attachment 396181 [details]
WIP Patch

[Don Olmstead]

Created attachment 396182 [details]
WIP Patch

[Don Olmstead]

Created attachment 396184 [details]
WIP Patch

[Don Olmstead]

Created attachment 396193 [details]
WIP Patch

[Don Olmstead]

Created attachment 396194 [details]
WIP Patch

[Don Olmstead]

Created attachment 396314 [details]
WIP Patch

[Don Olmstead]

Created attachment 396851 [details]
WIP Patch

[Don Olmstead]

Created attachment 396853 [details]
WIP Patch

[Don Olmstead]

Created attachment 396854 [details]
WIP Patch

[Don Olmstead]

Created attachment 396928 [details]
WIP Patch

[Don Olmstead]

Created attachment 396929 [details]
WIP Patch

[Don Olmstead]

Created attachment 396931 [details]
WIP Patch

[Don Olmstead]

Created attachment 397389 [details]
WIP Patch

[Don Olmstead]

Created attachment 397473 [details]
WIP Patch

[Don Olmstead]

Created attachment 397475 [details]
WIP Patch

[Don Olmstead]

Created attachment 397487 [details]
WIP Patch

[Don Olmstead]

Created attachment 397491 [details]
WIP Patch

[Don Olmstead]

Created attachment 397498 [details]
WIP Patch

[Don Olmstead]

Created attachment 397698 [details]
WIP Patch

[Don Olmstead]

Created attachment 397701 [details]
WIP Patch

[Don Olmstead]

Created attachment 397703 [details]
WIP Patch

[Don Olmstead]

Created attachment 397717 [details]
WIP Patch

[Don Olmstead]

Created attachment 397724 [details]
WIP Patch

[Don Olmstead]

Created attachment 397726 [details]
WIP Patch

[Don Olmstead]

Created attachment 397728 [details]
WIP Patch

[Don Olmstead]

Created attachment 397735 [details]
WIP Patch

[Don Olmstead]

Created attachment 397747 [details]
WIP Patch

[Don Olmstead]

Created attachment 397787 [details]
WIP Patch

[Don Olmstead]

Created attachment 397788 [details]
WIP Patch

[Don Olmstead]

Created attachment 397790 [details]
WIP Patch

[Don Olmstead]

Created attachment 397793 [details]
WIP Patch

[Don Olmstead]

Created attachment 397798 [details]
WIP Patch

[Don Olmstead]

Created attachment 397799 [details]
WIP Patch

[Don Olmstead]

Created attachment 397844 [details]
WIP Patch

[Don Olmstead]

Created attachment 397853 [details]
WIP Patch

[Don Olmstead]

Created attachment 397874 [details]
WIP Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Don Olmstead]

Created attachment 397877 [details]
WIP Patch

[Don Olmstead]

Created attachment 397881 [details]
WIP Patch

[Don Olmstead]

Created attachment 398065 [details]
WIP Patch

[Don Olmstead]

Created attachment 398075 [details]
WIP Patch

[Michael Catanzaro]

Does this obsolete bug #198093?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #49)
> Does this obsolete bug #198093?

Yes, it would obsolete it :]

[Michael Catanzaro]

It looks like most ports are green, except iOS and JSCOnly. Stalled on those?

[Don Olmstead]

Created attachment 415452 [details]
WIP Patch

[Don Olmstead]

Created attachment 415454 [details]
WIP Patch

[Don Olmstead]

Created attachment 415565 [details]
WIP Patch

[Adrian Perez]

*** Bug 219457 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Looking at the linked bugs, can we also get rid of ADD_WHOLE_ARCHIVE_TO_LIBRARIES? Or reduce its scope such that it's defined only right before it is used?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #56)
> Looking at the linked bugs, can we also get rid of
> ADD_WHOLE_ARCHIVE_TO_LIBRARIES? Or reduce its scope such that it's defined
> only right before it is used?

Yes, I think we should (and could) get rid of it. I am trying to pinpoint
why the GTK bot does not seem happy, let's see if we manage to help Don
get this landed 🛬️

[Don Olmstead]

Created attachment 415701 [details]
WIP Patch

[Don Olmstead]

Created attachment 415702 [details]
WIP Patch

[Don Olmstead]

Created attachment 415703 [details]
WIP Patch

[Don Olmstead]

Created attachment 415706 [details]
WIP Patch

[Don Olmstead]

Created attachment 415708 [details]
WIP Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Don Olmstead]

Created attachment 415709 [details]
WIP Patch

[Don Olmstead]

Created attachment 415713 [details]
WIP Patch

[Don Olmstead]

Created attachment 415715 [details]
WIP Patch

[Don Olmstead]

Created attachment 415720 [details]
WIP Patch

[Don Olmstead]

Created attachment 415721 [details]
Patch

[Don Olmstead]

Comment on attachment 415721 [details]
Patch

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

So GTK and WPE are green! 🎊

The additional exports can probably be split into its own patch and as noted the TextChecker thing needs a resolution.

GTK folks should probably run the tests and everything though as is to make sure that nothing fun cropped up. Other than that though GTK builds with hidden visibility!

> Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:37
> +#if USE(GCRYPT)
> +#include <pal/crypto/gcrypt/Initialization.h>
> +#endif
> +

Same reasoning as WebProcess

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:53
> +#if USE(GCRYPT)
> +        PAL::GCrypt::initialize();
> +#endif
> +

I moved the `USE(GCRYPT)` in here since it makes more sense but also because of WebKit linking WebCore/PAL as PRIVATE libraries so the gcrypt lib and includes don't propagate to the WebProcess build.

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> +    // I think this needs to be in a WK API
> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);

I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?

[Adrian Perez]

(In reply to Don Olmstead from comment #69)
> Comment on attachment 415721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=415721&action=review
> 
> So GTK and WPE are green! 🎊
> 
> The additional exports can probably be split into its own patch and as noted
> the TextChecker thing needs a resolution.
> 
> GTK folks should probably run the tests and everything though as is to make
> sure that nothing fun cropped up. Other than that though GTK builds with
> hidden visibility!

I'm making a build and will run a full run, to see where we are, but I do
not expect (much?) trouble.

> > Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:37
> > +#if USE(GCRYPT)
> > +#include <pal/crypto/gcrypt/Initialization.h>
> > +#endif
> > +
> 
> Same reasoning as WebProcess
> 
> > Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:53
> > +#if USE(GCRYPT)
> > +        PAL::GCrypt::initialize();
> > +#endif
> > +
> 
> I moved the `USE(GCRYPT)` in here since it makes more sense but also because
> of WebKit linking WebCore/PAL as PRIVATE libraries so the gcrypt lib and
> includes don't propagate to the WebProcess build.

This looks sensible to me.

It's actually kind of amusing that this was not done before; I wonder
if the GCrypt initialization code might predate the existence of an
overridable platformInitialize() method 🤔️

> > Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> > +    // I think this needs to be in a WK API
> > +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
> 
> I'm remembering that doing a `WK_EXPORT` on that static method causes the
> Mac builds to fail. Looking at what the Apple stuff is doing you all might
> want to have some kind of internal API that you can do `WK_EXPORT` and call
> that instead. That something the GTK folks can handle here?

I'll take a look.

[Michael Catanzaro]

Comment on attachment 415721 [details]
Patch

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

My only concern is that we need to make sure this does not bring back bug #179914. We can check that by:

 (a) running $ Tools/Scripts/check-for-global-bss-symbols-in-webkigtk-libs _build/lib/libjavascriptcoregtk-4.0.so _build/lib/libwebkit2gtk-4.0.so. (Note: type fixed in bug #219686)
 (b) Sanity-check: add WTFLogAlways() to the PerProcess<> constructor to make sure these objects are still created exactly once and not twice.

Then I'm not sure if it makes sense to keep webkitglib-symbols.map. I guess it doesn't hurt anything, in that it's an extra layer of protection to ensure we don't export things in release builds by mistake. But it also prevents us from running API tests in release builds. So maybe we should delete it.

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:75
> +    // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now.
> +    unsetenv("GTK_THEME");

Nit: I would leave this where it was, since unsetenv is very dangerous, and it's more clear that it comes at the very top of main when there are not objects like WebProcessMain that have to be read and understood to see that.

[Michael Catanzaro]

Comment on attachment 415721 [details]
Patch

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

>> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:75
>> +    // Ignore the GTK_THEME environment variable, the theme is always set by the UI process now.
>> +    unsetenv("GTK_THEME");
> 
> Nit: I would leave this where it was, since unsetenv is very dangerous, and it's more clear that it comes at the very top of main when there are not objects like WebProcessMain that have to be read and understood to see that.

Actually, hi Carlos, is this obsolete? Since the web process doesn't use the GTK theme anymore?

[Adrian Perez]

Comment on attachment 415721 [details]
Patch

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

> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:76
> +

I *think* that we might be able to remove this now that we are not using
the GTK external rendering anymore for controls and we let WebKit paint
them.

At any rate, not a blocker for landing. I'll double check with Carlos
García, and we may was well zap this in a follow-up patch.

> Source/cmake/OptionsGTK.cmake:28
> +    set(CMAKE_CXX_VISIBILITY_PRESET hidden)

We should probably throw in also a

    set(CMAKE_C_VISIBILITY_PRESET hidden)

as we have a few plain C files which are part of the build as well.

>>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
>>> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
>> 
>> I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?
> 
> I'll take a look.

I think we can get away with using the public API, because at this point
in the code we know for a fact that the web view being used is a GTK
WebKitWebView. I did this locally, and currently running tests, I'll let
you know how it goes:


--- a/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
+++ b/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
@@ -77,8 +77,9 @@ void UIScriptControllerGtk::doAsyncTask(JSValueRef callback)

 void UIScriptControllerGtk::setContinuousSpellCheckingEnabled(bool enabled)
 {
-    // I think this needs to be in a WK API
-    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
+    auto* webView = TestController::singleton().mainWebView();
+    auto* webContext = webkit_web_view_get_context(webView);
+    webkit_web_context_set_spell_checking_enabled(webContext, enabled ? TRUE : FALSE);
 }

 void UIScriptControllerGtk::copyText(JSStringRef text)

[Michael Catanzaro]

(In reply to Adrian Perez from comment #73)
> We should probably throw in also a
> 
>     set(CMAKE_C_VISIBILITY_PRESET hidden)
> 
> as we have a few plain C files which are part of the build as well.

Oops, yes please. :)

[Carlos Garcia Campos]

Comment on attachment 415721 [details]
Patch

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

>> Source/WebKit/WebProcess/gtk/WebProcessMainGtk.cpp:76
>> +
> 
> I *think* that we might be able to remove this now that we are not using
> the GTK external rendering anymore for controls and we let WebKit paint
> them.
> 
> At any rate, not a blocker for landing. I'll double check with Carlos
> García, and we may was well zap this in a follow-up patch.

Note that we sill use GtkStyleContext in GTK3 to get the selection colors

>>>> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
>>>> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
>>> 
>>> I'm remembering that doing a `WK_EXPORT` on that static method causes the Mac builds to fail. Looking at what the Apple stuff is doing you all might want to have some kind of internal API that you can do `WK_EXPORT` and call that instead. That something the GTK folks can handle here?
>> 
>> I'll take a look.
> 
> I think we can get away with using the public API, because at this point
> in the code we know for a fact that the web view being used is a GTK
> WebKitWebView. I did this locally, and currently running tests, I'll let
> you know how it goes:
> 
> 
> --- a/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
> +++ b/Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp
> @@ -77,8 +77,9 @@ void UIScriptControllerGtk::doAsyncTask(JSValueRef callback)
> 
>  void UIScriptControllerGtk::setContinuousSpellCheckingEnabled(bool enabled)
>  {
> -    // I think this needs to be in a WK API
> -    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);
> +    auto* webView = TestController::singleton().mainWebView();
> +    auto* webContext = webkit_web_view_get_context(webView);
> +    webkit_web_context_set_spell_checking_enabled(webContext, enabled ? TRUE : FALSE);
>  }
> 
>  void UIScriptControllerGtk::copyText(JSStringRef text)

I don't this that works, in WTR there's not a WebKitWebView, it's a WebKitWebViewBase

[Don Olmstead]

Created attachment 415922 [details]
WIP Patch

[Don Olmstead]

Created attachment 415930 [details]
No GTK hidden visibility

[Don Olmstead]

Created attachment 415932 [details]
No GTK hidden visibility

[Don Olmstead]

Comment on attachment 415932 [details]
No GTK hidden visibility

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

I have https://bugs.webkit.org/show_bug.cgi?id=219749 with the gcrypt changes ready for review. Other than that I feel this patch is about ready for review so feel free to leave comments on it.

> Source/WebCore/PlatformGTK.cmake:16
> +# Remove when turning on hidden visibility
> +list(APPEND WebCore_PRIVATE_LIBRARIES WebKit::WTF)
> +if (NOT USE_SYSTEM_MALLOC)
> +    list(APPEND WebCore_PRIVATE_LIBRARIES WebKit::bmalloc)
> +endif ()
> +

So at this time it looks like GTK isn't ready to turn on hidden visibility so I added this in.

Could we open a bug for turning on hidden visibility for GTK and potentially land this with a FIXME pointing at that bug.

I'm happy to work with Adrian or whoever on getting hidden visibility working.

[Don Olmstead]

Found https://bugs.webkit.org/show_bug.cgi?id=181916 so I'll just point things at that.

[Don Olmstead]

Created attachment 416001 [details]
Patch

[EWS]

Committed r270690: <https://trac.webkit.org/changeset/270690>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416001 [details].