From 865a77ae2016c171287d0f65e5b3823f77beef49 Mon Sep 17 00:00:00 2001
From: Anthony Thibault <ajt@hyperlogic.org>
Date: Sat, 2 Apr 2016 21:48:22 -0700
Subject: [PATCH] Model: fixed two crash issues when changing avatars

* When the GeometryReader has the last ref to the GeometryResource ptr
  It needs to hold on to the reference until invokeMethod is completed.
  Otherwise, invokeMethod will call a method on a deleted object, leading
  to memory corruption or crashes.

* When the Model URL is changed, the clusterMatrices are invalided and the
  RenderItemsSets are cleared.  However, there still might be renderItems in
  the scene pending changes list that might refer to those RenderItems and their
  clusterMatrices.  We need to guard against this access to prevent reading from
  memory that was previously freed.

Both of these issues were uncovered using the [avatar-thrasher](https://gist.github.com/hyperlogic/d82a61d141df43d576428501a82c5ee6) test script.
---
 .../src/model-networking/ModelCache.cpp       | 25 +++++++++++++------
 libraries/render-utils/src/Model.cpp          |  7 ++++--
 libraries/render-utils/src/Model.h            |  2 ++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/libraries/model-networking/src/model-networking/ModelCache.cpp b/libraries/model-networking/src/model-networking/ModelCache.cpp
index 41c2a2e194..9b5acc726e 100644
--- a/libraries/model-networking/src/model-networking/ModelCache.cpp
+++ b/libraries/model-networking/src/model-networking/ModelCache.cpp
@@ -118,10 +118,8 @@ void GeometryReader::run() {
     }
     QThread::currentThread()->setPriority(QThread::LowPriority);
 
-    // Ensure the resource is still being requested
-    auto resource = _resource.toStrongRef();
-    if (!resource) {
-        qCWarning(modelnetworking) << "Abandoning load of" << _url << "; could not get strong ref";
+    if (!_resource.data()) {
+        qCWarning(modelnetworking) << "Abandoning load of" << _url << "; resource was deleted";
         return;
     }
 
@@ -146,14 +144,27 @@ void GeometryReader::run() {
                 throw QString("unsupported format");
             }
 
-            QMetaObject::invokeMethod(resource.data(), "setGeometryDefinition",
-                Q_ARG(void*, fbxGeometry));
+            // Ensure the resource has not been deleted, and won't be while invoke method is in flight.
+            auto resource = _resource.toStrongRef();
+            if (!resource) {
+                qCWarning(modelnetworking) << "Abandoning load of" << _url << "; could not get strong ref";
+            } else {
+                QMetaObject::invokeMethod(resource.data(), "setGeometryDefinition", Qt::BlockingQueuedConnection, Q_ARG(void*, fbxGeometry));
+            }
         } else {
             throw QString("url is invalid");
         }
     } catch (const QString& error) {
+
         qCDebug(modelnetworking) << "Error reading " << _url << ": " << error;
-        QMetaObject::invokeMethod(resource.data(), "finishedLoading", Q_ARG(bool, false));
+
+        auto resource = _resource.toStrongRef();
+        // Ensure the resoruce has not been deleted, and won't be while invoke method is in flight.
+        if (!resource) {
+            qCWarning(modelnetworking) << "Abandoning load of" << _url << "; could not get strong ref";
+        } else {
+            QMetaObject::invokeMethod(resource.data(), "finishedLoading", Qt::BlockingQueuedConnection, Q_ARG(bool, false));
+        }
     }
 
     QThread::currentThread()->setPriority(originalPriority);
diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp
index 858f7d3ee7..eda856342c 100644
--- a/libraries/render-utils/src/Model.cpp
+++ b/libraries/render-utils/src/Model.cpp
@@ -159,11 +159,13 @@ void Model::updateRenderItems() {
         Transform collisionMeshOffset;
         collisionMeshOffset.postTranslate(self->_offset);
 
+        uint32_t deleteGeometryCounter = self->_deleteGeometryCounter;
+
         render::PendingChanges pendingChanges;
         foreach (auto itemID, self->_modelMeshRenderItems.keys()) {
-            pendingChanges.updateItem<ModelMeshPartPayload>(itemID, [modelTransform, modelMeshOffset](ModelMeshPartPayload& data) {
+            pendingChanges.updateItem<ModelMeshPartPayload>(itemID, [modelTransform, modelMeshOffset, deleteGeometryCounter](ModelMeshPartPayload& data) {
                 // Ensure the model geometry was not reset between frames
-                if (data._model->isLoaded()) {
+                if (data._model && data._model->isLoaded() && deleteGeometryCounter == data._model->_deleteGeometryCounter) {
                     // lazy update of cluster matrices used for rendering.  We need to update them here, so we can correctly update the bounding box.
                     data._model->updateClusterMatrices(modelTransform.getTranslation(), modelTransform.getRotation());
 
@@ -1146,6 +1148,7 @@ void Model::setBlendedVertices(int blendNumber, const std::weak_ptr<NetworkGeome
 }
 
 void Model::deleteGeometry() {
+    _deleteGeometryCounter++;
     _blendedVertexBuffers.clear();
     _meshStates.clear();
     _rig->destroyAnimGraph();
diff --git a/libraries/render-utils/src/Model.h b/libraries/render-utils/src/Model.h
index 4e6e727b44..581184918d 100644
--- a/libraries/render-utils/src/Model.h
+++ b/libraries/render-utils/src/Model.h
@@ -394,6 +394,8 @@ protected:
 
     friend class ModelMeshPartPayload;
     RigPointer _rig;
+
+    uint32_t _deleteGeometryCounter { 0 };
 };
 
 Q_DECLARE_METATYPE(ModelPointer)