Bug 38097 – Disentangle initializing the main thread from initializing threading

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sam Weinig]

We should disentangle the initialization of WebCores notion of the main thread from the rest of the initialization done in JSC::initializeThreading and WTF::initializeThreading.

[Sam Weinig]

Created attachment 54246 [details]
Patch

[Sam Weinig]

Comment on attachment 54246 [details]
Patch

Forgot something.  Obsoleting.

[Sam Weinig]

Created attachment 54247 [details]
Patch

[WebKit Review Bot]

Attachment 54247 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/win/WebKitClassFactory.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebKit/efl/ewk/ewk_main.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 54247 [details]
Patch

Some minor comments below. Overall this seems reasonable other than the "deprecated" naming. ALso I think it would be good to log or assert or throw an exception or something in the cases where deprecatedInitializeMainThread is called from a thread other than the main thread (since it implies misuse of the original WebKit API). r- to consider these issues, please upload a fresh version.

JavaScriptCore/ChangeLog:15
 +          (WTF::deprecatedInitializeMainThread):
The difference between initializeMainThread and deprecatedInitializeMainThread is not clear from the names. Also from offline discussion it seems like the "deprecated" version is not going away soon or possibly ever, so that doesn't seem like a very good name.

JavaScriptCore/wtf/MainThread.cpp:101
 +      MutexLocker locker(mainThreadInitializationMutex());
Why a mutex here instead of a "once" type mechanism like pthread_once?

JavaScriptCore/wtf/mac/MainThreadMac.mm:75
 +  void deprecatedInitializeMainThreadPlatform()
it looks to me like mainThreadPthread will never be initialized if WebKit is called off the main thread the first time, even on platforms where it would otherwise be initialized to pthread_self(). Is that ok?

[Sam Weinig]

(In reply to comment #5)
> (From update of attachment 54247 [details])
> Some minor comments below. Overall this seems reasonable other than the
> "deprecated" naming. ALso I think it would be good to log or assert or throw an
> exception or something in the cases where deprecatedInitializeMainThread is
> called from a thread other than the main thread (since it implies misuse of the
> original WebKit API). r- to consider these issues, please upload a fresh
> version.
> 
> JavaScriptCore/ChangeLog:15
>  +          (WTF::deprecatedInitializeMainThread):
> The difference between initializeMainThread and deprecatedInitializeMainThread
> is not clear from the names. Also from offline discussion it seems like the
> "deprecated" version is not going away soon or possibly ever, so that doesn't
> seem like a very good name.

Which name seems better, initializeMainThreadToSystemMainThread() or legacyInitializeMainThread()?

> 
> JavaScriptCore/wtf/MainThread.cpp:101
>  +      MutexLocker locker(mainThreadInitializationMutex());
> Why a mutex here instead of a "once" type mechanism like pthread_once?

Will fix.

> JavaScriptCore/wtf/mac/MainThreadMac.mm:75
>  +  void deprecatedInitializeMainThreadPlatform()
> it looks to me like mainThreadPthread will never be initialized if WebKit is
> called off the main thread the first time, even on platforms where it would
> otherwise be initialized to pthread_self(). Is that ok?

That is fine.  If deprecatedInitializeMainThreadPlatform() is ever called, it is being used from an environment which expects WebCore's main thread to be the system's main thread.

[Sam Weinig]

I didn't note this anywhere, and I feel silly for not having done so, but this is the first step in getting rid of the WEB_THREAD #define and paving the way to allowing mac both WebKit and WebKit2 to use the same WebCore.  I will put this information in the next versions changelog.

[Sam Weinig]

Created attachment 54338 [details]
Patch 3

[WebKit Review Bot]

Attachment 54338 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/win/WebKitClassFactory.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebKit/efl/ewk/ewk_main.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Maciej Stachowiak]

Comment on attachment 54338 [details]
Patch 3

r=me

[Sam Weinig]

Landed in r58266.

[Sam Weinig]

Fix tiger build in r58268.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58266 might have broken Chromium Win Release