static destructors calling Timer.stop in wrong thread
https://bugs.webkit.org/show_bug.cgi?id=63928
Summary static destructors calling Timer.stop in wrong thread
Olaf Flebbe
Reported 2011-07-05 03:09:08 PDT
Created attachment 99688 [details] Patch to allocate Timer on Heap, rather statically I have filed a problem with the latest eclipse indigo release for Fedora 15 at https://bugs.eclipse.org/bugs/show_bug.cgi?id=348746 I have the same problem on opensuse 11.4. It is a bug exiting an SWT application using webkit. The heart of the Problem is located in the way WebKit Timers Source/WebCore/platform/Timers.cpp are intermixed with Threads. It is not possible to stop() a Timer in a different thread than the one it was created in. Timers are thread specific by design. Thus Timers own an internal data structure referenced through TLS (Thread local storage). (The ThreadSpecific<ThreadGlobalData> class), organizing all the Timeres of the current Thread. There is a static allocated Timer used by the Cairo backend: PurgeScratchBufferTimer in Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp This Timer is allocated when libwebkit.so is loaded. I have filed a problem with the latest eclipse indigo release for Fedora 15 at https://bugs.eclipse.org/bugs/show_bug.cgi?id=348746 I have the same problem on opensuse 11.4. I am using the webkit Source RPM to reproduce the problem but found the offending code is the essentially the same on webkit-nightly. I was not able to build webkit nightly because of the libsoup issues mentioned here before. To reproduce the problem in Short: Use FC15 oder opensuse 11.4 with all defaults (preinstalled libwebkitgtk, openjdk, etc). Download current eclipse, show HTML help, quit eclipse and crash most of the time --or -- Use SWT Browser Demo (jar in the eclipse bugtracker) and finish it (sometimes crashing). After some investigation it seems IMHO to be a gtk webkit problem: Here is some kind of workaround for libwebkitgtk. But the real problem is triggered by how java uses webkit-gtk in SWT. In WebKit Timers Source/WebCore/platform/Timers.cpp are intermixed with Threads. It is not possible to stop() a Timer in a different thread than the one it was created in. Timers are thread specific by design. Thus Timers own an internal data structure referenced through TLS (Thread local storage). (The ThreadSpecific<ThreadGlobalData> class), organizing all the Timeres of the current Thread. There is a static allocated Timer used by the Cairo backend: PurgeScratchBufferTimer in Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp This Timer is allocated when java is loading the JNI/SWT/gtk libraries, by the current thread, which happens _not_ to be the _main_ thread. This PurgeScratchBufferTimer is constructed at the time libwebkitgtk is loaded into SWT and may be used later on. What happens if an SWT examples exits()? The libc Runtime runs all the static destructors, including those objects located in the shared libraries. And it runs these destructors within the thread calling exit()! But this exit()ed thread is not be the thread which constructed the C++ Objects. At least for Java-SWT. The patch supplied allocates the timers on heap, not statically before. Thus the destructors are not running on exit(), rather they are running when the threads ends its life. The second patch is to demonstrate the problem. Compile webkit with --enable-debug, run Programs/GtkLauncher and wait 10 seconds for webkit to crash. I double checked the patch: It solves my SWT problem.
Attachments
Patch to allocate Timer on Heap, rather statically (2.06 KB, patch)
2011-07-05 03:09 PDT, Olaf Flebbe
no flags
Patch to GtkLauncher, just to demonstrate the Problem compile with --enable-debug ! (761 bytes, patch)
2011-07-05 03:10 PDT, Olaf Flebbe
no flags
Allocate Timers non-statically (8.18 KB, patch)
2011-07-06 00:51 PDT, Olaf Flebbe
no flags
Allocate Timers non-statically (next try) (8.20 KB, patch)
2011-07-06 00:58 PDT, Olaf Flebbe
eric: review+
eric: commit-queue-
Olaf Flebbe
Comment 1 2011-07-05 03:10:02 PDT
Created attachment 99689 [details] Patch to GtkLauncher, just to demonstrate the Problem compile with --enable-debug !
Olaf Flebbe
Comment 2 2011-07-05 03:16:20 PDT
Sorry Cut'n'paste happend here. Since bugzilla tickets can't be editet: Please omit text between the second "I have filed a problem" until "to be used later on".
Alexey Proskuryakov
Comment 3 2011-07-05 10:39:43 PDT
Nice catch! We normally use a different idiom for these: static PurgeScratchBufferTimer& purgeScratchBufferTimer() { DEFINE_STATIC_LOCAL(PurgeScratchBufferTimer, staticTimer, ()); return staticTimer; } To get the fix reviewed and accepted, you'll need to add a ChangeLog, and to mark the patch for review: <http://www.webkit.org/coding/contributing.html>.
Olaf Flebbe
Comment 4 2011-07-06 00:48:24 PDT
Hi, I reformatted the Patch as suggested and added a testcase, but did not add it to the Makefile because it is not a unittest. Its Outcome is crash/no crash, rather OK/NOT OK.
Olaf Flebbe
Comment 5 2011-07-06 00:51:56 PDT
Created attachment 99801 [details] Allocate Timers non-statically It adds an testcase, but does not build and run it. I can remove testcase if not useful that way.
WebKit Review Bot
Comment 6 2011-07-06 00:54:16 PDT
Attachment 99801 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testexitotherthread.c" Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:67: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:68: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/graphics/cairo/ContextShadowCairo.cpp:69: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:70: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:71: Tab found; better to use spaces [whitespace/tab] [1] Source/WebCore/platform/gtk/WidgetRenderingContext.cpp:72: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 6 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Olaf Flebbe
Comment 7 2011-07-06 00:58:19 PDT
Created attachment 99802 [details] Allocate Timers non-statically (next try)
Eric Seidel (no email)
Comment 8 2012-02-16 13:54:26 PST
Comment on attachment 99802 [details] Allocate Timers non-statically (next try) View in context: https://bugs.webkit.org/attachment.cgi?id=99802&action=review This looks reasonable. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This will block the commit-queue. Should be replaced with a description of/path-to your test.
Note You need to log in before you can comment on or make changes to this bug.