Summary: | WebWorker threads can outlive the QApplication instance | ||
---|---|---|---|
Product: | WebKit | Reporter: | Andrew Wason <rectalogic> |
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | ap, dimich, hausmann, jchaffraix, levin |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Attachments: |
Description
Andrew Wason
2011-11-16 13:11:40 PST
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. |