Bug 26565 - Windows version of Mutex.lock() should ASSERT on re-entrance
Summary: Windows version of Mutex.lock() should ASSERT on re-entrance
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-19 19:02 PDT by Jeremy Orlow
Modified: 2009-06-22 12:01 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2009-06-19 19:02:44 PDT
Windows version of Mutex.lock() should ASSERT on re-entrance.  The pthread code does not use the flag that allows re-entrance, so we should protect against Windows developers writing code that depends on re-entrance.  An ASSERT seems like the right solution since doing so it not fatal, it's just something we want people to avoid.

I'll send out a patch soon.

Webkit-dev discussion on the topic:
http://lists.macosforge.org/pipermail/webkit-dev/2009-June/008338.html
Comment 1 Jeremy Orlow 2009-06-22 12:01:15 PDT
I looked at the code, and it seems the only change necessary is adding the following ASSERT in JavaScriptCore/wtf/threadingwin.cpp's Mutex::lock() function:

void Mutex::lock()
{
    EnterCriticalSection(&m_mutex.m_internalMutex);
    ++m_mutex.m_recursionCount;
    ASSERT(m_mutex.m_recursionCount == 1);
}

Mutex::tryLock already has the correct behavior.

Unfortunately, it seems that safari.exe relies on the recursive behavior.  Obviously I have no way to fix these dependencies, so it seems that this is a non-starter (at least for someone outside of Apple).  I still think it's a good idea though.

If Safari's Windows code cannot possibly avoid its re-entrance assumption, maybe we could create a sub-class of Mutex called ReentrantMutex.  Only the constructor and lock would need to be overridden.  (No need for anything to be virtual...which would obviously not be practical anyway.)