#5127: Fix crashes when destroying Populator objects deriving from Th…

…readedResourceTreePopulator and introducing their own members.

Due to C++ dtor execution order threads will crash when the base class ThreadedResourceTreePopulator calls Delete().
The 100% solution would be to disallow subclassing from wxThread, but I'd like to keep the abstraction and the code re-use - therefore deriving classes must invoke EnsureStopped in their destructors. If they fail to do so, there's another safety measure in ResourceTreeView to call EnsureStopped before releasing the instance, to give classes a chance to clean up while the hierarchy is still intact.

libs/wxutil/dataview/IResourceTreePopulator.h

@@ -29,6 +29,10 @@ class IResourceTreePopulator
     // This might spawn a worker thread which performs the
     // population in the background - use EnsurePopulated to synchronise.
     virtual void Populate() = 0;
+
+    // In threaded implementations this method should cancel the
+    // async method and block until it's finished
+    virtual void EnsureStopped() = 0;
 };
 
 }

libs/wxutil/dataview/ResourceTreeView.cpp

@@ -39,6 +39,20 @@ ResourceTreeView::ResourceTreeView(wxWindow* parent, const TreeModel::Ptr& model
     AssociateModel(_treeStore.get());
 
     Bind(wxEVT_DATAVIEW_ITEM_CONTEXT_MENU, &ResourceTreeView::_onContextMenu, this);
+    Bind(EV_TREEMODEL_POPULATION_FINISHED, &ResourceTreeView::_onTreeStorePopulationFinished, this);
+}
+
+ResourceTreeView::~ResourceTreeView()
+{
+    if (_populator)
+    {
+        // This call isn't strictly necessary if the implementing thread
+        // populators are equipped with proper destructors.
+        // In case some user code is missing such a CancelAndWait call in their
+        // destructor, let's call it here while the object is still intact.
+        _populator->EnsureStopped();
+        _populator.reset();
+    }
 }
 
 const TreeModel::Ptr& ResourceTreeView::getTreeModel()
