Created attachment 115429 [details] sample code Currently when a QWebPage that has WebWorkers is destroyed, or when close() is called from the worker JS, the WorkerThread is quit (WebCore::Document::detach calls WebCore::ScriptExecutionContext::stopActiveDOMObjects which calls WebCore::Worker::stop) but not waited for. So there is a race condition, if the QApplication event loop quits and QApplication is destroyed, the WorkerThreads may still be running and may access internal Qt state and crash. See bug 72155 for some discussion of this. The attached sample code demonstrates this. It loads a page that spawns WebWorkers, then waits and quits QApplication. Setting a breakpoint in QCoreApplication::~QCoreApplication you can see a number of WorkerThreads are still active and have not finished shutting down. Somewhere we should wait for the WorkerThreads to finish.
Created attachment 115431 [details] gdb backtrace showing running WorkerThreads during QApplication destruction
Created attachment 115691 [details] wait for WorkerThread to terminate in stop()
How do the other ports handle this?
(In reply to comment #3) > How do the other ports handle this? It looks to me like they don't, they have the same problem. This patch is not port-specific so should fix it for all of them. I just checked with the gtk port, spawned a bunch of workers and put a breakpoint in _exit and when I quit GtkLauncher many WorkerThreads were still active. But I think not waiting for WorkerThreads is a bigger problem for the Qt port since the threads seem to be using some Qt objects after QApplication is destroyed leading to periodic crashes.
Making thread stopping slow and synchronous does not seem so great. IIRC the design is that even though the thread continues to run for a short while, it should not cause any problems as communication is cut off. Dave, Dmitry, could you please comment? As a separate question, why does QApplication ever need to be destroyed?
(In reply to comment #5) > > As a separate question, why does QApplication ever need to be destroyed? In Qt applications it's typically destroyed when main() returns. I guess it could be allocated on the heap and leaked, but even then there is global static internal Qt state that is destroyed as part of application shutdown and this can cause a crash when lingering WorkerThreads threads indirectly access it.
I agree with Alexey here. There is all kinds of confirmations going between ScriptExecutionContext of the page and WorkerContext about shutdowns and terminations (not of the actual OS thread but of a message queue-based 'worker thread'). It should be enough to deallocate any resources allocated by port's implementation. "workerContextDestroyed" callback is the one I would probably look at. We should not assume there is always going to be 1:1 correspondence between workers and OS threads. At least this is going to be port-specific. For example, Chromium can run workers in separate process and might need to manage threads/resources/shutdown sequence in a specific way there.
(In reply to comment #7) > It should be enough to deallocate any resources allocated by port's implementation. The platform specific resource is the QThread itself, and the only time it is destroyed is when the thread exits. Bug 72155 helps somewhat with this by dropping QThread in favor of native threads, but native threads become QAdoptedThreads when WTF::scheduleDispatchFunctionsOnMainThread is called from them to postMessage from the WebWorker - so we have a similar problem. Perhaps Qt's WTF::scheduleDispatchFunctionsOnMainThread could be changed to connect QCoreApplication::aboutToQuit to a slot that does QThread::wait, and connect QThread::finished to a slot that disconnects that. So this way when QApplication is finished, it will wait for all outstanding worker QThreads to finish. I'll look into this. Currently my workaround for this bug in my application is to install a signal handler for SIGSEGV when QApplication::exec returns, and in the handler just _exit(0). This is nasty.
Created attachment 116462 [details] wait for WorkerThread only for Qt platform I've tried a number of approaches but can't see how to do this safely with Qt. Part of the problem is knowing when the application is shutting down in order to safely wait for the workers - connecting to QCoreApplication::aboutToQuit is too early because any QWebPages may not be destroyed yet, so they won't have told their workers to quit. Connecting to QCoreApplication::destroyed is too late because some workers may still be starting up and crash because QCoreApplication::instance() is now NULL. Attached patch makes waiting for WorkerThreads in stop() conditional on PLATFORM(QT)
Comment on attachment 116462 [details] wait for WorkerThread only for Qt platform This puts a burden on whoever need to modify this code next. They would have to understand both regular code path and what QT does, which is not so easy.
(In reply to comment #7) > We should not assume there is always going to be 1:1 correspondence between workers and OS threads. At least this is going to be port-specific. For example, Chromium can run workers in separate process and might need to manage threads/resources/shutdown sequence in a specific way there. That's a good point, but even if workers are implemented as processes, don't you want those to terminate in a "controlled" way? How does chromium enforce the termination of shared workers if for example I close the last window?
Nevermind my previous comment, I see now how this is kind of a Qt specific race condition. Hmmmm.
This bug was made irrelevant by bug 72155 - now that we don't use Qt objects in worker threads, it's safe to let them outlive QApplication. For future reference, if we ever do need to wait for threads when QApplication is being destroyed - it looks like a Qt qAddPostRoutine() is the best place to do it.