Bug 167828 – Better handle Thread and RunLoop initialization

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Better handle Thread and RunLoop initialization.

The order and where these operations happen is important and is mixed all around WebKit:

    WTF::initializeThreading();
    WTF::initializeMainThread();
    JSC::initializeThreading();
    WTF::RunLoop::initializeMainRunLoop();
    WTF::initializeWebThread();

Note also that there are some requirements for some but not others on different ports.

wtf/Threading.h:

    // This function must be called from the main thread. It is safe to call it repeatedly.
    // Darwin is an exception to this rule: it is OK to call it from any thread, the only 
    // requirement is that the calls are not reentrant.
    WTF_EXPORT_PRIVATE void initializeThreading();

JavaScriptCore/InitializeThreading.h:

    // This function must be called from the main thread. It is safe to call it repeatedly.
    // Darwin is an exception to this rule: it is OK to call this function from any thread, even reentrantly.
    JS_EXPORT_PRIVATE void initializeThreading();

wtf/MainThread.h:

    // Must be called from the main thread.
    WTF_EXPORT_PRIVATE void initializeMainThread();

wtf/RunLoop.h: (NOTE: This is currently a lie on Mac)

    // Must be called from the main thread (except for the Mac platform, where it
    // can be called from any thread).
    WTF_EXPORT_PRIVATE static void initializeMainRunLoop();

See some good investigation from Carlos:
https://bugs.webkit.org/show_bug.cgi?id=167776#c2

[Joseph Pecoraro]

I think my original change putting initialize main run loop in the JSC path is incorrect. That is rather unfortunate though. It would be good to have some dependable run loop in an application. However it doesn't seem like that is currently possible.

[Joseph Pecoraro]

I'll start by rolling out the changes r211486 / r211629. I now think the original change was not ideal.

[Carlos Garcia Campos]

I've been looking at this again, more info:

JSC::initializeThreading()
  WTF::double_conversion::initialize()
  WTF::initializeThreading()
    WTF::double_conversion::initialize()
    StringImpl::empty()
    threadMapMutex()
    initializeRandomNumberGenerator()
    ThreadIdentifierData::initializeOnce()
    wtfThreadData()
    initializeDates()
  WTF::initializeGCThreads()

WTF::initializeMainThread()
  initializeMainThreadPlatform()
    Mac: initializeGCThreads()
  initializeGCThreads()

WTF::initializeMainThreadToProcessMainThread()
  initializeGCThreads()

WTF::initializeWebThread()

RunLoop::initializeMainRunLoop()

WTF::initializeThreading() is required by everything, so anything done there doesn't need to be done in all other initializations, like double_conversion::initialize(), for example.

WTF::initializeGCThreads() only makes sense for processes that care about main thread, it's only used in asserts to ensure the current thread is the main one or a GC thread. So, it should probably be called only by WTF::initializeMainThread(), and doesn't need to be called again in initializeMainThreadPlatform().

initializeMainThread() requires WTF threading to be initialized when called, it could always call WTF::initializeThreading to remove that dependency and also make it work in cases where it's called before JSC::initializeThreading.

RunLoop::initializeMainRunLoop() requires initializeMainThread() to be called before in some ports, so it should explicitly call it instead of assuming it will happen. The only expcetion is mac legacy WebKit that requires that WTF::initializeMainThreadToProcessMainThread() is called before, which already ensured.

[Carlos Garcia Campos]

Created attachment 302381 [details]
Patch

[Michael Catanzaro]

40694 JSC-only test failures.

[Yusuke Suzuki]

Comment on attachment 302381 [details]
Patch

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

> Source/JavaScriptCore/jsc.cpp:3766
>      WTF::initializeMainThread();

This should be `JSC::initializeMainThread()` I think.

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 302381 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302381&action=review
> 
> > Source/JavaScriptCore/jsc.cpp:3766
> >      WTF::initializeMainThread();
> 
> This should be `JSC::initializeMainThread()` I think.

Oh, I removed the wrong one.

[Carlos Garcia Campos]

Created attachment 302404 [details]
Patch

jsc.cpp change was indeed wrong

[Yusuke Suzuki]

Comment on attachment 302404 [details]
Patch

r=me

[Build Bot]

Comment on attachment 302404 [details]
Patch

Attachment 302404 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3173566

New failing tests:
editing/spelling/spellcheck-async.html

[Build Bot]

Created attachment 302415 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Committed r212878: <http://trac.webkit.org/changeset/212878>

[Joseph Pecoraro]

Nice!