[webkit-reviews] review denied: [Bug 72155] Replace Qt QThread threading back-end with pthread/Win32 threading back-ends : [Attachment 116639] initialize webkit_main_thread_invoker() on main thread

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 25 23:35:34 PST 2011


Simon Hausmann <hausmann at webkit.org> has denied Andrew Wason
<rectalogic at rectalogic.com>'s request for review:
Bug 72155: Replace Qt QThread threading back-end with pthread/Win32 threading
back-ends
https://bugs.webkit.org/show_bug.cgi?id=72155

Attachment 116639: initialize webkit_main_thread_invoker() on main thread
https://bugs.webkit.org/attachment.cgi?id=116639&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116639&action=review


r- because I believe the removal of the moveToThread() code will cause layout
test failures (like it did on Thursday).

> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:73
> +    QCoreApplication::postEvent(webkit_main_thread_invoker(), new
QEvent(static_cast<QEvent::Type>(s_mainThreadInvokerEventType)));

There's a problem with the removal of the moveToThread code. There is currently
no guarantee that initializeMainThreadPlatform is called before
scheduleDispatchFunctionsOnMainThread() is called the first time from a thread.
As a result it may happen that the webkit_main_thread_invoker object
will live on one of the threads other than the gui thread and as a result the
functionality of scheduling the dispatch of functions on the main thread
won't work. We had exactly this problem on Thursday morning and if you run the
layout tests on Linux with your patch applied, you'll see that all the appcache

tests will break and time out.

There are two solutions AFAICS:

    1) Keep the moveToThread code that I added. That's least intrusive and
probably most robust, but it's so "clean".

    2) Ensure initializeMainThreadPlatform() is called before any thread other
than the gui thread is started.

The modification you're going to need for (2) is around the static functions in
QWebSettings, in particular the icon database related ones. If you
call QWebSettings::setIconDatabasePath() before any other function in QtWebKit
(like QWebSettings::globalSettings() or creating a QWebPage), then you
have the situation that the WebCore IconDatabase will spawn its thread before
initializeMainThreadPlatform() is called, and that thread will call
scheduleDispatchFunctionsOnMainThread(), which in turn will cause the creation
of the invoker object in the wrong thread with the wrong thread affinity.


More information about the webkit-reviews mailing list