Bug 22364 – Crashes seen on Tiger buildbots due to worker threads exhausting pthread keys

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

[Alexey Proskuryakov]

Currently, each heap creates a pthread key that tracks threads that use this heap. This isn't necessary for workers, which are tied to a single thread.

[Alexey Proskuryakov]

Created attachment 25307 [details]
proposed fix

[Darin Adler]

Comment on attachment 25307 [details]
proposed fix

>  #ifndef NDEBUG
> +        int error =
>  #endif
> +            pthread_key_delete(m_currentThreadRegistrar);
> +        ASSERT(!error);
> +    }

The little ditties we do so we can assert the results of these functions are ugly enough that I think we should instead, in the future, make little helper inline functions to encapsulate them. They distract too much from the rest of the flow of the functions they are in.

> +void Heap::makeUsableFromMultipleThreads()
> +{
> +    if (m_currentThreadRegistrar)
> +        return;
> +
> +    int error = pthread_key_create(&m_currentThreadRegistrar, unregisterThread);
> +    if (error)
> +        CRASH();
> +}

I wonder what we should do about cases like this. I think I understand what's going on -- this is an assertion, but one so important that you'd like it to be included in production code. But I'm not sure that would be clear to other readers. Also, it's not clear when to do abort() and when to do CRASH(). We should create some guidelines for this.

> +#if ENABLE(JSC_MULTIPLE_THREADS)
> +        instance->makeUsableFromMultipleThreads();
> +#endif

In general, when the semantics of a function like this is simple, I prefer to make empty inline versions so we don't need to make each call site conditional. That would have prevented us from needing conditionals in JSGlobalData.h and here in JSGlobalData.cpp and in JSContextRef.cpp.

What do you think of that practice?

r=me as is

[Alexey Proskuryakov]

Committed revision 38622.

(In reply to comment #2)
> Also, it's not clear when to do abort() and when to do CRASH(). We
> should create some guidelines for this.

I don't like calling abort(), because it doesn't give any information for a bug report. Also, I think it's impolite to silently close an application. as if it was never running.

> What do you think of that practice?

In general, it's fine, but I didn't want code that's not guarded with ENABLE(JSC_MULTIPLE_THREADS) to call makeUsableFromMultipleThreads(), and I still don't want to.