Fixed crashes resulting from the Tile Animation Editor

The TilesetModel in the TileAnimationEditor could stay around after its TilesetDocument was deleted, causing crashes while painting or layouting. Not reported as far as I know, but I saw this crash on Sentry several times per week.
mapeditor · Jul 7, 2021 · b2761f2 · b2761f2
1 parent 68a1784
commit b2761f2
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 32 deletions.
diff --git a/src/tiled/tileanimationeditor.cpp b/src/tiled/tileanimationeditor.cpp
@@ -35,9 +35,10 @@
 
 #include <QAbstractListModel>
 #include <QCloseEvent>
+#include <QMimeData>
+#include <QScopedValueRollback>
 #include <QShortcut>
 #include <QUndoStack>
-#include <QMimeData>
 
 namespace Tiled {
 
@@ -275,14 +276,8 @@ void FrameListModel::setDefaultFrameTime(int duration)
 TileAnimationEditor::TileAnimationEditor(QWidget *parent)
     : QDialog(parent, Qt::Window)
     , mUi(new Ui::TileAnimationEditor)
-    , mTilesetDocument(nullptr)
-    , mTile(nullptr)
     , mFrameListModel(new FrameListModel(this))
-    , mApplyingChanges(false)
-    , mSuppressUndo(false)
     , mPreviewAnimationDriver(new TileAnimationDriver(this))
-    , mPreviewFrameIndex(0)
-    , mPreviewUnusedTime(0)
 {
     mUi->setupUi(this);
     resize(Utils::dpiScaled(size()));
@@ -341,13 +336,19 @@ TileAnimationEditor::~TileAnimationEditor()
 
 void TileAnimationEditor::setTilesetDocument(TilesetDocument *tilesetDocument)
 {
-    if (mTilesetDocument)
+    if (mTilesetDocument) {
         mTilesetDocument->disconnect(this);
+        delete mUi->tilesetView->model();
+    }
 
+    setTile(nullptr);   // Avoid roaming pointers in FrameListModel and preview
     mTilesetDocument = tilesetDocument;
+
     mUi->tilesetView->setTilesetDocument(tilesetDocument);
 
     if (mTilesetDocument) {
+        mUi->tilesetView->setModel(new TilesetModel(mTilesetDocument, mUi->tilesetView));
+
         connect(mTilesetDocument, &TilesetDocument::tileAnimationChanged,
                 this, &TileAnimationEditor::tileAnimationChanged);
 
@@ -359,17 +360,11 @@ void TileAnimationEditor::setTilesetDocument(TilesetDocument *tilesetDocument)
 void TileAnimationEditor::setTile(Tile *tile)
 {
     mTile = tile;
-    delete mUi->tilesetView->model();
 
-    if (tile) {
+    if (tile)
         mFrameListModel->setFrames(tile->tileset(), tile->frames());
-
-        TilesetModel *tilesetModel = new TilesetModel(mTilesetDocument,
-                                                      mUi->tilesetView);
-        mUi->tilesetView->setModel(tilesetModel);
-    } else {
+    else
         mFrameListModel->setFrames(nullptr, QVector<Frame>());
-    }
 
     mUi->frameList->setEnabled(tile);
 
@@ -407,17 +402,15 @@ void TileAnimationEditor::hideEvent(QHideEvent *)
 
 void TileAnimationEditor::framesEdited()
 {
-
-    if (mSuppressUndo)
+    if (mSuppressUndo || !mTilesetDocument)
         return;
 
-    QUndoStack *undoStack = mTilesetDocument->undoStack();
+    QScopedValueRollback<bool> applyingChanges(mApplyingChanges, true);
 
-    mApplyingChanges = true;
+    QUndoStack *undoStack = mTilesetDocument->undoStack();
     undoStack->push(new ChangeTileAnimation(mTilesetDocument,
                                             mTile,
                                             mFrameListModel->frames()));
-    mApplyingChanges = false;
 }
 
 void TileAnimationEditor::setDefaultFrameTime(int duration)

diff --git a/src/tiled/tileanimationeditor.h b/src/tiled/tileanimationeditor.h
@@ -79,15 +79,15 @@ public slots:
 
     Ui::TileAnimationEditor *mUi;
 
-    TilesetDocument *mTilesetDocument;
-    Tile *mTile;
+    TilesetDocument *mTilesetDocument = nullptr;
+    Tile *mTile = nullptr;
     FrameListModel *mFrameListModel;
-    bool mApplyingChanges;
-    bool mSuppressUndo;
+    bool mApplyingChanges = false;
+    bool mSuppressUndo = false;
 
     TileAnimationDriver *mPreviewAnimationDriver;
-    int mPreviewFrameIndex;
-    int mPreviewUnusedTime;
+    int mPreviewFrameIndex = 0;
+    int mPreviewUnusedTime = 0;
 };
 
 } // namespace Tiled
diff --git a/src/tiled/tilecollisiondock.cpp b/src/tiled/tilecollisiondock.cpp
@@ -438,7 +438,6 @@ void TileCollisionDock::setTile(Tile *tile)
         mToolManager->setMapDocument(nullptr);
     }
 
-
     emit dummyMapDocumentChanged(mDummyMapDocument.data());
 
     setHasSelectedObjects(false);

diff --git a/src/tiled/tileseteditor.cpp b/src/tiled/tileseteditor.cpp
@@ -140,8 +140,6 @@ TilesetEditor::TilesetEditor(QObject *parent)
     , mZoomComboBox(new QComboBox)
     , mStatusInfoLabel(new QLabel)
     , mTileAnimationEditor(new TileAnimationEditor(mMainWindow))
-    , mCurrentTilesetDocument(nullptr)
-    , mCurrentTile(nullptr)
 {
     mMainWindow->setDockOptions(mMainWindow->dockOptions() | QMainWindow::GroupedDragging);
     mMainWindow->setDockNestingEnabled(true);

diff --git a/src/tiled/tileseteditor.h b/src/tiled/tileseteditor.h
@@ -163,9 +163,9 @@ class TilesetEditor final : public Editor
     TileAnimationEditor *mTileAnimationEditor;
 
     QHash<TilesetDocument*, TilesetView*> mViewForTileset;
-    TilesetDocument *mCurrentTilesetDocument;
+    TilesetDocument *mCurrentTilesetDocument = nullptr;
 
-    Tile *mCurrentTile;
+    Tile *mCurrentTile = nullptr;
     bool mSettingSelectedTiles = false;
 };