Replace Qt QThread threading back-end with pthread/Win32 threading back-ends

[Andrew Wason]

WebWorker QThreads can outlive the QApplication instance, and even various internal Qt global static state. This leads to random SEGVs and hangs on application exit. Moving away from ThreadingQt QThread back-end to use ThreadingPthreads or ThreadingWin helps to avoid this and reduce maintenance due to more code shared with the other platforms.

See bug 71718 for a valgrind/helgrind log showing the problem, and a discussion of using POSIX/Win32 threads instead of QThreads.

[Andrew Wason]

Created attachment 114751 [details]
use native threading back-end

Tested on Linux and MacOS, but not Win32.

[Simon Hausmann]

(In reply to comment #0)
> WebWorker QThreads can outlive the QApplication instance, and even various internal Qt global static state. This leads to random SEGVs and hangs on application exit. Moving away from ThreadingQt QThread back-end to use ThreadingPthreads or ThreadingWin helps to avoid this and reduce maintenance due to more code shared with the other platforms.
> 
> See bug 71718 for a valgrind/helgrind log showing the problem, and a discussion of using POSIX/Win32 threads instead of QThreads.

I like the idea of more code sharing and while introducing the pthreads dependency on windows is painful, I clearly see benefits. But I'm curious about the web workers:

Could you elaborate a bit how it is possible that web workers can outlive the QApplication instance? It doesn't sound like something "intentional" to me, but I might easily be missing something :)

[Andrew Wason]

Created attachment 114959 [details]
gdb backtrace showing WebWorker threads active after QCoreApplication destruction

(In reply to comment #2)
> 
> I like the idea of more code sharing and while introducing the pthreads dependency on windows is
> painful, I clearly see benefits.

I don't think this introduces a pthreads dependency on windows, ThreadingWin.cpp uses native Win32 threading APIs.

> Could you elaborate a bit how it is possible that web workers can outlive the QApplication instance?
> It doesn't sound like something "intentional" to me, but I might easily be missing something :)

There is nothing that waits for the worker threads to finish - so there is a race condition. The WorkerThread event loops are quit, but they can still be busy tearing down state after QApplication has been destroyed.

I'm attaching a gdb backtrace where a breakpoint was set at the end of  QCoreApplication::~QCoreApplication - after all application state has been deleted, and you can see a number of worker threads are still active (actively shutting down, but still active). This is using the sample app attached to bug 71718. Also various JS garbage collector threads can remain active.

Threads can also remain active after global static Qt internal state has been destroyed (e.g. after various Qt QGlobalStaticDeleters have destroyed global state). e.g. Q_GLOBAL_STATIC(QInternal_CallBackTable, global_callback_table) can be destroyed, and some QThread then attempts to disconnect() a signal handler and crashes because disconnect() needs global_callback_table.

[Simon Hausmann]

(In reply to comment #3)
> Created an attachment (id=114959) [details]
> gdb backtrace showing WebWorker threads active after QCoreApplication destruction
> 
> (In reply to comment #2)
> > 
> > I like the idea of more code sharing and while introducing the pthreads dependency on windows is
> > painful, I clearly see benefits.
> 
> I don't think this introduces a pthreads dependency on windows, ThreadingWin.cpp uses native Win32 threading APIs.
> 
> > Could you elaborate a bit how it is possible that web workers can outlive the QApplication instance?
> > It doesn't sound like something "intentional" to me, but I might easily be missing something :)
> 
> There is nothing that waits for the worker threads to finish - so there is a race condition. The WorkerThread event loops are quit, but they can still be busy tearing down state after QApplication has been destroyed.

Hm, but shouldn't there be something that waits for them?

I mean... is it really true that for example when running a worker script in say Chrome it can actually outlive the life time of the "tab" or even window that started it? I find that difficult to believe, especially given that without being able to post a message back, why should it be left running? :)

[Andrew Wason]

(In reply to comment #4)
>
> Hm, but shouldn't there be something that waits for them?

Yeah, it seems like there should. I created bug 72538 for that.

Meanwhile, I think there is still value in moving away from the Qt specific threading back end, so would like to proceed with getting this patch reviewed.

[Simon Hausmann]

Comment on attachment 114751 [details]
use native threading back-end

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

Patch looks good to me! Except for the initialization bit (see comment below). So r- just to clarify that, but otherwise nice cleanup! :)

> Source/WebKit/qt/Api/qwebsettings.cpp:311
> +    if (!global) {
> +        WebCore::ScriptController::initializeThreading();
>          global = new QWebSettings;
> +    }

Actually we call this already in QWebPagePagePrivate. Are you sure you need it here?

If yes, then we should move all those initializations into a central function that we can call from QWebPagePrivate as well as from here. I don't like the idea of
sprinkling Foo::initializeBar() over the code :)

[Simon Hausmann]

I see one problem (paroga just pointed out on IRC): By removing the use of QThread we might be breaking callOnMainThread, which needs scheduleDispatchFunctionsOnMainThread. in qt/MainThreadQt.cpp we post a message to the main gui thread, which requires that the current thread is a QThread.

[Andrew Wason]

(In reply to comment #7)
> in qt/MainThreadQt.cpp we post a message to the main gui thread, which requires
> that the current thread is a QThread.

I don't think that's the case - the native thread automatically becomes a QAdoptedThread if needed and posting messages to the main thread from it seems to work fine.

Setting a breakpoint in WTF::MainThreadInvoker::dispatch() and WTF::scheduleDispatchFunctionsOnMainThread() dispatch() is called on the main thread for each corresponding call to WTF::scheduleDispatchFunctionsOnMainThread()

[Andrew Wason]

(In reply to comment #6)
> > Source/WebKit/qt/Api/qwebsettings.cpp:311
> > +    if (!global) {
> > +        WebCore::ScriptController::initializeThreading();
> >          global = new QWebSettings;
> > +    }
> 
> Actually we call this already in QWebPagePagePrivate. Are you sure you need it here?

Yes, global QWebSettings can be used before any QWebPage is created - and some settings (e.g. page cache settings) rely on threading being initialized.  QtTestBrowser does this and asserts without the above.

> If yes, then we should move all those initializations into a central function that we can call
> from QWebPagePrivate as well as from here. I don't like the idea of sprinkling Foo::initializeBar()
> over the code :)

Do you mean extract all the global webkit initialization code (e.g. the first 10 lines of QWebPagePrivate::QWebPagePrivate) into some initializeWebKit() function that is called from QWebPagePrivate and QWebSettings::globalSettings?

[Andrew Wason]

Created attachment 115874 [details]
extract initialization into common function

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1

Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 13 files


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

[Andrew Wason]

Created attachment 115875 [details]
extract initialization into common function

Fix style issue.

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > in qt/MainThreadQt.cpp we post a message to the main gui thread, which requires
> > that the current thread is a QThread.
> 
> I don't think that's the case - the native thread automatically becomes a QAdoptedThread if needed and posting messages to the main thread from it seems to 
work fine.

You're right. I saw the adopted thread stuff, but my grep missed the place where we create the adopted threads :)
 
> Setting a breakpoint in WTF::MainThreadInvoker::dispatch() and WTF::scheduleDispatchFunctionsOnMainThread() dispatch() is called on the main thread for each corresponding call to WTF::scheduleDispatchFunctionsOnMainThread()

[Simon Hausmann]

Comment on attachment 115875 [details]
extract initialization into common function

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

r=me, but I think this should be landed manually. It's quite a core change where we should watch the Qt bots closely.

I won't get around to doing it before the end of the week, so please feel free to beat me to it :)

> Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp:2
> + * Copyright 2011 Hewlett-Packard Development Company, L.P. All rights reserved.

Out of curiosity: You're using a rectalogic.com email address. How come this file is copyright HP?

[Andrew Wason]

(In reply to comment #14)
> Out of curiosity: You're using a rectalogic.com email address. How come this file is copyright HP?

I work for HP, but the corporate Exchange email sucks.

[Andrew Wason]

(In reply to comment #14)
> I won't get around to doing it before the end of the week, so please feel free to beat me to it :)

Now I'm wondering if we should hold off on this patch.
Since the patch to wait for worker threads in bug 72538 is not going to be accepted, it may be more difficult to fix that if this patch is applied.

The problem is if we aren't using a QThreads back-end, then all the worker threads end up becoming QAdoptedThreads anyway (due to scheduleDispatchFunctionsOnMainThread), and we have no control over the QAdoptedThread objects lifetime (it is destroyed by Qt as part of thread shutdown). With the QThreads back-end, we control the QThread object lifetime and so could wait for any remaining QThreads when QApplication is destroyed.

[Andrew Wason]

Created attachment 116639 [details]
initialize webkit_main_thread_invoker() on main thread

Updated the patch to initialize webkit_main_thread_invoker() on the main thread. The recent changes to dispatchFunctionsFromMainThread() from bug 72704 prevent the native threads from being infected by QAdoptedThread - but we need to only create webkit_main_thread_invoker() on the main thread or else the thread it is first created on becomes a QAdoptedThread leading to problems discussed in bug 72538.

So I believe this patch now also fixes bug 72538 - i.e. Qt no longer needs to wait for worker threads to exit when shutting down since the workers are plain native threads.

However, my test application still crashes occasionally - but not necessarily during shutdown. I was able to reproduce the crash in a test app that never shuts down (just periodically reloads a QWebPage with workers) - so I think that is a different bug.

So I think we should proceed with applying this patch, assuming it passes review, and then I'll close bug 72538 and open a new bug for this new worker crash issue.

[Simon Hausmann]

Comment on attachment 116639 [details]
initialize webkit_main_thread_invoker() on main thread

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.

[Andrew Wason]

Created attachment 116764 [details]
initialize WebCore from public static QWebSettings methods

(In reply to comment #18)
>     2) Ensure initializeMainThreadPlatform() is called before any thread other than the gui thread is started.

I did this. Any public static QWebSettings method could be called before a QWebPage has been constructed, and if those methods call any WebCore APIs they should ensure it has been initialized.

[Andrew Wason]

Created attachment 116771 [details]
update patch for build-system changes in trunk

[Simon Hausmann]

Comment on attachment 116771 [details]
update patch for build-system changes in trunk

r=me. Will land by hand.

[Simon Hausmann]

Comment on attachment 116771 [details]
update patch for build-system changes in trunk

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

Due to the removal an implicit qstring.h disappeared and breaks the WK2 build. I'll fix that when landing.

> Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp:33
> +#include "Logging.h"

This include is causing a problem with the WebKit2 build due to the include path order and the double existance of Logging.h in the tree. I'll work around it by an intermediate inclusion when landing.

[Simon Hausmann]

Additional fixes required: http://trac.webkit.org/changeset/101482 http://trac.webkit.org/changeset/101483 http://trac.webkit.org/changeset/101484 http://trac.webkit.org/changeset/101491

Committed r101477: <http://trac.webkit.org/changeset/101477>