Bug 153176 - [CMake][GTK][OSX] ThreadIdentifierData::initialize assertion fails because there are two copies of WTF in process (one as part of webkit2, one as part of jsc)
Summary: [CMake][GTK][OSX] ThreadIdentifierData::initialize assertion fails because th...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 126492
  Show dependency treegraph
 
Reported: 2016-01-15 19:58 PST by Jeremy Huddleston Sequoia
Modified: 2016-03-14 08:05 PDT (History)
4 users (show)

See Also:


Attachments
Doesn't fix the problem (2.78 KB, patch)
2016-03-13 21:17 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Huddleston Sequoia 2016-01-15 19:58:08 PST
Reported on MacPorts at https://trac.macports.org/ticket/50339

After working through various build issues (bug #152641, bug #152720, bug #153117, bug #153120, bug #153138), we get a successful build, but the resulting library aborts at initialization.

The issue is in ThreadIdentifierData::initialize(ThreadIdentifier id), we're failing the RELEASE_ASSERT.  m_key is PTHREAD_KEYS_MAX for some reason.

void ThreadIdentifierData::initialize(ThreadIdentifier id)
{
    ASSERT(!identifier());
    // Ideally we'd have this as a release assert everywhere, but that would hurt performane.
    // Having this release assert here means that we will catch "didn't call
    // WTF::initializeThreading() soon enough" bugs in release mode.
    RELEASE_ASSERT(m_key != PTHREAD_KEYS_MAX);
    pthread_setspecific(m_key, new ThreadIdentifierData(id));
}

---

I didn't quite believe it, so I changed the RELEASE_ASSERT to a real assert(), and sure enough:

Assertion failed: (m_key != PTHREAD_KEYS_MAX), function identifier, file /.../webkitgtk-2.11.3/Source/WTF/wtf/ThreadIdentifierDataPthreads.cpp, line 64.
Comment 1 Jeremy Huddleston Sequoia 2016-01-15 22:16:48 PST
Oh... that's not surprising at all.  It's set to that value initially:

namespace WTF {
pthread_key_t ThreadIdentifierData::m_key = PTHREAD_KEYS_MAX;
...

So why the assertion?  Don't we expect it to be PTHREAD_KEYS_MAX initially?
Comment 2 Jeremy Huddleston Sequoia 2016-01-15 22:24:55 PST
m_key is supposed to be setup in WTF::ThreadIdentifierData::initializeOnce(), which is called by initializeThreading WTF::initializeThreading().  I'm guessing WTF::initializeThreading() isn't getting called like we'd expect.

Who is responsible for calling WTF::initializeThreading()?  I don't see it being called anywhere in the library.  I see it called in main() in some things in WTF/benchmarks.  Is the client responsible for calling that?  It seems odd that the library doesn't handle it.
Comment 3 Jeremy Huddleston Sequoia 2016-01-15 22:44:49 PST
Oh, I missed it.

JSC::initializeThreading() calls WTF::initializeThreading(), and JSC::initializeThreading() is the first thing called by WebKit2::InitializeWebKit2

namespace JSC {
void initializeThreading()
{
    static std::once_flag initializeThreadingOnceFlag;

    std::call_once(initializeThreadingOnceFlag, []{
        WTF::double_conversion::initialize();
        WTF::initializeThreading();
...

---

void InitializeWebKit2()
{
#if PLATFORM(COCOA)
    InitWebCoreSystemInterface();
#endif
#if PLATFORM(IOS)
    InitWebCoreThreadSystemInterface();
#endif

    JSC::initializeThreading();
    WTF::initializeMainThread();
...

So that all looks OK, BUT the issue comes because of 2-level name-spacing.  Here's the backtrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib              0x00007fff8ac13f06 __pthread_kill + 10
1   libsystem_pthread.dylib             0x00007fff8fa204ec pthread_kill + 90
2   libsystem_c.dylib                   0x00007fff8b86e6e7 abort + 129
3   libsystem_c.dylib                   0x00007fff8b835df8 __assert_rtn + 321
4   libwebkit2gtk-4.0.37.dylib          0x000000010b27cb54 WTF::ThreadIdentifierData::identifier() + 68
5   libwebkit2gtk-4.0.37.dylib          0x000000010b27db3d WTF::currentThread() + 13
6   libwebkit2gtk-4.0.37.dylib          0x000000010b222e32 WTF::initializeMainThread() + 34
7   libwebkit2gtk-4.0.37.dylib          0x0000000108e0236e WebKit::InitializeWebKit2() + 14

libwebkit2gtk's WTF::initializeMainThread() was never called.

libjavascriptcoregtk has its own copy of WTF::initializeMainThread(), and THAT was what was called:


webkitgtk-2.11.3/lib $ nm libjavascriptcoregtk-4.0.dylib | grep initializeOnce
0000000000ac29a0 T __ZN3WTF20ThreadIdentifierData14initializeOnceEv
0000000000e3f9f0 b __ZZN3WTF12_GLOBAL__N_112myThreadDataEvE14initializeOnce
0000000000e3fa30 b __ZZN3WTF12_GLOBAL__N_112myThreadDataEvE14initializeOnce

webkitgtk-2.11.3/lib $ nm libwebkit2gtk-4.0.dylib | grep initializeOnce
0000000002634a00 T __ZN3WTF20ThreadIdentifierData14initializeOnceEv
0000000003702500 b __ZZN3WTF12_GLOBAL__N_112myThreadDataEvE14initializeOnce
00000000037024b0 b __ZZN3WTF12_GLOBAL__N_112myThreadDataEvE14initializeOnce
Comment 4 Jeremy Huddleston Sequoia 2016-01-16 14:17:21 PST
In webkit-gtk 2.4.x, WTF is not exported by libwebkitgtk.  It's exported by libjavascriptcoregtk.  Thus, I suspect the appropriate fix is to remove WTF from libwebkit2gtk.  This was likely fallout from the autoconf -> cmake transition.
Comment 5 Michael Catanzaro 2016-01-16 15:30:20 PST
(In reply to comment #4)
> In webkit-gtk 2.4.x, WTF is not exported by libwebkitgtk.  It's exported by
> libjavascriptcoregtk.  Thus, I suspect the appropriate fix is to remove WTF
> from libwebkit2gtk.  This was likely fallout from the autoconf -> cmake
> transition.

Sounds fine to me. In Source/WebKit2/CMakeLists.txt, try removing JavaScriptCore (which depends on WTF) from here:

set(WebKit2_LIBRARIES
    JavaScriptCore
    WebCore
)

Then see if it builds....
Comment 6 Jeremy Huddleston Sequoia 2016-01-16 21:16:58 PST
That does nothing because WebCore also depends on JavaScriptCore.

Also removing JavaScriptCore and WTF from WebCore gets us a link line that lacks both  libjavascriptcoregtk-4.0.18.3.2.dylib and libWTFGTK.a.  Thus we fail because we need to link against libjavascriptcoregtk-4.0.18.3.2.dylib (but not libWTFGTK).

Just removing WTF from WebCore_LIBRARIES doesn't work either.  We still end up linking against both.

Just removing both JavaScriptCore and WTF from WebCore_LIBRARIES doesn't work either.  We still end up linking against both.

I can't see how to get the build system to link against libjavascriptcoregtk but not libWTFGTK.

cmake drives me crazy...
Comment 7 Jeremy Huddleston Sequoia 2016-01-16 22:10:06 PST
I'm giving up on trying to beat cmake into submission.  Our workaround for now is just to edit link.txt after running cmake.  ie:

sed -i '' "s|../../lib/libWTFGTK.a||" Source/WebKit2/CMakeFiles/WebKit2.dir/link.txt
Comment 8 Jeremy Huddleston Sequoia 2016-01-16 22:12:06 PST
Although it looks like I need to do the same for WebKitWebProcess and other places where it's linked in statically.
Comment 9 Jeremy Huddleston Sequoia 2016-01-16 22:19:46 PST
It looks like this goes well beyond just WTF.  Most of the executables are linking against both the dylibs and static archives.  For example MiniBrowser is linking

libwebkit2gtk-4.0.37.13.0.dylib
libjavascriptcoregtk-4.0.18.3.2.dylib
libbmalloc.a 
libWebCoreGTK.a
libANGLESupport.a
libGObjectDOMBindings.a
libWebCorePlatformGTK.a
libWTFGTK.a

Why does cmake not realize that the functionality provided by those static archives is already rolled into the dylibs that it built?  Depending on JavaScriptCore should just result in linking that dylib, not linking that dylib *AND* all the static archives that went into that dylib.

Have I mentioned how much I dislike cmake?
Comment 10 Michael Catanzaro 2016-01-17 09:04:34 PST
(In reply to comment #9)
> It looks like this goes well beyond just WTF.  Most of the executables are
> linking against both the dylibs and static archives.  For example
> MiniBrowser is linking
> 
> libwebkit2gtk-4.0.37.13.0.dylib
> libjavascriptcoregtk-4.0.18.3.2.dylib
> libbmalloc.a 
> libWebCoreGTK.a
> libANGLESupport.a
> libGObjectDOMBindings.a
> libWebCorePlatformGTK.a
> libWTFGTK.a
> 
> Why does cmake not realize that the functionality provided by those static
> archives is already rolled into the dylibs that it built?  Depending on
> JavaScriptCore should just result in linking that dylib, not linking that
> dylib *AND* all the static archives that went into that dylib.
> 
> Have I mentioned how much I dislike cmake?

Clearly we need to figure out what's up here. The static libraries are "convenience libs" which are never installed: they're only there to make reasoning about the build system easier, and so that we can catch layering violations between the libraries. MiniBrowser should only be linking to libwebkit2gtk-4.0.37.13.0.dylib and libjavascriptcoregtk-4.0.18.3.2.dylib.

I wonder if the Mac port is having any similar linking problems?
Comment 11 Michael Catanzaro 2016-01-17 09:37:40 PST
Taking a quick look over https://cmake.org/cmake/help/v3.4/command/target_link_libraries.html

I think this might be what the PUBLIC, PRIVATE, and INTERFACE specifiers are for. What if, in Source/JavaScriptCore/CMakeLists.txt, instead of this:

target_link_libraries(JavaScriptCore ${JavaScriptCore_LIBRARIES})

You were to replace it with:

target_link_libraries(JavaScriptCore PRIVATE ${JavaScriptCore_LIBRARIES})

Perhaps that would fix this?

Similarly (for Source/WebKit2/CMakeLists.txt):

set(WebKit2_LIBRARIES
    PRIVATE WebCore
    INTERFACE JavaScriptCore
)
Comment 12 Michael Catanzaro 2016-01-17 09:50:54 PST
(In reply to comment #11)
> set(WebKit2_LIBRARIES
>     PRIVATE WebCore
>     INTERFACE JavaScriptCore
> )

Hm, putting the visibility specifiers inside the lists is probably not going to work; you might need to split it up into WebKit2_PRIVATE_LIBRARIES, WebKit2_PUBLIC_LIBRARIES, and WebKit2_INTERFACE_LIBRARIES.
Comment 13 Michael Catanzaro 2016-01-17 13:38:22 PST
*** Bug 153093 has been marked as a duplicate of this bug. ***
Comment 14 Philip Chimento 2016-03-13 21:17:37 PDT
Created attachment 273923 [details]
Doesn't fix the problem

I've been trying to correct this, but have been trying various combinations of PUBLIC, PRIVATE, and INTERFACE all weekend (and waiting most times for WebCore to rebuild, as cmake doesn't seem to be able to determine that all I've changed is the linking libraries). Attached is a work-in-progress patch (not formatted for committing) of what I have now, which doesn't fix the crash.

I think the problem is that I don't understand how the various convenience libraries should be linked together. From the other comments here, it seems it should be something like this:

WTF -> convenience library, linked into JavaScriptCore
    -> symbols used from JavaScriptCore, WebCore, and WebKit2

JavaScriptCore -> public, uses and exports WTF symbols

WebCore -> convenience library, linked into WebKit2
        -> uses WTF and JavaScriptCore symbols
        -> symbols used from WebKit2

WebKit2 -> public, uses WTF, JavaScriptCore, WebCore symbols

I _think_ that's what I've implemented in the patch, but I still end up with JavaScriptCore and WebKit2 each containing their own copy of WTF.

Advice would be appreciated.
Comment 15 Michael Catanzaro 2016-03-14 08:05:04 PDT
(In reply to comment #14)
> WTF -> convenience library, linked into JavaScriptCore
>     -> symbols used from JavaScriptCore, WebCore, and WebKit2
> 
> JavaScriptCore -> public, uses and exports WTF symbols
> 
> WebCore -> convenience library, linked into WebKit2
>         -> uses WTF and JavaScriptCore symbols
>         -> symbols used from WebKit2
> 
> WebKit2 -> public, uses WTF, JavaScriptCore, WebCore symbols

Yes, that's all correct.