#5408: Fix change tracker registration/deregistration leak when handl…

…ing root nodes in the MapResource class.
codereader · Oct 25, 2021 · 2144bcb · 2144bcb
1 parent b5439e4
commit 2144bcb
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 20 deletions.
diff --git a/radiantcore/map/MapResource.cpp b/radiantcore/map/MapResource.cpp
@@ -58,6 +58,11 @@ MapResource::MapResource(const std::string& resourcePath)
     refreshLastModifiedTime();
 }
 
+MapResource::~MapResource()
+{
+    clear();
+}
+
 void MapResource::rename(const std::string& fullPath)
 {
     constructPaths(fullPath);
@@ -98,8 +103,7 @@ bool MapResource::load()
 	if (!_mapRoot)
     {
 		// Map not loaded yet, acquire map root node from loader
-		_mapRoot = loadMapNode();
-		connectMap();
+        setRootNode(loadMapNode());
 		mapSave();
 	}
 
@@ -232,19 +236,23 @@ const scene::IMapRootNodePtr& MapResource::getRootNode()
 
 void MapResource::setRootNode(const scene::IMapRootNodePtr& root)
 {
+    // Unsubscribe from the old root node first
+    if (_mapRoot)
+    {
+        _mapRoot->getUndoChangeTracker().setChangedCallback(std::function<void()>());
+    }
+
     _mapRoot = root;
-}
 
-void MapResource::clear()
-{
-    // Disconnect from the old root first
     if (_mapRoot)
     {
-        _mapRoot->getUndoChangeTracker().setChangedCallback(Callback());
+        _mapRoot->getUndoChangeTracker().setChangedCallback(std::bind(&MapResource::onMapChanged, this));
     }
+}
 
-    _mapRoot = std::make_shared<RootNode>("");
-	connectMap();
+void MapResource::clear()
+{
+    setRootNode(std::make_shared<RootNode>(""));
 }
 
 bool MapResource::fileHasBeenModifiedSinceLastSave()
@@ -259,15 +267,6 @@ void MapResource::onMapChanged()
 	GlobalMap().setModified(!_mapRoot->getUndoChangeTracker().saved());
 }
 
-void MapResource::connectMap()
-{
-    if (_mapRoot)
-    {
-        // Reroute the changed callback to the onMapChanged() call.
-        _mapRoot->getUndoChangeTracker().setChangedCallback(std::bind(&MapResource::onMapChanged, this));
-    }
-}
-
 void MapResource::mapSave()
 {
     if (_mapRoot)

diff --git a/radiantcore/map/MapResource.h b/radiantcore/map/MapResource.h
@@ -40,6 +40,7 @@ class MapResource :
 public:
 	// Constructor
 	MapResource(const std::string& resourcePath);
+    virtual ~MapResource();
 
 	virtual void rename(const std::string& fullPath) override;
 
@@ -85,8 +86,6 @@ class MapResource :
 
 	RootNodePtr loadMapNode();
 
-	void connectMap();
-
 	// Opens a stream for the given path, which might be VFS path or an absolute one. 
     // Throws IMapResource::OperationException on stream open failure.
 	stream::MapResourceStream::Ptr openFileStream(const std::string& path);