Bug 23073 - (r39465) Workers crash when running raytracer
Summary: (r39465) Workers crash when running raytracer
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Windows XP
: P1 Normal
Assignee: Alexey Proskuryakov
URL: http://nerget.com/rayjs-mt/rayjs.html...
Keywords: InRadar
Depends on:
Reported: 2009-01-02 02:51 PST by Oliver Hunt
Modified: 2009-01-05 10:16 PST (History)
2 users (show)

See Also:

attempted fix (1.84 KB, patch)
2009-01-02 13:27 PST, Alexey Proskuryakov
oliver: review-
Details | Formatted Diff | Diff
proposed fix (5.26 KB, patch)
2009-01-05 08:54 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2009-01-02 02:51:58 PST
Haven't yet  determined exacty where this goes wrong, but tcmalloc crash is trivially reproducible with the referenced url
Comment 1 Oliver Hunt 2009-01-02 02:57:00 PST
Crash occurs as:
>	WebKit.dll!WTF::PageHeapAllocator<WTF::TCMalloc_ThreadCache>::New()  Line 843	C++
 	WebKit.dll!WTF::TCMalloc_ThreadCache::NewHeap(unsigned long tid=1104)  Line 2472 + 0x5 bytes	C++
 	WebKit.dll!WTF::TCMalloc_ThreadCache::CreateCacheIfNecessary()  Line 2583 + 0x6 bytes	C++
 	WebKit.dll!WTF::fastMalloc(unsigned int size=148)  Line 3235 + 0x2a bytes	C++
 	WebKit.dll!WebCore::WorkerThread::workerThread()  Line 82 + 0xa bytes	C++
 	WebKit.dll!WebCore::WorkerThread::workerThreadStart(void * thread=0x7f88e420)  Line 76	C++
 	WebKit.dll!WTF::threadEntryPoint(void * contextData=0x7fabb2d0)  Line 57 + 0x3 bytes	C++
 	WebKit.dll!WTF::wtfThreadEntryPoint(void * param=)  Line 182 + 0x3 bytes	C++
 	msvcr80.dll!_callthreadstartex()  Line 348 + 0x6 bytes	C
 	msvcr80.dll!_threadstartex(void * ptd=0x03fa3d28)  Line 326 + 0x5 bytes	C
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

Comment 2 Oliver Hunt 2009-01-02 03:45:04 PST
Whoops, wrong url
Comment 3 Oliver Hunt 2009-01-02 03:56:46 PST
Can't reproduce this in a debug build, only release.

Steps to repro:
1. fast/workers/worker-replace-self.html
2. fast/workers/worker-terminate.html

Comment 4 Oliver Hunt 2009-01-02 04:26:02 PST
Comment 5 Oliver Hunt 2009-01-02 04:41:49 PST
Introduced between r39450 and r39469
Comment 6 Oliver Hunt 2009-01-02 04:47:45 PST
r39465 looks likely
Comment 7 Alexey Proskuryakov 2009-01-02 13:27:56 PST
Created attachment 26382 [details]
attempted fix

Not tested yet, and I'm not quite sure if this will help.
Comment 8 Oliver Hunt 2009-01-02 14:33:03 PST
Am testing now, will take a while to turn a build.
Comment 9 Oliver Hunt 2009-01-02 15:04:59 PST
Comment on attachment 26382 [details]
attempted fix

Alas this patch does not appear to fix the issue bug :-(
Comment 10 Alexey Proskuryakov 2009-01-03 07:02:34 PST
OK. This patch is actually a no-op - after someone's "build fix" for another platform, this code wasn't compiled in anyway.

Something else that goes wrong is that ThreadGlobalData is re-created from its own destructor, because AtomicString destructor tries to look up an already zeroed out thread specific instance. I'm not quite sure whether this is the culprit yet.
Comment 11 Alexey Proskuryakov 2009-01-05 08:54:16 PST
Created attachment 26432 [details]
proposed fix

This fixes the crash for me (a debug build with FastMalloc enabled).

I'm not sure if this gets to the lowest level root cause though - it could be some FastMalloc bug related to using it after TCMalloc_ThreadCache has been destroyed from a pthread_key_create callback.
Comment 12 Darin Adler 2009-01-05 09:36:37 PST
Comment on attachment 26432 [details]
proposed fix

> +        (WTF::::destroy): Changed to clear the pointer only after data object destruction is
> +        finished - otherwise, WebCore::ThreadGlobalData destructor was re-creating the object
> +        in order to access atomic string table.

The prepare-ChangeLog script generated a bad function name here.

> +        (WTF::operator T*): Symmetrically, set up the per-thread pointer before data constructor
> +        is called.

And here. Human intervention is required!

> +    data->value->~T();
> +    fastFree(data->value);
>      pthread_setspecific(data->owner->m_key, 0);
> -    delete data->value;

I think this really needs a comment. It's quite non-obvious that the ordering is important from  looking at the code.

> -        ptr = new T();
> +        ptr = static_cast<T*>(fastMalloc(sizeof(T)));
>          set(ptr);
> +        new (ptr) T;

Same thing goes here, but even more so. You need to justify why it was necessary to use placement new here.

Change looks great.

Comment 13 Alexey Proskuryakov 2009-01-05 10:16:28 PST
Committed as <http://trac.webkit.org/changeset/39604>.