Bug 72155 - Replace Qt QThread threading back-end with pthread/Win32 threading back-ends
Summary: Replace Qt QThread threading back-end with pthread/Win32 threading back-ends
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-11 10:57 PST by Andrew Wason
Modified: 2011-11-30 05:01 PST (History)
4 users (show)

See Also:


Attachments
use native threading back-end (16.90 KB, patch)
2011-11-11 12:05 PST, Andrew Wason
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
gdb backtrace showing WebWorker threads active after QCoreApplication destruction (32.11 KB, text/plain)
2011-11-14 09:06 PST, Andrew Wason
no flags Details
extract initialization into common function (26.52 KB, patch)
2011-11-18 14:31 PST, Andrew Wason
no flags Details | Formatted Diff | Diff
extract initialization into common function (26.52 KB, patch)
2011-11-18 14:40 PST, Andrew Wason
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff
initialize webkit_main_thread_invoker() on main thread (27.11 KB, patch)
2011-11-25 14:37 PST, Andrew Wason
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
initialize WebCore from public static QWebSettings methods (32.69 KB, patch)
2011-11-28 08:34 PST, Andrew Wason
no flags Details | Formatted Diff | Diff
update patch for build-system changes in trunk (32.61 KB, patch)
2011-11-28 09:47 PST, Andrew Wason
hausmann: review+
rectalogic: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wason 2011-11-11 10:57:31 PST
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.
Comment 1 Andrew Wason 2011-11-11 12:05:06 PST
Created attachment 114751 [details]
use native threading back-end

Tested on Linux and MacOS, but not Win32.
Comment 2 Simon Hausmann 2011-11-14 07:57:21 PST
(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 :)
Comment 3 Andrew Wason 2011-11-14 09:06:40 PST
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.
Comment 4 Simon Hausmann 2011-11-16 03:00:55 PST
(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? :)
Comment 5 Andrew Wason 2011-11-16 13:14:07 PST
(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.
Comment 6 Simon Hausmann 2011-11-18 00:19:22 PST
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 :)
Comment 7 Simon Hausmann 2011-11-18 00:57:25 PST
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.
Comment 8 Andrew Wason 2011-11-18 08:50:52 PST
(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()
Comment 9 Andrew Wason 2011-11-18 09:22:38 PST
(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?
Comment 10 Andrew Wason 2011-11-18 14:31:59 PST
Created attachment 115874 [details]
extract initialization into common function
Comment 11 WebKit Review Bot 2011-11-18 14:35:05 PST
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.
Comment 12 Andrew Wason 2011-11-18 14:40:40 PST
Created attachment 115875 [details]
extract initialization into common function

Fix style issue.
Comment 13 Simon Hausmann 2011-11-21 00:08:32 PST
(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()
Comment 14 Simon Hausmann 2011-11-21 00:13:15 PST
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?
Comment 15 Andrew Wason 2011-11-21 06:51:55 PST
(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.
Comment 16 Andrew Wason 2011-11-23 16:29:54 PST
(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.
Comment 17 Andrew Wason 2011-11-25 14:37:07 PST
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.
Comment 18 Simon Hausmann 2011-11-25 23:35:35 PST
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.
Comment 19 Andrew Wason 2011-11-28 08:34:34 PST
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.
Comment 20 Andrew Wason 2011-11-28 09:47:00 PST
Created attachment 116771 [details]
update patch for build-system changes in trunk
Comment 21 Simon Hausmann 2011-11-30 01:44:52 PST
Comment on attachment 116771 [details]
update patch for build-system changes in trunk

r=me. Will land by hand.
Comment 22 Simon Hausmann 2011-11-30 03:01:47 PST
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.