Bug 23073 – (r39465) Workers crash when running raytracer

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

Haven't yet  determined exacty where this goes wrong, but tcmalloc crash is trivially reproducible with the referenced url

[Oliver Hunt]

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
 	kernel32.dll!7c80b50b() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	kernel32.dll!7c8399f3() 	

[Oliver Hunt]

Whoops, wrong url

[Oliver Hunt]

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

[Oliver Hunt]

<rdar://problem/6471129>

[Oliver Hunt]

Introduced between r39450 and r39469

[Oliver Hunt]

r39465 looks likely

[Alexey Proskuryakov]

Created attachment 26382 [details]
attempted fix

Not tested yet, and I'm not quite sure if this will help.

[Oliver Hunt]

Am testing now, will take a while to turn a build.

[Oliver Hunt]

Comment on attachment 26382 [details]
attempted fix

Alas this patch does not appear to fix the issue bug :-(

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

r=me

[Alexey Proskuryakov]

Committed as <http://trac.webkit.org/changeset/39604>.