Merge pull request #15060 from AndrewMeadows/avoid-mem-leaked-entities

Case 21494: Avoid mem leaked entities

interface/src/Application.cpp

@@ -3773,9 +3773,12 @@ std::map<QString, QString> Application::prepareServerlessDomainContents(QUrl dom
         tmpTree->reaverageOctreeElements();
         tmpTree->sendEntities(&_entityEditSender, getEntities()->getTree(), 0, 0, 0);
     }
+    std::map<QString, QString> namedPaths = tmpTree->getNamedPaths();
 
-    return tmpTree->getNamedPaths();
+    // we must manually eraseAllOctreeElements(false) else the tmpTree will mem-leak
+    tmpTree->eraseAllOctreeElements(false);
 
+    return namedPaths;
 }
 
 void Application::loadServerlessDomain(QUrl domainURL) {

libraries/entities-renderer/src/EntityTreeRenderer.cpp

@@ -232,8 +232,6 @@ void EntityTreeRenderer::clearNonLocalEntities() {
             }
         }
         scene->enqueueTransaction(transaction);
-    } else {
-        qCWarning(entitiesrenderer) << "EntitityTreeRenderer::clear(), Unexpected null scene, possibly during application shutdown";
     }
 
     _renderablesToUpdate = savedEntities;

libraries/entities/src/EntityTree.cpp

@@ -52,7 +52,15 @@ EntityTree::EntityTree(bool shouldReaverage) :
 }
 
 EntityTree::~EntityTree() {
-    eraseAllOctreeElements(false);
+    // NOTE: to eraseAllOctreeElements() in this context is useless because
+    // any OctreeElements in the tree still have shared backpointers to this Tree
+    // which means the dtor never would have been called in the first place!
+    //
+    // I'm keeping this useless commented-out line to remind us:
+    // we don't need shared pointer overhead for EntityTrees.
+    // TODO: EntityTreeElement::_tree should be raw back pointer.
+    // AND: EntityItem::_element should be a raw back pointer.
+    //eraseAllOctreeElements(false); // KEEP THIS
 }
 
 void EntityTree::setEntityScriptSourceWhitelist(const QString& entityScriptSourceWhitelist) { 
@@ -74,16 +82,15 @@ void EntityTree::eraseNonLocalEntities() {
     emit clearingEntities();
 
     if (_simulation) {
-        // This will clear all entities host types including local entities, because local entities
-        // are not in the physics simulation
+        // local entities are not in the simulation, so we clear ALL
         _simulation->clearEntities();
     }
-    _staleProxies.clear();
-    QHash<EntityItemID, EntityItemPointer> localMap;
-    localMap.swap(_entityMap);
-    QHash<EntityItemID, EntityItemPointer> savedEntities;
     this->withWriteLock([&] {
-        foreach(EntityItemPointer entity, localMap) {
+        QHash<EntityItemID, EntityItemPointer> savedEntities;
+        // NOTE: lock the Tree first, then lock the _entityMap.
+        // It should never be done the other way around.
+        QReadLocker locker(&_entityMapLock);
+        foreach(EntityItemPointer entity, _entityMap) {
             EntityTreeElementPointer element = entity->getElement();
             if (element) {
                 element->cleanupNonLocalEntities();
@@ -91,11 +98,16 @@ void EntityTree::eraseNonLocalEntities() {
 
             if (entity->isLocalEntity()) {
                 savedEntities[entity->getEntityItemID()] = entity;
+            } else {
+                int32_t spaceIndex = entity->getSpaceIndex();
+                if (spaceIndex != -1) {
+                    // stale spaceIndices will be freed later
+                    _staleProxies.push_back(spaceIndex);
+                }
             }
         }
+        _entityMap.swap(savedEntities);
     });
-    localMap.clear();
-    _entityMap = savedEntities;
 
     resetClientEditStats();
     clearDeletedEntities();
@@ -113,13 +125,13 @@ void EntityTree::eraseNonLocalEntities() {
         _needsParentFixup = localEntitiesNeedsParentFixup;
     }
 }
+
 void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
     emit clearingEntities();
 
     if (_simulation) {
         _simulation->clearEntities();
     }
-    _staleProxies.clear();
     QHash<EntityItemID, EntityItemPointer> localMap;
     localMap.swap(_entityMap);
     this->withWriteLock([&] {
@@ -128,6 +140,11 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) {
             if (element) {
                 element->cleanupEntities();
             }
+            int32_t spaceIndex = entity->getSpaceIndex();
+            if (spaceIndex != -1) {
+                // assume stale spaceIndices will be freed later
+                _staleProxies.push_back(spaceIndex);
+            }
         }
     });
     localMap.clear();
@@ -764,9 +781,9 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator)
             _simulation->prepareEntityForDelete(theEntity);
         }
 
-        // keep a record of valid stale spaceIndices so they can be removed from the Space
         int32_t spaceIndex = theEntity->getSpaceIndex();
         if (spaceIndex != -1) {
+            // stale spaceIndices will be freed later
             _staleProxies.push_back(spaceIndex);
         }
     }