Mitigate crashes during preflight by disconnecting signals/slots. (Bl…

…ueQuartzSoftware#314) Also use an unlimited undo stack size. The AddFilterCommand or the RemoveFilterCommand are part of the UndoStack and are created every time a filter is created or removed from the pipeline. The signal/slot was hooked up using a lambda as the target of the slot. The QUndoStack max undo size was set to 10. After the 10th filter was added to the pipeline, attempting to set a value in the first filter input widget, which would cause a preflight would crash the pipeline. This is because when the 11th filter is added the first "AddFilterCommand" object gets removed from the stack and destroyed. The signal/slots were never disconnected before the object is destroyed. This repercussion is that a signal/slot connection is still going to be executed on a piece of memory will get corrupted by being written over with new allocations as part of the program. The implemented solution is 2 fold: During the destructor of the AddFilterCommand and RemoveFilterCommand we call the disconnectSignalsSlots() function. We also use a QMetaObject::Connection object to store the connection to the lambda and then disconnect the QMetaObject::Connection during the destructor. We also now use an unlimited undo stack. Updates BlueQuartzSoftware#313 Closes BlueQuartzSoftware#313 Fixes BlueQuartzSoftware#313 Signed-off-by: Michael Jackson <[email protected]>
imikejackson · Feb 10, 2019 · 4f1ddcf · 4f1ddcf
1 parent e4eaca5
commit 4f1ddcf
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 35 deletions.
diff --git a/Source/SVWidgetsLib/Widgets/PipelineListWidget.cpp b/Source/SVWidgetsLib/Widgets/PipelineListWidget.cpp
@@ -95,9 +95,9 @@ void PipelineListWidget::on_startPipelineBtn_clicked()
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void PipelineListWidget::preflightFinished(FilterPipeline::Pointer pipeline, int err)
+void PipelineListWidget::preflightFinished(int pipelineFilterCount, int err)
 {
-  if(err >= 0 && !pipeline->getFilterContainer().empty())
+  if(err >= 0 && pipelineFilterCount > 0)
   {
     startPipelineBtn->setEnabled(true);
   }

diff --git a/Source/SVWidgetsLib/Widgets/PipelineListWidget.h b/Source/SVWidgetsLib/Widgets/PipelineListWidget.h
@@ -64,7 +64,7 @@ class SVWidgetsLib_EXPORT PipelineListWidget : public QFrame, private Ui::Pipeli
     /**
      * @brief preflightFinished
      */
-    void preflightFinished(FilterPipeline::Pointer pipeline, int err);
+    void preflightFinished(int pipelineFilterCount, int err);
 
     /**
      * @brief pipelineFinished

diff --git a/Source/SVWidgetsLib/Widgets/PipelineView.cpp b/Source/SVWidgetsLib/Widgets/PipelineView.cpp
@@ -66,7 +66,7 @@ void PipelineView::addUndoCommand(QUndoCommand* cmd)
 void PipelineView::setupUndoStack()
 {
   m_UndoStack = QSharedPointer<QUndoStack>(new QUndoStack());
-  m_UndoStack->setUndoLimit(10);
+ // m_UndoStack->setUndoLimit(4);
 
   m_ActionUndo = m_UndoStack->createUndoAction(m_UndoStack.data());
   m_ActionRedo = m_UndoStack->createRedoAction(m_UndoStack.data());

diff --git a/Source/SVWidgetsLib/Widgets/SVPipelineView.cpp b/Source/SVWidgetsLib/Widgets/SVPipelineView.cpp
@@ -426,7 +426,7 @@ void SVPipelineView::preflightPipeline()
     }
   }
 
-  emit preflightFinished(pipeline, err);
+  emit preflightFinished(count, err);
   updateFilterInputWidgetIndices();
   //qDebug() << "----------- SVPipelineView::preflightPipeline End --------------";
 

diff --git a/Source/SVWidgetsLib/Widgets/SVPipelineView.h b/Source/SVWidgetsLib/Widgets/SVPipelineView.h
@@ -339,7 +339,7 @@ public slots:
   void addPlaceHolderFilter(QPoint p);
   void removePlaceHolderFilter();
 
-  void preflightFinished(FilterPipeline::Pointer pipeline, int err);
+  void preflightFinished(int32_t pipelineFilterCount, int err);
 
   void filterParametersChanged(AbstractFilter::Pointer filter);
 

diff --git a/Source/SVWidgetsLib/Widgets/util/AddFilterCommand.cpp b/Source/SVWidgetsLib/Widgets/util/AddFilterCommand.cpp
@@ -110,7 +110,15 @@ AddFilterCommand::AddFilterCommand(std::vector<AbstractFilter::Pointer> filters,
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-AddFilterCommand::~AddFilterCommand() = default;
+AddFilterCommand::~AddFilterCommand()
+{
+  for(const auto& filter : m_Filters)
+  {
+   // qDebug() << "~AddFilterCommand(): ";
+   // if(nullptr != filter.get()) { qDebug() << filter->getNameOfClass(); }
+    disconnectFilterSignalsSlots(filter);
+  }
+}
 
 // -----------------------------------------------------------------------------
 //
@@ -196,7 +204,7 @@ void AddFilterCommand::redo()
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void AddFilterCommand::addFilter(AbstractFilter::Pointer filter, int insertionIndex)
+void AddFilterCommand::addFilter(const AbstractFilter::Pointer &filter, int insertionIndex)
 {
   // Reset the filter's Removing property
   filter->setRemoving(false);
@@ -264,7 +272,7 @@ void AddFilterCommand::removeFilter(const QPersistentModelIndex& index)
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void AddFilterCommand::connectFilterSignalsSlots(AbstractFilter::Pointer filter)
+void AddFilterCommand::connectFilterSignalsSlots(const AbstractFilter::Pointer& filter)
 {
   PipelineModel* model = m_PipelineView->getPipelineModel();
   QModelIndex index = model->indexOfFilter(filter.get());
@@ -275,7 +283,7 @@ void AddFilterCommand::connectFilterSignalsSlots(AbstractFilter::Pointer filter)
 
   FilterInputWidget* fiw = model->filterInputWidget(index);
 
-  QObject::connect(fiw, &FilterInputWidget::filterParametersChanged, [=] (bool preflight) {
+  m_connection = QObject::connect(fiw, &FilterInputWidget::filterParametersChanged, [=] (bool preflight) {
     if (preflight)
     {
       m_PipelineView->preflightPipeline();
@@ -287,16 +295,15 @@ void AddFilterCommand::connectFilterSignalsSlots(AbstractFilter::Pointer filter)
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void AddFilterCommand::disconnectFilterSignalsSlots(AbstractFilter::Pointer filter)
+void AddFilterCommand::disconnectFilterSignalsSlots(const AbstractFilter::Pointer& filter)
 {
-  PipelineModel* model = m_PipelineView->getPipelineModel();
-  QModelIndex index = model->indexOfFilter(filter.get());
+  if(filter.get() == nullptr)
+  {
+    return;
+  }
+ // PipelineModel* model = m_PipelineView->getPipelineModel();
 
   QObject::disconnect(filter.get(), &AbstractFilter::filterCompleted, nullptr, nullptr);
 
   QObject::disconnect(filter.get(), &AbstractFilter::filterInProgress, nullptr, nullptr);
-
-  FilterInputWidget* fiw = model->filterInputWidget(index);
-
-  QObject::disconnect(fiw, &FilterInputWidget::filterParametersChanged, nullptr, nullptr);
 }
diff --git a/Source/SVWidgetsLib/Widgets/util/AddFilterCommand.h b/Source/SVWidgetsLib/Widgets/util/AddFilterCommand.h
@@ -67,13 +67,14 @@ class SVWidgetsLib_EXPORT AddFilterCommand : public QUndoCommand
   std::vector<int> m_FilterRows;
   bool m_FirstRun = true;
   bool m_UseAnimationOnFirstRun;
+  QMetaObject::Connection m_connection;
 
   /**
    * @brief addFilter
    * @param filter
    * @param parentIndex
    */
-  void addFilter(AbstractFilter::Pointer filter, int insertionIndex = -1);
+  void addFilter(const AbstractFilter::Pointer& filter, int insertionIndex = -1);
 
   /**
    * @brief removeFilter
@@ -86,13 +87,13 @@ class SVWidgetsLib_EXPORT AddFilterCommand : public QUndoCommand
    * @brief connectFilterSignalsSlots
    * @param filter
    */
-  void connectFilterSignalsSlots(AbstractFilter::Pointer filter);
+  void connectFilterSignalsSlots(const AbstractFilter::Pointer& filter);
 
   /**
    * @brief disconnectFilterSignalsSlots
    * @param filter
    */
-  void disconnectFilterSignalsSlots(AbstractFilter::Pointer filter);
+  void disconnectFilterSignalsSlots(const AbstractFilter::Pointer &filter);
 
 public:
   AddFilterCommand(const AddFilterCommand&) = delete; // Copy Constructor Not Implemented

diff --git a/Source/SVWidgetsLib/Widgets/util/RemoveFilterCommand.cpp b/Source/SVWidgetsLib/Widgets/util/RemoveFilterCommand.cpp
@@ -92,7 +92,15 @@ RemoveFilterCommand::RemoveFilterCommand(std::vector<AbstractFilter::Pointer> fi
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-RemoveFilterCommand::~RemoveFilterCommand() = default;
+RemoveFilterCommand::~RemoveFilterCommand()
+{
+  for(const auto& filter : m_Filters)
+  {
+ //   qDebug() << "~RemoveFilterCommand(): ";
+ //   if(nullptr != filter.get()) { qDebug() << filter->getNameOfClass(); }
+    disconnectFilterSignalsSlots(filter);
+  }
+}
 
 // -----------------------------------------------------------------------------
 //
@@ -186,7 +194,7 @@ void RemoveFilterCommand::redo()
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void RemoveFilterCommand::addFilter(AbstractFilter::Pointer filter, int insertionIndex)
+void RemoveFilterCommand::addFilter(const AbstractFilter::Pointer& filter, int insertionIndex)
 {
   filter->setRemoving(false);
 
@@ -217,7 +225,7 @@ void RemoveFilterCommand::addFilter(AbstractFilter::Pointer filter, int insertio
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void RemoveFilterCommand::removeFilter(AbstractFilter::Pointer filter)
+void RemoveFilterCommand::removeFilter(const AbstractFilter::Pointer& filter)
 {
   // Check if the given filter is already being removed before removing it again
   // Multiple calls to remove the same object causes crashes.
@@ -254,7 +262,7 @@ void RemoveFilterCommand::removeFilter(AbstractFilter::Pointer filter)
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void RemoveFilterCommand::connectFilterSignalsSlots(AbstractFilter::Pointer filter)
+void RemoveFilterCommand::connectFilterSignalsSlots(const AbstractFilter::Pointer& filter)
 {
   PipelineModel* model = m_PipelineView->getPipelineModel();
   QModelIndex index = model->indexOfFilter(filter.get());
@@ -277,16 +285,16 @@ void RemoveFilterCommand::connectFilterSignalsSlots(AbstractFilter::Pointer filt
 // -----------------------------------------------------------------------------
 //
 // -----------------------------------------------------------------------------
-void RemoveFilterCommand::disconnectFilterSignalsSlots(AbstractFilter::Pointer filter)
+void RemoveFilterCommand::disconnectFilterSignalsSlots(const AbstractFilter::Pointer &filter)
 {
-  PipelineModel* model = m_PipelineView->getPipelineModel();
-  QModelIndex index = model->indexOfFilter(filter.get());
+  if(filter.get() == nullptr)
+  {
+    return;
+  }
+  //PipelineModel* model = m_PipelineView->getPipelineModel();
 
   QObject::disconnect(filter.get(), &AbstractFilter::filterCompleted, nullptr, nullptr);
 
   QObject::disconnect(filter.get(), &AbstractFilter::filterInProgress, nullptr, nullptr);
-
-  FilterInputWidget* fiw = model->filterInputWidget(index);
-
-  QObject::disconnect(fiw, &FilterInputWidget::filterParametersChanged, nullptr, nullptr);
+  QObject::disconnect( m_connection );
 }
diff --git a/Source/SVWidgetsLib/Widgets/util/RemoveFilterCommand.h b/Source/SVWidgetsLib/Widgets/util/RemoveFilterCommand.h
@@ -65,31 +65,32 @@ class SVWidgetsLib_EXPORT RemoveFilterCommand : public QUndoCommand
   std::vector<int> m_FilterRows;
   bool m_FirstRun = true;
   bool m_UseAnimationOnFirstRun = true;
+  QMetaObject::Connection m_connection;
 
   /**
    * @brief addFilter
    * @param filter
    * @param insertionIndex
    */
-  void addFilter(AbstractFilter::Pointer filter, int insertionIndex = -1);
+  void addFilter(const AbstractFilter::Pointer &filter, int insertionIndex = -1);
 
   /**
    * @brief removeFilter
    * @param row
    */
-  void removeFilter(AbstractFilter::Pointer filter);
+  void removeFilter(const AbstractFilter::Pointer& filter);
 
   /**
    * @brief connectFilterSignalsSlots
    * @param filter
    */
-  void connectFilterSignalsSlots(AbstractFilter::Pointer filter);
+  void connectFilterSignalsSlots(const AbstractFilter::Pointer& filter);
 
   /**
    * @brief disconnectFilterSignalsSlots
    * @param filter
    */
-  void disconnectFilterSignalsSlots(AbstractFilter::Pointer filter);
+  void disconnectFilterSignalsSlots(const AbstractFilter::Pointer& filter);
 
 public:
   RemoveFilterCommand(const RemoveFilterCommand&) = delete; // Copy Constructor Not Implemented