Skip to content

Commit

Permalink
MessageManager: Improve thread safety of Lock type
Browse files Browse the repository at this point in the history
Previously, the following sequence of events was possible:

Background thread                   Main thread
------------------------------------------------------------------------
Lock::tryAcquire()
    Run to blockingMessage->post()

                                    BlockingMessage::messageCallback()
                                        Run to abortWait.set (1)

Lock::tryAcquire()
    Exit through return true

Lock::~Lock()
    Destroy memory used for Lock

                                    BlockingMessage::messageCallback()
                                        Execute lockedEvent.signal()
                                        Memory already freed, crash
  • Loading branch information
reuk committed May 2, 2023
1 parent 2d31153 commit 33e8161
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 51 deletions.
104 changes: 56 additions & 48 deletions modules/juce_events/messages/juce_MessageManager.cpp
Expand Up @@ -284,25 +284,41 @@ bool MessageManager::existsAndIsCurrentThread() noexcept
*/
struct MessageManager::Lock::BlockingMessage : public MessageManager::MessageBase
{
BlockingMessage (const MessageManager::Lock* parent) noexcept
: owner (parent)
{}
explicit BlockingMessage (const MessageManager::Lock* parent) noexcept
: owner (parent) {}

void messageCallback() override
{
{
ScopedLock lock (ownerCriticalSection);
std::unique_lock lock { mutex };

if (auto* o = owner.get())
o->messageCallback();
if (owner != nullptr)
{
owner->abort();
acquired = true;
}

releaseEvent.wait();
condvar.wait (lock, [&] { return owner == nullptr; });
}

void stopWaiting()
{
const ScopeGuard scope { [&] { condvar.notify_one(); } };
const std::scoped_lock lock { mutex };
owner = nullptr;
}

bool wasAcquired()
{
const std::scoped_lock lock { mutex };
return acquired;
}

CriticalSection ownerCriticalSection;
Atomic<const MessageManager::Lock*> owner;
WaitableEvent releaseEvent;
private:
std::mutex mutex;
std::condition_variable condvar;

const MessageManager::Lock* owner = nullptr;
bool acquired = false;

JUCE_DECLARE_NON_COPYABLE (BlockingMessage)
};
Expand All @@ -323,9 +339,12 @@ bool MessageManager::Lock::tryAcquire (bool lockIsMandatory) const noexcept
return false;
}

if (! lockIsMandatory && (abortWait.get() != 0))
if (! lockIsMandatory && [&]
{
const std::scoped_lock lock { mutex };
return std::exchange (abortWait, false);
}())
{
abortWait.set (0);
return false;
}

Expand All @@ -350,65 +369,54 @@ bool MessageManager::Lock::tryAcquire (bool lockIsMandatory) const noexcept
return false;
}

do
for (;;)
{
while (abortWait.get() == 0)
lockedEvent.wait (-1);

abortWait.set (0);
{
std::unique_lock lock { mutex };
condvar.wait (lock, [&] { return std::exchange (abortWait, false); });
}

if (lockGained.get() != 0)
if (blockingMessage->wasAcquired())
{
mm->threadWithLock = Thread::getCurrentThreadId();
return true;
}

} while (lockIsMandatory);
if (! lockIsMandatory)
break;
}

// we didn't get the lock
blockingMessage->releaseEvent.signal();

{
ScopedLock lock (blockingMessage->ownerCriticalSection);

lockGained.set (0);
blockingMessage->owner.set (nullptr);
}

blockingMessage->stopWaiting();
blockingMessage = nullptr;
return false;
}

void MessageManager::Lock::exit() const noexcept
{
if (lockGained.compareAndSetBool (false, true))
{
auto* mm = MessageManager::instance;
if (blockingMessage == nullptr)
return;

jassert (mm == nullptr || mm->currentThreadHasLockedMessageManager());
lockGained.set (0);
const ScopeGuard scope { [&] { blockingMessage = nullptr; } };

if (mm != nullptr)
mm->threadWithLock = {};
blockingMessage->stopWaiting();

if (blockingMessage != nullptr)
{
blockingMessage->releaseEvent.signal();
blockingMessage = nullptr;
}
}
}
if (! blockingMessage->wasAcquired())
return;

void MessageManager::Lock::messageCallback() const
{
lockGained.set (1);
abort();
if (auto* mm = MessageManager::instance)
{
jassert (mm->currentThreadHasLockedMessageManager());
mm->threadWithLock = {};
}
}

void MessageManager::Lock::abort() const noexcept
{
abortWait.set (1);
lockedEvent.signal();
const ScopeGuard scope { [&] { condvar.notify_one(); } };
const std::scoped_lock lock { mutex };
abortWait = true;
}

//==============================================================================
Expand Down
6 changes: 3 additions & 3 deletions modules/juce_events/messages/juce_MessageManager.h
Expand Up @@ -298,12 +298,12 @@ class JUCE_API MessageManager final
friend class ReferenceCountedObjectPtr<BlockingMessage>;

bool tryAcquire (bool) const noexcept;
void messageCallback() const;

//==============================================================================
mutable ReferenceCountedObjectPtr<BlockingMessage> blockingMessage;
WaitableEvent lockedEvent;
mutable Atomic<int> abortWait, lockGained;
mutable std::mutex mutex;
mutable std::condition_variable condvar;
mutable bool abortWait = false, acquired = false;
};

//==============================================================================
Expand Down

0 comments on commit 33e8161

Please sign in to comment.