@@ -155,6 +169,12 @@ std::string ResourceTreeView::getSelection()
 
 void ResourceTreeView::setSelection(const std::string& fullName)
 {
+    // Wait for any running populator
+    if (_populator)
+    {
+        _populator->EnsurePopulated();
+    }
+
     // If the selection string is empty, collapse the treeview and return with
     // no selection
     if (fullName.empty())
@@ -179,11 +199,39 @@ void ResourceTreeView::setSelection(const std::string& fullName)
 
 void ResourceTreeView::clear()
 {
-    // Clear the media browser on MaterialManager unrealisation
+    // Clear any data and/or active population objects
+    _populator.reset();
     _treeStore->Clear();
     _emptyFavouritesLabel = wxDataViewItem();
 }
 
+void ResourceTreeView::populate(const IResourceTreePopulator::Ptr& populator)
+{
+    // Remove any data or running populators first
+    clear();
+
+    // Add the loading icon to the tree
+    TreeModel::Row row = getTreeModel()->AddItem();
+
+    row[_columns.iconAndName] = wxVariant(wxDataViewIconText(_("Loading resources...")));
+    row[_columns.isFavourite] = true;
+    row[_columns.isFolder] = false;
+
+    row.SendItemAdded();
+
+    // Start population (this might be a thread or not)
+    // In any case this will fire the TreeModel::PopulationFinishedEvent when done
+    _populator = populator;
+    _populator->Populate();
+}
+
+void ResourceTreeView::_onTreeStorePopulationFinished(TreeModel::PopulationFinishedEvent& ev)
+{
+    UnselectAll();
+    setTreeModel(ev.GetTreeModel());
+    _populator.reset();
+}
+
 void ResourceTreeView::_onContextMenu(wxDataViewEvent& ev)
 {
     if (!_popupMenu)

libs/wxutil/dataview/ResourceTreeView.h

@@ -3,6 +3,7 @@
 #include "TreeView.h"
 #include "TreeModel.h"
 #include "TreeModelFilter.h"
+#include "IResourceTreePopulator.h"
 #include "../menu/PopupMenu.h"
 
 namespace wxutil
@@ -63,10 +64,15 @@ class ResourceTreeView :
     TreeModelFilter::Ptr _treeModelFilter;
     wxDataViewItem _emptyFavouritesLabel;
 
+    // The currently active populator object
+    IResourceTreePopulator::Ptr _populator;
+
 public:
     ResourceTreeView(wxWindow* parent, const Columns& columns, long style = wxDV_SINGLE);
     ResourceTreeView(wxWindow* parent, const TreeModel::Ptr& model, const Columns& columns, long style = wxDV_SINGLE);
 
+    virtual ~ResourceTreeView();
+
     // Returns a reference to the model we're using
     virtual const TreeModel::Ptr& getTreeModel();
     virtual void setTreeModel(const TreeModel::Ptr& treeModel);
@@ -83,6 +89,9 @@ class ResourceTreeView :
     virtual bool isDirectorySelected();
     virtual bool isFavouriteSelected();
 
+    // Populate this tree using the given populator object
+    virtual void populate(const IResourceTreePopulator::Ptr& populator);
+
 protected:
     virtual void populateContextMenu(wxutil::PopupMenu& popupMenu);
 
@@ -92,6 +101,7 @@ class ResourceTreeView :
 
 private:
     void _onContextMenu(wxDataViewEvent& ev);
+    void _onTreeStorePopulationFinished(TreeModel::PopulationFinishedEvent& ev);
 
     bool _testAddToFavourites();
     bool _testRemoveFromFavourites();

libs/wxutil/dataview/ThreadedResourceTreePopulator.h

@@ -15,6 +15,10 @@ namespace wxutil
  * 
  * At the end of the thread execution this populator will send
  *  a wxutil::TreeModel::PopulationFinishedEvent to the handler.
+ * 
+ * Note: if a subclass is introducing additional class members
+ * that are used in the PopulateModel/SortModel methods, it's mandatory
+ * for the subclass destructor to call EnsureStopped().
  */
 class ThreadedResourceTreePopulator :
     public IResourceTreePopulator,
@@ -77,14 +81,15 @@ class ThreadedResourceTreePopulator :
 
     virtual ~ThreadedResourceTreePopulator()
     {
-        if (IsAlive())
-        {
-            Delete(); // cancel the running thread
-        }
+        // When running into crashes with a calling thread waiting for this
+        // method, this might be due to the deriving class methods referencing
+        // members that have already been destructed at this point.
+        // Be sure to call EnsureStopped() in the subclass destructor.
+        EnsureStopped();
     }
 
     // Blocks until the worker thread is done. 
-    void EnsurePopulated() override
+    virtual void EnsurePopulated() override
     {
         // Start the thread now if we have to
         if (!_started)
@@ -100,7 +105,7 @@ class ThreadedResourceTreePopulator :
     }
 
     // Start the thread, if it isn't already running
-    void Populate() override
+    virtual void Populate() override
     {
         if (IsRunning())
         {
@@ -111,6 +116,14 @@ class ThreadedResourceTreePopulator :
         _started = true;
         wxThread::Run();
     }
+
+    virtual void EnsureStopped() override
+    {
+        if (IsAlive())
+        {
+            Delete(); // cancel the running thread
+        }
+    }
 };
 
 }

radiant/ui/mediabrowser/MediaBrowserTreeView.cpp

@@ -161,7 +161,7 @@ struct ShaderNameFunctor
     }
 };
 
