From f867a9e74025d66161c180c285b56c5faa31431c Mon Sep 17 00:00:00 2001
From: Antti Soininen <soininen@ill.fr>
Date: Tue, 18 Sep 2018 14:40:50 +0200
Subject: [PATCH] Add locking in WorkspaceTreeWidget fixing a segfault.

When renaming workspaces in a Python script, two threads were accessing
m_selectedNames and m_renameMap; one of them calling the clear() method
and the other keys(). To prevent the race condition there, the methods
in question now acquire a lock before any read/write operation.

Re #23513
---
 docs/source/release/v3.14.0/ui.rst            |  1 +
 .../WorkspacePresenter/WorkspaceTreeWidget.h  |  3 ++
 .../WorkspaceTreeWidget.cpp                   | 46 ++++++++++---------
 3 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/docs/source/release/v3.14.0/ui.rst b/docs/source/release/v3.14.0/ui.rst
index 60f605b9b21..f4f3393a4b7 100644
--- a/docs/source/release/v3.14.0/ui.rst
+++ b/docs/source/release/v3.14.0/ui.rst
@@ -33,5 +33,6 @@ BugFixes
 ########
 
 - Fixed issue where an open set of data from ITableWorkspace wouldn't update if the data was changed via python
+- Fixed an issue where MantidPlot would crash when renaming workspaces.
 
 :ref:`Release 3.14.0 <v3.14.0>`
diff --git a/qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidget.h b/qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidget.h
index 5a678f60a51..4b18414ace9 100644
--- a/qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidget.h
+++ b/qt/widgets/common/inc/MantidQtWidgets/Common/WorkspacePresenter/WorkspaceTreeWidget.h
@@ -18,6 +18,7 @@
 #include <QHash>
 #include <QMap>
 #include <QMetaType>
+#include <QMutex>
 #include <boost/shared_ptr.hpp>
 #include <map>
 
@@ -278,6 +279,8 @@ private:
   QStringList m_selectedNames;
   /// Keep a map of renamed workspaces between updates
   QHash<QString, QString> m_renameMap;
+  /// A mutex to lock m_renameMap and m_selectedNames for reading/writing
+  mutable QMutex m_mutex;
 
 private slots:
   void handleUpdateTree(const TopLevelItems &);
diff --git a/qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidget.cpp b/qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidget.cpp
index 3eec3ff7c68..0fcfb2e5959 100644
--- a/qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidget.cpp
+++ b/qt/widgets/common/src/WorkspacePresenter/WorkspaceTreeWidget.cpp
@@ -50,7 +50,7 @@ WorkspaceTreeWidget::WorkspaceTreeWidget(MantidDisplayBase *mdb,
     : QWidget(parent), m_mantidDisplayModel(mdb), m_updateCount(0),
       m_treeUpdating(false), m_promptDelete(false),
       m_saveFileType(SaveFileType::Nexus), m_sortCriteria(SortCriteria::ByName),
-      m_sortDirection(SortDirection::Ascending) {
+      m_sortDirection(SortDirection::Ascending), m_mutex(QMutex::Recursive) {
   setObjectName(
       "exploreMantid"); // this is needed for QMainWindow::restoreState()
   m_saveMenu = new QMenu(this);
@@ -277,6 +277,7 @@ void WorkspaceTreeWidget::recordWorkspaceRename(const std::string &oldName,
   QString qs_oldName = QString::fromStdString(oldName);
   QString qs_newName = QString::fromStdString(newName);
 
+  QMutexLocker renameMapLock(&m_mutex);
   // check if old_name has been recently a new name
   QList<QString> oldNames = m_renameMap.keys(qs_oldName);
   // non-empty list of oldNames become new_name
@@ -806,28 +807,30 @@ void WorkspaceTreeWidget::updateTree(const TopLevelItems &items) {
  */
 void WorkspaceTreeWidget::populateTopLevel(const TopLevelItems &topLevelItems,
                                            const QStringList &expanded) {
-  // collect names of selected workspaces
-  QList<QTreeWidgetItem *> selected = m_tree->selectedItems();
-  m_selectedNames.clear(); // just in case
-  foreach (QTreeWidgetItem *item, selected) {
-    m_selectedNames << item->text(0);
-  }
+  {
+    QMutexLocker lock(&m_mutex);
+    // collect names of selected workspaces
+    QList<QTreeWidgetItem *> selected = m_tree->selectedItems();
+    m_selectedNames.clear(); // just in case
+    foreach (QTreeWidgetItem *item, selected) {
+      m_selectedNames << item->text(0);
+    }
 
-  // populate the tree from scratch
-  m_tree->clear();
-  auto iend = topLevelItems.end();
-  for (auto it = topLevelItems.begin(); it != iend; ++it) {
-    auto *node = addTreeEntry(*it);
-    QString name = node->text(0);
-    if (expanded.contains(name))
-      node->setExpanded(true);
-    // see if item must be selected
-    if (shouldBeSelected(name))
-      node->setSelected(true);
+    // populate the tree from scratch
+    m_tree->clear();
+    auto iend = topLevelItems.end();
+    for (auto it = topLevelItems.begin(); it != iend; ++it) {
+      auto *node = addTreeEntry(*it);
+      QString name = node->text(0);
+      if (expanded.contains(name))
+        node->setExpanded(true);
+      // see if item must be selected
+      if (shouldBeSelected(name))
+        node->setSelected(true);
+    }
+    m_selectedNames.clear();
+    m_renameMap.clear();
   }
-  m_selectedNames.clear();
-  m_renameMap.clear();
-
   // apply any filtering
   filterWorkspaceTree(m_workspaceFilter->text());
 }
@@ -868,6 +871,7 @@ MantidTreeWidgetItem *WorkspaceTreeWidget::addTreeEntry(
  * @param name :: Name of a workspace to check.
  */
 bool WorkspaceTreeWidget::shouldBeSelected(QString name) const {
+  QMutexLocker lock(&m_mutex);
   QStringList renamed = m_renameMap.keys(name);
   if (!renamed.isEmpty()) {
     foreach (QString oldName, renamed) {
-- 
GitLab