From 9bc687e20992cf5b7941343d7bce379f2c66b252 Mon Sep 17 00:00:00 2001
From: fredzio <chardon.frederic@gmail.com>
Date: Sat, 9 Jan 2021 20:56:20 +0100
Subject: [PATCH] Avoid a rare but possible deadlock around
 mCollisionWorldMutex.

What happened is that the last handle to an Actor shared_ptr was a
promoted weak_ptr. When the shared_ptr goes out of scope, the Actor dtor
is invoked. That involves removing the Actor collision object after
exclusively locking mCollisionWorldMutex. In this case, the lock was
already held in the outter scope of the promoted weak_ptr.

Reduce the scope of the mCollisionWorldMutex to never encompass the
lifetime of a promoted weak_ptr.
---
 apps/openmw/mwphysics/mtphysics.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/apps/openmw/mwphysics/mtphysics.cpp b/apps/openmw/mwphysics/mtphysics.cpp
index 0d1e5962a..c77bc6acd 100644
--- a/apps/openmw/mwphysics/mtphysics.cpp
+++ b/apps/openmw/mwphysics/mtphysics.cpp
@@ -357,7 +357,6 @@ namespace MWPhysics
     {
         if (!mDeferAabbUpdate || immediate)
         {
-            std::unique_lock lock(mCollisionWorldMutex);
             updatePtrAabb(ptr);
         }
         else
@@ -410,7 +409,7 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::updateAabbs()
     {
-        std::scoped_lock lock(mCollisionWorldMutex, mUpdateAabbMutex);
+        std::scoped_lock lock(mUpdateAabbMutex);
         std::for_each(mUpdateAabb.begin(), mUpdateAabb.end(),
             [this](const std::weak_ptr<PtrHolder>& ptr) { updatePtrAabb(ptr); });
         mUpdateAabb.clear();
@@ -420,6 +419,7 @@ namespace MWPhysics
     {
         if (const auto p = ptr.lock())
         {
+            std::scoped_lock lock(mCollisionWorldMutex);
             if (const auto actor = std::dynamic_pointer_cast<Actor>(p))
             {
                 actor->updateCollisionObjectPosition();
@@ -451,9 +451,11 @@ namespace MWPhysics
             int job = 0;
             while (mRemainingSteps && (job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs)
             {
-                MaybeSharedLock lockColWorld(mCollisionWorldMutex, mThreadSafeBullet);
                 if(const auto actor = mActorsFrameData[job].mActor.lock())
+                {
+                    MaybeSharedLock lockColWorld(mCollisionWorldMutex, mThreadSafeBullet);
                     MovementSolver::move(mActorsFrameData[job], mPhysicsDt, mCollisionWorld.get(), *mWorldFrameData);
+                }
             }
 
             mPostStepBarrier->wait();
@@ -478,13 +480,13 @@ namespace MWPhysics
 
     void PhysicsTaskScheduler::updateActorsPositions()
     {
-        std::unique_lock lock(mCollisionWorldMutex);
         for (auto& actorData : mActorsFrameData)
         {
             if(const auto actor = actorData.mActor.lock())
             {
                 if (actor->setPosition(actorData.mPosition))
                 {
+                    std::scoped_lock lock(mCollisionWorldMutex);
                     actor->updateCollisionObjectPosition();
                     mCollisionWorld->updateSingleAabb(actor->getCollisionObject());
                 }