From 8bf76e51119aa49e745b078ceed3ffa67a89aea1 Mon Sep 17 00:00:00 2001
From: Sean Harmer <sean.harmer@kdab.com>
Date: Thu, 2 Jun 2016 10:38:58 +0100
Subject: Mutex protect the RenderStateCollection

Without the mutex, this class will race when a framegraph contains
multiple branches that result in renderview jobs being created. This
is because one thread will call RenderStateCollection::renderStates()
as part of building render commands. Then some time later (or at the
same time) another thread could call this function too resulting in
undefined behavior because the data changed by the first thread will
not be pushed to the cpu cache of the second thread (or may be only
partially updated) breaking the invariants of this class.

Added a mutex to avoid the data race. In the future we should instead
have the jobs maintain a local cache of such data so that we do not
end up locking the QBackendNodes a lot in the jobs as this will kill
parallelism just as QVariant does. Better to have the backend nodes as
purely read only during job execution even if this means having an
earlier job that populates an intermediate cache like we planned for
the layer filtering (analogous to a relational databases temporary
table concept).

Task-number: QTBUG-53783
Change-Id: I33043dfd23d8b2e01369bdae25b933e3cfec78c8
Reviewed-by: Paul Lemire <paul.lemire@kdab.com>
---
 src/render/renderstates/renderstatecollection.cpp | 5 +++++
 src/render/renderstates/renderstatecollection_p.h | 2 ++
 2 files changed, 7 insertions(+)

(limited to 'src')

diff --git a/src/render/renderstates/renderstatecollection.cpp b/src/render/renderstates/renderstatecollection.cpp
index 17e794820..f9a7cb305 100644
--- a/src/render/renderstates/renderstatecollection.cpp
+++ b/src/render/renderstates/renderstatecollection.cpp
@@ -55,11 +55,13 @@ RenderStateCollection::~RenderStateCollection()
 
 void RenderStateCollection::setDirty(bool dirty)
 {
+    QMutexLocker lock(&m_mutex);
     m_dirty = dirty;
 }
 
 QVector<RenderStateNode*> RenderStateCollection::renderStates(RenderStateManager *manager) const
 {
+    QMutexLocker lock(&m_mutex);
     if (m_dirty) {
         m_renderStateNodes.clear();
 
@@ -79,11 +81,13 @@ QVector<RenderStateNode*> RenderStateCollection::renderStates(RenderStateManager
 
 bool RenderStateCollection::hasRenderStates() const
 {
+    QMutexLocker lock(&m_mutex);
     return !m_renderStateIds.empty();
 }
 
 void RenderStateCollection::appendRenderState(Qt3DCore::QNodeId renderStateId)
 {
+    QMutexLocker lock(&m_mutex);
     if (!m_renderStateIds.contains(renderStateId)) {
         m_renderStateIds.append(renderStateId);
         m_dirty = true;
@@ -92,6 +96,7 @@ void RenderStateCollection::appendRenderState(Qt3DCore::QNodeId renderStateId)
 
 void RenderStateCollection::removeRenderState(Qt3DCore::QNodeId renderStateId)
 {
+    QMutexLocker lock(&m_mutex);
     if (m_renderStateIds.removeAll(renderStateId) > 0) {
         m_dirty = true;
     }
diff --git a/src/render/renderstates/renderstatecollection_p.h b/src/render/renderstates/renderstatecollection_p.h
index b86f499fe..751d41250 100644
--- a/src/render/renderstates/renderstatecollection_p.h
+++ b/src/render/renderstates/renderstatecollection_p.h
@@ -49,6 +49,7 @@
 //
 
 #include <Qt3DRender/private/renderstatenode_p.h>
+#include <QtCore/qmutex.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -82,6 +83,7 @@ private:
     mutable QVector<Qt3DCore::QNodeId> m_renderStateIds;
     mutable QVector<RenderStateNode*> m_renderStateNodes;
     mutable bool m_dirty;
+    mutable QMutex m_mutex;
 };
 
 } // namespace Render
-- 
cgit v1.2.3