[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 #4 from Mark Toller <mark.toller at samsung.com>  2012-09-26 00:45:59 PST ---
(In reply to comment #2)
> (From update of attachment 165565 [details])
> I think that this code should be:
> 
> -    ASSERT(pthread_mutex_trylock(m_scavengeMutex));
> +    ASSERT(!pthread_mutex_trylock(&m_scavengeMutex));
> 
> Does such an assert make more sense?

No, because if that code executes it will either deadlock if the lock is acquired, or assert if it fails... And it can (and does) fail if the TCMalloc_PageHeap::scavengerThread() has locked the mutex, and not yet released it in the pthread_cond_wait() call.

As Benjamin stated, I should put some of this detail in the Changelog as well.

You *could* change it to an :

ASSERT(pthread_mutex_lock(&m_scavengeMutex));
if (!m_scavengeThreadActive && shouldScavenge())
        pthread_cond_signal(&m_scavengeCondition);
ASSERT(pthread_mutex_unlock(&m_scavengeMutex));

However, if the mutex lock/unlock is *needed* then this code shouldn't be wrapped in ASSERT macros, because they are compiled out in release builds... And if they're not required for release builds, and provide no additional benefit in debug, then why have the overhead at all?

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