Merge pull request #30418 from mantidproject/30362_FixUnreliableFitScriptGeneratorViewTest

Fix Unreliable Access Violation when using QtPropertyBrowser

qt/widgets/common/CMakeLists.txt

@@ -1059,10 +1059,12 @@ set(
   test/FileDialogHandlerTest.h
   test/FindFilesThreadPoolManagerTest.h
   test/FindFilesWorkerTest.h
+  test/FitOptionsBrowserTest.h
   test/FitPropertyBrowserTest.h
   test/FitScriptGeneratorDataTableTest.h
   test/FitScriptGeneratorPresenterTest.h
-  # test/FitScriptGeneratorViewTest.h
+  test/FitScriptGeneratorViewTest.h
+  test/FunctionTreeViewTest.h
   test/ImageInfoModelMatrixWSTest.h
   test/ImageInfoModelMDTest.h
   test/ImageInfoPresenterTest.h

qt/widgets/common/inc/MantidQtWidgets/Common/FitOptionsBrowser.h

@@ -52,6 +52,7 @@ public:
   /// Constructor
   FitOptionsBrowser(QWidget *parent = nullptr,
                     FittingType fitType = Simultaneous);
+  ~FitOptionsBrowser();
   QString getProperty(const QString &name) const;
   void setProperty(const QString &name, const QString &value);
   void copyPropertiesToAlgorithm(Mantid::API::IAlgorithm &fit) const;

qt/widgets/common/src/FitOptionsBrowser.cpp

@@ -50,7 +50,8 @@ namespace MantidWidgets {
  * @param fitType :: The type of the underlying fitting algorithm.
  */
 FitOptionsBrowser::FitOptionsBrowser(QWidget *parent, FittingType fitType)
-    : QWidget(parent), m_decimals(6), m_fittingType(fitType) {
+    : QWidget(parent), m_fittingTypeProp(nullptr), m_minimizer(nullptr),
+      m_decimals(6), m_fittingType(fitType) {
   // create m_browser
   createBrowser();
   createProperties();
@@ -60,6 +61,14 @@ FitOptionsBrowser::FitOptionsBrowser(QWidget *parent, FittingType fitType)
   layout->setContentsMargins(0, 0, 0, 0);
 }
 
+FitOptionsBrowser::~FitOptionsBrowser() {
+  m_browser->unsetFactoryForManager(m_stringManager);
+  m_browser->unsetFactoryForManager(m_doubleManager);
+  m_browser->unsetFactoryForManager(m_intManager);
+  m_browser->unsetFactoryForManager(m_boolManager);
+  m_browser->unsetFactoryForManager(m_enumManager);
+}
+
 /**
  * Create the Qt property browser and set up property managers.
  */

qt/widgets/common/src/FunctionTreeView.cpp

@@ -99,7 +99,21 @@ FunctionTreeView::FunctionTreeView(QWidget *parent, bool multi,
 /**
  * Destructor
  */
-FunctionTreeView::~FunctionTreeView() {}
+FunctionTreeView::~FunctionTreeView() {
+  m_browser->unsetFactoryForManager(m_parameterManager);
+  m_browser->unsetFactoryForManager(m_attributeStringManager);
+  m_browser->unsetFactoryForManager(m_attributeDoubleManager);
+  m_browser->unsetFactoryForManager(m_attributeIntManager);
+  m_browser->unsetFactoryForManager(m_attributeBoolManager);
+  m_browser->unsetFactoryForManager(m_indexManager);
+  m_browser->unsetFactoryForManager(m_tieManager);
+  m_browser->unsetFactoryForManager(m_constraintManager);
+  m_browser->unsetFactoryForManager(m_filenameManager);
+  m_browser->unsetFactoryForManager(m_formulaManager);
+  m_browser->unsetFactoryForManager(m_workspaceManager);
+  m_browser->unsetFactoryForManager(m_attributeSizeManager);
+  m_browser->unsetFactoryForManager(m_attributeVectorDoubleManager);
+}
 
 /**
  * Create the Qt property browser and set up property managers.

qt/widgets/common/test/FitOptionsBrowserTest.h

+// Mantid Repository : https://github.com/mantidproject/mantid
+//
+// Copyright © 2020 ISIS Rutherford Appleton Laboratory UKRI,
+//   NScD Oak Ridge National Laboratory, European Spallation Source,
+//   Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS
+// SPDX - License - Identifier: GPL - 3.0 +
+#pragma once
+
+#include "MantidQtWidgets/Common/FitOptionsBrowser.h"
+
+#include <cxxtest/TestSuite.h>
+
+#include <memory>
+
+using namespace MantidQt::MantidWidgets;
+
+/*
+ * This test was created in response to finding an unreliable Read Access
+ * Violation when creating the FitOptionsBrowser. This failure would happen once
+ * every 100-200 attempts to instantiate this class.
+ *
+ * Its cause was a dangling pointer to a manager object being left behind when
+ * destructing a FitOptionsBrowser. This dangling point was still existing in a
+ * global static variable (m_managerToFactoryToViews or
+ * m_viewToManagerToFactory) in qtpropertybrowser.cpp. When creating a new
+ * instance of FitOptionsBrowser, the memory location would sometimes be reused,
+ * causing problems.
+
+ * The solution used to fix this was to do:
+ * m_browser->unsetFactoryForManager(m_manager)
+ * in the destructor of FitOptionsBrowser.
+ *
+ * A further issue caused by uninitialized memory was also fixed, and is covered
+ * by this test.
+ */
+
+class FitOptionsBrowserTest : public CxxTest::TestSuite {
+
+public:
+  static FitOptionsBrowserTest *createSuite() {
+    return new FitOptionsBrowserTest;
+  }
+  static void destroySuite(FitOptionsBrowserTest *suite) { delete suite; }
+
+  void setUp() override { m_numberOfTries = 100u; }
+
+  void tearDown() override { m_fitOptionsBrowser.reset(); }
+
+  void
+  test_that_the_FitOptionsBrowser_can_be_instantiated_many_times_without_instability() {
+    for (auto i = 0u; i < m_numberOfTries; ++i)
+      m_fitOptionsBrowser = std::make_unique<FitOptionsBrowser>(nullptr);
+  }
+
+private:
+  std::size_t m_numberOfTries;
+  std::unique_ptr<FitOptionsBrowser> m_fitOptionsBrowser;
+};

qt/widgets/common/test/FunctionTreeViewTest.h

+// Mantid Repository : https://github.com/mantidproject/mantid
+//
+// Copyright © 2020 ISIS Rutherford Appleton Laboratory UKRI,
+//   NScD Oak Ridge National Laboratory, European Spallation Source,
+//   Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS
+// SPDX - License - Identifier: GPL - 3.0 +
+#pragma once
+
+#include "MantidQtWidgets/Common/FunctionTreeView.h"
+
+#include <cxxtest/TestSuite.h>
+
+#include <memory>
+
+using namespace MantidQt::MantidWidgets;
+
+/*
+ * This test was created in response to finding an unreliable Read Access
+ * Violation when creating the FunctionTreeView. This failure would happen once
+ * every 100-200 attempts to instantiate this class.
+ *
+ * Its cause was a dangling pointer to a manager object being left behind when
+ * destructing a FunctionTreeView. This dangling point was still existing in a
+ * global static variable (m_managerToFactoryToViews or
+ * m_viewToManagerToFactory) in qtpropertybrowser.cpp. When creating a new
+ * instance of FunctionTreeView, the memory location would sometimes be reused,
+ * causing problems.
+
+ * The solution used to fix this was to do:
+ * m_browser->unsetFactoryForManager(m_manager)
+ * in the destructor of FunctionTreeView.
+ */
+
+class FunctionTreeViewTest : public CxxTest::TestSuite {
+
+public:
+  static FunctionTreeViewTest *createSuite() {
+    return new FunctionTreeViewTest;
+  }
+  static void destroySuite(FunctionTreeViewTest *suite) { delete suite; }
+
+  void setUp() override { m_numberOfTries = 100u; }
+
+  void tearDown() override { m_functionTreeView.reset(); }
+
+  void
+  test_that_the_FunctionTreeView_can_be_instantiated_many_times_without_instability() {
+    for (auto i = 0u; i < m_numberOfTries; ++i)
+      m_functionTreeView = std::make_unique<FunctionTreeView>(nullptr, true);
+  }
+
+private:
+  std::size_t m_numberOfTries;
+  std::unique_ptr<FunctionTreeView> m_functionTreeView;
+};