[Webkit-unassigned] [Bug 97539] Broken and incorrect code in FastMalloc.cpp

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=97539





--- Comment #16 from Mark Toller <mark.toller at samsung.com>  2012-09-28 00:59:24 PST ---
(In reply to comment #15)

> m_scavengeThreadActive is an atomic variable. Why do you need to lock across assignment?

I think you're right. However, we should lock pageheap_lock before calling 'shouldScavenge()', as we only want to scavenge when really required, so it's better to wait for any current memory operation to complete before checking. In this case, we may as well set m_scavengeThreadActive = false while still holding the lock.

Do we need to re-lock before setting m_scavengeThreadActive = true ? No. In fact, it's probably better to do so outside of the lock, as this could prevent an unnecessary signal of the condvar (if we waited for a current delete op holding the spinlock to complete, which calls signalScavenger()).

> 
> > Source/WTF/wtf/FastMalloc.cpp:2564
> >            pthread_mutex_unlock(&m_scavengeMutex);
> 
> I believe this unlock is wrong, as per the pthread_cond_mutex man page:
> 
> 
>      The pthread_cond_wait() function atomically unlocks the mutex argument
>      and waits on the cond argument. Before returning control to the calling
>      function, pthread_cond_wait() re-acquires the mutex.

If we don't unlock the mutex (type is PTHREAD_MUTEX_NORMAL) which we now hold after exiting the pthread_cond_wait, then next time around the loop we will deadlock:

    If the mutex type is PTHREAD_MUTEX_NORMAL, deadlock detection is not 
    provided. Attempting to relock the mutex causes deadlock.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.