-class Populator :
+class MediaPopulator final :
     public wxutil::ThreadedResourceTreePopulator
 {
 private:
@@ -172,13 +172,19 @@ class Populator :
 
 public:
     // Construct and initialise variables
-    Populator(const MediaBrowserTreeView::TreeColumns& columns, wxEvtHandler* finishedHandler) :
+    MediaPopulator(const MediaBrowserTreeView::TreeColumns& columns, wxEvtHandler* finishedHandler) :
         ThreadedResourceTreePopulator(columns, finishedHandler),
         _columns(columns)
     {
         _favourites = GlobalFavouritesManager().getFavourites(decl::Type::Material);
     }
 
+    ~MediaPopulator()
+    {
+        // Stop the worker while the class is still intact
+        EnsureStopped();
+    }
+
 protected:
     void PopulateModel(const wxutil::TreeModel::Ptr& model) override
     {
@@ -259,8 +265,7 @@ class Populator :
 };
 
 MediaBrowserTreeView::MediaBrowserTreeView(wxWindow* parent) :
-    ResourceTreeView(parent, _columns, wxDV_NO_HEADER),
-    _isPopulated(false)
+    ResourceTreeView(parent, _columns, wxDV_NO_HEADER)
 {
     auto* textCol = AppendIconTextColumn(_("Shader"), _columns.iconAndName.getColumnIndex(),
         wxDATAVIEW_CELL_INERT, wxCOL_WIDTH_AUTOSIZE);
@@ -275,7 +280,6 @@ MediaBrowserTreeView::MediaBrowserTreeView(wxWindow* parent) :
     getTreeModel()->SetHasDefaultCompare(false);
 
     Bind(wxEVT_DATAVIEW_ITEM_ACTIVATED, &MediaBrowserTreeView::_onTreeViewItemActivated, this);
-    Bind(wxutil::EV_TREEMODEL_POPULATION_FINISHED, &MediaBrowserTreeView::_onTreeStorePopulationFinished, this);
 }
 
 const MediaBrowserTreeView::TreeColumns& MediaBrowserTreeView::getColumns() const
@@ -295,50 +299,7 @@ void MediaBrowserTreeView::setTreeMode(MediaBrowserTreeView::TreeMode mode)
 
 void MediaBrowserTreeView::populate()
 {
-    if (_isPopulated) return;
-
-    // Clear our treestore and put a single item in it
-    clear();
-
-    // Set the flag to true to avoid double-entering this function
-    _isPopulated = true;
-
-    wxutil::TreeModel::Row row = getTreeModel()->AddItem();
-
-    wxIcon icon;
-    icon.CopyFromBitmap(wxArtProvider::GetBitmap(GlobalUIManager().ArtIdPrefix() + TEXTURE_ICON));
-    row[_columns.iconAndName] = wxVariant(wxDataViewIconText(_("Loading, please wait..."), icon));
-    row[_columns.isFavourite] = true;
-    row[_columns.isFolder] = false;
-
-    row.SendItemAdded();
-
-    // Start the background thread
-    _populator.reset(new Populator(_columns, this));
-    _populator->Populate();
-}
-
-void MediaBrowserTreeView::clear()
-{
-    // Stop any populator thread that might be running
-    _populator.reset();
-    _isPopulated = false;
-
-    ResourceTreeView::clear();
-}
-
-void MediaBrowserTreeView::_onTreeStorePopulationFinished(wxutil::TreeModel::PopulationFinishedEvent& ev)
-{
-    UnselectAll();
-    setTreeModel(ev.GetTreeModel());
-}
-
-void MediaBrowserTreeView::setSelection(const std::string& fullName)
-{
-    // Make sure the treestore is loaded
-    _populator->EnsurePopulated();
-
-    ResourceTreeView::setSelection(fullName);
+    ResourceTreeView::populate(std::make_shared<MediaPopulator>(_columns, this));
 }
 
 void MediaBrowserTreeView::populateContextMenu(wxutil::PopupMenu& popupMenu)

radiant/ui/mediabrowser/MediaBrowserTreeView.h

@@ -1,7 +1,6 @@
 #pragma once
 
 #include "wxutil/dataview/ResourceTreeView.h"
-#include "wxutil/dataview/IResourceTreePopulator.h"
 #include "wxutil/menu/IconTextMenuItem.h"
 #include "wxutil/dataview/TreeModelFilter.h"
 
@@ -24,27 +23,16 @@ class MediaBrowserTreeView :
     };
 
 private:
-    // Populates the Media Browser in its own thread
-    std::unique_ptr<wxutil::IResourceTreePopulator> _populator;
-
-    // false, if the tree is not yet initialised.
-    bool _isPopulated;
-
     MediaBrowserTreeView::TreeColumns _columns;
 
 public:
     MediaBrowserTreeView(wxWindow* parent);
 
     const TreeColumns& getColumns() const;
 
-    void setSelection(const std::string& fullName) override;
-
-    // Clear all items, stop any populator thread
-    void clear() override;
-
     void setTreeMode(TreeMode mode) override;
 
-    // Populates the treeview
+    // Loads all the materials
     void populate();
 
 protected:
@@ -58,7 +46,6 @@ class MediaBrowserTreeView :
     void _onLoadInTexView();
     void _onSelectItems(bool select);
     void _onTreeViewItemActivated(wxDataViewEvent& ev);
-    void _onTreeStorePopulationFinished(wxutil::TreeModel::PopulationFinishedEvent& ev);
 };
 
 }