From 2fe146646ccf069a1c9651551c1fb8a41a58eef0 Mon Sep 17 00:00:00 2001
From: Paul Lemire <paul.lemire@kdab.com>
Date: Fri, 17 Apr 2020 10:09:50 +0200
Subject: Animations: fix race condition on running clip animators vector

Was caused by incorrect dependencies on the jobs.

Introduced a clearDependencies on QAspectJobPrivate to clear all
dependencies as removeDepencies(emptyVector) only removes null
dependencies.

Change-Id: I8119a9edaf841db6c5ab2a971dc5640da2192cba
Reviewed-by: Mike Krus <mike.krus@kdab.com>
---
 src/animation/backend/handler.cpp | 25 ++++++++++---------------
 src/core/jobs/qaspectjob_p.h      |  2 ++
 2 files changed, 12 insertions(+), 15 deletions(-)

(limited to 'src')

diff --git a/src/animation/backend/handler.cpp b/src/animation/backend/handler.cpp
index 84543c264..95363d56f 100644
--- a/src/animation/backend/handler.cpp
+++ b/src/animation/backend/handler.cpp
@@ -44,6 +44,7 @@
 #include <Qt3DAnimation/private/animationlogging_p.h>
 #include <Qt3DAnimation/private/buildblendtreesjob_p.h>
 #include <Qt3DAnimation/private/evaluateblendclipanimatorjob_p.h>
+#include <Qt3DCore/private/qaspectjob_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -115,11 +116,7 @@ void Handler::setClipAnimatorRunning(const HClipAnimator &handle, bool running)
 
     // If being marked as not running, remove from set of running clips
     if (!running) {
-        const auto it = std::find_if(m_runningClipAnimators.begin(),
-                                     m_runningClipAnimators.end(),
-                                     [handle](const HClipAnimator &h) { return h == handle; });
-        if (it != m_runningClipAnimators.end())
-            m_runningClipAnimators.erase(it);
+        m_runningClipAnimators.removeAll(handle);
     }
 }
 
@@ -205,13 +202,14 @@ QVector<Qt3DCore::QAspectJobPtr> Handler::jobsToExecute(qint64 time)
     const bool hasFindRunningClipAnimatorsJob = !m_dirtyClipAnimators.isEmpty();
     if (hasFindRunningClipAnimatorsJob) {
         qCDebug(HandlerLogic) << "Added FindRunningClipAnimatorsJob";
-        m_findRunningClipAnimatorsJob->removeDependency(QWeakPointer<Qt3DCore::QAspectJob>());
         cleanupHandleList(&m_dirtyClipAnimators);
         m_findRunningClipAnimatorsJob->setDirtyClipAnimators(m_dirtyClipAnimators);
+        // Only set the dependency once
+        if (Q_UNLIKELY(m_findRunningClipAnimatorsJob->dependencies().empty()))
+            m_findRunningClipAnimatorsJob->addDependency(m_loadAnimationClipJob);
         jobs.push_back(m_findRunningClipAnimatorsJob);
         if (hasLoadAnimationClipJob)
-            m_findRunningClipAnimatorsJob->addDependency(m_loadAnimationClipJob);
-        m_dirtyClipAnimators.clear();
+            m_dirtyClipAnimators.clear();
     }
 
     // Rebuild blending trees if a blend tree is dirty
@@ -244,13 +242,10 @@ QVector<Qt3DCore::QAspectJobPtr> Handler::jobsToExecute(qint64 time)
         // Set each job up with an animator to process and set dependencies
         for (int i = 0; i < newSize; ++i) {
             m_evaluateClipAnimatorJobs[i]->setClipAnimator(m_runningClipAnimators[i]);
-            m_evaluateClipAnimatorJobs[i]->removeDependency(QWeakPointer<Qt3DCore::QAspectJob>());
-            if (hasLoadAnimationClipJob &&
-                    !m_evaluateClipAnimatorJobs[i]->dependencies().contains(m_loadAnimationClipJob))
+            Qt3DCore::QAspectJobPrivate::get(m_evaluateClipAnimatorJobs[i].data())->clearDependencies();
+            if (hasLoadAnimationClipJob)
                 m_evaluateClipAnimatorJobs[i]->addDependency(m_loadAnimationClipJob);
-
-            if (hasFindRunningClipAnimatorsJob &&
-                    !m_evaluateClipAnimatorJobs[i]->dependencies().contains(m_findRunningClipAnimatorsJob))
+            if (hasFindRunningClipAnimatorsJob)
                 m_evaluateClipAnimatorJobs[i]->addDependency(m_findRunningClipAnimatorsJob);
             jobs.push_back(m_evaluateClipAnimatorJobs[i]);
         }
@@ -273,7 +268,7 @@ QVector<Qt3DCore::QAspectJobPtr> Handler::jobsToExecute(qint64 time)
         // Set each job up with an animator to process and set dependencies
         for (int i = 0; i < newSize; ++i) {
             m_evaluateBlendClipAnimatorJobs[i]->setBlendClipAnimator(m_runningBlendedClipAnimators[i]);
-            m_evaluateBlendClipAnimatorJobs[i]->removeDependency(QWeakPointer<Qt3DCore::QAspectJob>());
+            Qt3DCore::QAspectJobPrivate::get(m_evaluateBlendClipAnimatorJobs[i].data())->clearDependencies();
             if (hasLoadAnimationClipJob)
                 m_evaluateBlendClipAnimatorJobs[i]->addDependency(m_loadAnimationClipJob);
             if (hasBuildBlendTreesJob)
diff --git a/src/core/jobs/qaspectjob_p.h b/src/core/jobs/qaspectjob_p.h
index 0337fa107..0c7802b02 100644
--- a/src/core/jobs/qaspectjob_p.h
+++ b/src/core/jobs/qaspectjob_p.h
@@ -75,6 +75,8 @@ public:
     virtual bool isRequired() const;
     virtual void postFrame(QAspectManager *aspectManager);
 
+    void clearDependencies() { m_dependencies.clear(); }
+
     QVector<QWeakPointer<QAspectJob> > m_dependencies;
     JobId m_jobId;
     QString m_jobName;
-- 
cgit v1.2.3