use shared pointer inside Chunk Cache

In very seldom cases Minnutor still crashes with memory access violations: If a Chunk gets removed from the Cache but still gets loaded or rendered, the data structure is removed from heap, but still written to. By using QSharedPointer<Chunk> always some routine has ownership and the heap is only freed when all routines are finished with processing.
cre4ture · Aug 6, 2019 · ed12f90 · ed12f90
1 parent e0c1b08
commit ed12f90
Show file tree

Hide file tree

Showing 12 changed files with 47 additions and 38 deletions.
diff --git a/chunkcache.cpp b/chunkcache.cpp
@@ -69,39 +69,43 @@ QString ChunkCache::getPath() const {
   return path;
 }
 
-Chunk *ChunkCache::fetchCached(int cx, int cz) {
+QSharedPointer<Chunk> ChunkCache::fetchCached(int cx, int cz) {
   // try to get Chunk from Cache
   ChunkID id(cx, cz);
   mutex.lock();
-  Chunk *chunk = cache[id];   // const operation
+  QSharedPointer<Chunk> * p_chunk(cache[id]);   // const operation
   mutex.unlock();
 
-  return chunk;
+  if (p_chunk != NULL )
+    return QSharedPointer<Chunk>(*p_chunk);
+  else
+    return QSharedPointer<Chunk>(NULL);  // we're loading this chunk, or it's blank.
 }
 
-Chunk *ChunkCache::fetch(int cx, int cz) {
+QSharedPointer<Chunk> ChunkCache::fetch(int cx, int cz) {
   // try to get Chunk from Cache
   ChunkID id(cx, cz);
   mutex.lock();
-  Chunk *chunk = cache[id];   // const operation
+  QSharedPointer<Chunk> * p_chunk(cache[id]);   // const operation
   mutex.unlock();
-  if (chunk != NULL) {
+  if (p_chunk != NULL ) {
+    QSharedPointer<Chunk> chunk(*p_chunk);
     if (chunk->loaded)
       return chunk;
-    return NULL;  // we're loading this chunk, or it's blank.
+    return QSharedPointer<Chunk>(NULL);  // we're loading this chunk, or it's blank.
   }
   // launch background process to load this chunk
-  chunk = new Chunk();
-  connect(chunk, SIGNAL(structureFound(QSharedPointer<GeneratedStructure>)),
-          this,  SLOT  (routeStructure(QSharedPointer<GeneratedStructure>)));
+  p_chunk = new QSharedPointer<Chunk>(new Chunk());
+  connect(p_chunk->data(), SIGNAL(structureFound(QSharedPointer<GeneratedStructure>)),
+          this,            SLOT  (routeStructure(QSharedPointer<GeneratedStructure>)));
   mutex.lock();
-  cache.insert(id, chunk);    // non-const operation !
+  cache.insert(id, p_chunk);    // non-const operation !
   mutex.unlock();
   ChunkLoader *loader = new ChunkLoader(path, cx, cz);
   connect(loader, SIGNAL(loaded(int, int)),
           this,   SLOT(gotChunk(int, int)));
   loaderThreadPool.start(loader);
-  return NULL;
+  return QSharedPointer<Chunk>(NULL);
 }
 
 void ChunkCache::gotChunk(int cx, int cz) {

diff --git a/chunkcache.h b/chunkcache.h
@@ -6,6 +6,8 @@
 #include <QCache>
 #include "./chunk.h"
 
+// ChunkID is the key used to identify entries in the Cache
+// Chunks are identified by their coordinates (CX,CZ) but a single key is needed to access a map like structure
 class ChunkID {
  public:
   ChunkID(int cx, int cz);
@@ -15,6 +17,7 @@ class ChunkID {
   int cx, cz;
 };
 
+
 class ChunkCache : public QObject {
   Q_OBJECT
 
@@ -32,8 +35,8 @@ class ChunkCache : public QObject {
   void clear();
   void setPath(QString path);
   QString getPath() const;
-  Chunk *fetch(int cx, int cz);         // fetch Chunk and load when not found
-  Chunk *fetchCached(int cx, int cz);   // fetch Chunk only if cached
+  QSharedPointer<Chunk> fetch(int cx, int cz);         // fetch Chunk and load when not found
+  QSharedPointer<Chunk> fetchCached(int cx, int cz);   // fetch Chunk only if cached
 
  signals:
   void chunkLoaded(int cx, int cz);
@@ -47,11 +50,11 @@ class ChunkCache : public QObject {
   void routeStructure(QSharedPointer<GeneratedStructure> structure);
 
  private:
-  QString path;                   // path to folder with region files
-  QCache<ChunkID, Chunk> cache;   // real Cache
-  QMutex mutex;                   // Mutex for accessing the Cache
-  int maxcache;                   // number of Chunks that fit into Cache
-  QThreadPool loaderThreadPool;   // extra thread pool for loading
+  QString path;                                   // path to folder with region files
+  QCache<ChunkID, QSharedPointer<Chunk>> cache;   // real Cache
+  QMutex mutex;                                   // Mutex for accessing the Cache
+  int maxcache;                                   // number of Chunks that fit into Cache
+  QThreadPool loaderThreadPool;                   // extra thread pool for loading
 };
 
 #endif  // CHUNKCACHE_H_
diff --git a/chunkloader.cpp b/chunkloader.cpp
@@ -4,11 +4,13 @@
 #include "./chunkcache.h"
 #include "./chunk.h"
 
+
 ChunkLoader::ChunkLoader(QString path, int cx, int cz)
   : path(path)
   , cx(cx), cz(cz)
   , cache(ChunkCache::Instance())
 {}
+
 ChunkLoader::~ChunkLoader()
 {}
 
@@ -44,7 +46,7 @@ void ChunkLoader::run() {
     return;
   }
   // get existing Chunk entry from Cache
-  Chunk *chunk = cache.fetchCached(cx, cz);
+  QSharedPointer<Chunk> chunk(cache.fetchCached(cx, cz));
   // parse Chunk data
   // Chunk will be flagged "loaded" in a thread save way
   if (chunk) {

diff --git a/chunkrenderer.cpp b/chunkrenderer.cpp
@@ -1,4 +1,4 @@
-/** Copyright (c) 2019, Mc_Etlam */
+/** Copyright (c) 2019, EtlamGit */
 
 #include "./chunk.h"
 #include "./chunkrenderer.h"
@@ -19,15 +19,15 @@ ChunkRenderer::ChunkRenderer(int cx, int cz, int y, int flags)
 
 void ChunkRenderer::run() {
   // get existing Chunk entry from Cache
-  Chunk *chunk = cache.fetchCached(cx, cz);
+  QSharedPointer<Chunk> chunk(cache.fetchCached(cx, cz));
   // render Chunk data
   if (chunk) {
     renderChunk(chunk);
   }
   emit rendered(cx, cz);
 }
 
-void ChunkRenderer::renderChunk(Chunk *chunk) {
+void ChunkRenderer::renderChunk(QSharedPointer<Chunk> chunk) {
   int offset = 0;
   uchar *bits = chunk->image;
   uchar *depthbits = chunk->depth;

diff --git a/chunkrenderer.h b/chunkrenderer.h
@@ -1,4 +1,4 @@
-/** Copyright (c) 2019, Mc_Etlam */
+/** Copyright (c) 2019, EtlamGit */
 #ifndef CHUNKRENDERER_H
 #define CHUNKRENDERER_H
 
@@ -17,7 +17,7 @@ class ChunkRenderer : public QObject, public QRunnable {
   void run();
 
  public:  // public to allow usage from WorldSave
-  void renderChunk(Chunk *chunk);
+  void renderChunk(QSharedPointer<Chunk> chunk);
 
  signals:
   void rendered(int cx, int cz);

diff --git a/entity.cpp b/entity.cpp
@@ -1,4 +1,4 @@
-/** Copyright 2014 Mc_Etlam */
+/** Copyright 2014 EtlamGit */
 #include <QPainter>
 #include "./entity.h"
 #include "./entityidentifier.h"

diff --git a/entity.h b/entity.h
@@ -1,4 +1,4 @@
-/** Copyright 2014 Mc_Etlam */
+/** Copyright 2014 EtlamGit */
 #ifndef ENTITY_H_
 #define ENTITY_H_
 

diff --git a/entityidentifier.cpp b/entityidentifier.cpp
@@ -1,4 +1,4 @@
-/** Copyright (c) 2014, Mc_Etlam */
+/** Copyright (c) 2014, EtlamGit */
 #include <QDebug>
 #include <assert.h>
 #include "./entityidentifier.h"

diff --git a/entityidentifier.h b/entityidentifier.h
@@ -1,4 +1,4 @@
-/** Copyright (c) 2014, Mc_Etlam */
+/** Copyright (c) 2014, EtlamGit */
 #ifndef ENTITYIDENTIFIER_H_
 #define ENTITYIDENTIFIER_H_
 

diff --git a/mapview.cpp b/mapview.cpp
@@ -326,7 +326,7 @@ void MapView::redraw() {
   // draw the entities
   for (int cz = startz; cz < startz + blockstall; cz++) {
     for (int cx = startx; cx < startx + blockswide; cx++) {
-      Chunk *chunk = cache.fetch(cx, cz);
+      QSharedPointer<Chunk> chunk(cache.fetch(cx, cz));
       if (chunk) {
         // Entities from Chunks
         for (auto &type : overlayItemTypes) {
@@ -373,7 +373,7 @@ void MapView::drawChunk(int x, int z) {
 
   uchar *src = placeholder;
   // fetch the chunk
-  Chunk *chunk = cache.fetch(x, z);
+  QSharedPointer<Chunk> chunk(cache.fetch(x, z));
   if (chunk && !chunk->loaded) return;
 
   if (chunk && (chunk->renderedAt != depth ||
@@ -445,7 +445,7 @@ void MapView::drawChunk(int x, int z) {
 void MapView::getToolTip(int x, int z) {
   int cx = floor(x / 16.0);
   int cz = floor(z / 16.0);
-  Chunk *chunk = cache.fetch(cx, cz);
+  QSharedPointer<Chunk> chunk(cache.fetch(cx, cz));
   int offset = (x & 0xf) + (z & 0xf) * 16;
   int y = 0;
 
@@ -544,15 +544,15 @@ void MapView::setVisibleOverlayItemTypes(const QSet<QString>& itemTypes) {
 int MapView::getY(int x, int z) {
   int cx = floor(x / 16.0);
   int cz = floor(z / 16.0);
-  Chunk *chunk = cache.fetch(cx, cz);
+  QSharedPointer<Chunk> chunk(cache.fetch(cx, cz));
   return chunk ? chunk->depth[(x & 0xf) + (z & 0xf) * 16] : -1;
 }
 
 QList<QSharedPointer<OverlayItem>> MapView::getItems(int x, int y, int z) {
   QList<QSharedPointer<OverlayItem>> ret;
   int cx = floor(x / 16.0);
   int cz = floor(z / 16.0);
-  Chunk *chunk = cache.fetch(cx, cz);
+  QSharedPointer<Chunk> chunk(cache.fetch(cx, cz));
 
   if (chunk) {
     double invzoom = 10.0 / zoom;

diff --git a/worldsave.cpp b/worldsave.cpp
@@ -126,11 +126,11 @@ void WorldSave::run() {
       } else {
         uchar *raw = f.map(coffset * 4096, numSectors * 4096);
         NBT nbt(raw);
-        Chunk *chunk = new Chunk();
+        QSharedPointer<Chunk> chunk(new Chunk());
         chunk->load(nbt);
         f.unmap(raw);
         drawChunk(scanlines, width * 4 + 1, x - left, chunk);
-        delete chunk;
+        chunk.reset();
       }
       f.close();
     }
@@ -289,7 +289,7 @@ void WorldSave::blankChunk(uchar *scanlines, int stride, int x) {
     memset(scanlines + offset, 0, 16 * 4);
 }
 
-void WorldSave::drawChunk(uchar *scanlines, int stride, int x, Chunk *chunk) {
+void WorldSave::drawChunk(uchar *scanlines, int stride, int x, QSharedPointer<Chunk> chunk) {
   // calculate attenuation
   float attenuation = 1.0f;
   if (this->regionChecker && static_cast<int>(floor(chunk->chunkX / 32.0f) +

diff --git a/worldsave.h b/worldsave.h
@@ -25,7 +25,7 @@ class WorldSave : public QObject, public QRunnable {
 
  private:
   void blankChunk(uchar *scanlines, int stride, int x);
-  void drawChunk(uchar *scanlines, int stride, int x, Chunk *chunk);
+  void drawChunk(uchar *scanlines, int stride, int x, QSharedPointer<Chunk> chunk);
 
   QString filename;
   MapView *map;