static destructors calling Timer.stop in wrong thread

• Component: WebKitGTK
• Status: NEW ---

[Olaf Flebbe]

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.

[Olaf Flebbe]

Created attachment 99689 [details]
Patch to GtkLauncher, just to demonstrate the Problem compile with --enable-debug !

[Olaf Flebbe]

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]

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]

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]

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]

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]

Created attachment 99802 [details]
Allocate Timers non-statically (next try)

[Eric Seidel (no email)]

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.