Fix segfault at exit (#1317) (#1441)

As discussed in #1158, a segfault occurs during exit because Views created as a result of an ECM query by plugins have their destructors stored in the shared library of the plugin. For GUI plugins, the plugins are unloaded from memory before the ECM is destructed, so when it's time to destruct the Views, a segfault occurs because the pointer to the virtual destructor is invalid. This PR is fixes the problem by making View a regular class instead of a template. This ensures that the destructor of View is stored in the core library of ignition-gazebo. As a result, the ECM can be destructed after GUI plugins have been unloaded.

Signed-off-by: Addisu Z. Taddese <[email protected]>

Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Ashton Larkin <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>

Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Ashton Larkin <[email protected]>

Author: 3 people

include/ignition/gazebo/EntityComponentManager.hh

@@ -773,7 +773,7 @@ namespace ignition
       /// \tparam ComponentTypeTs All the component types that define a view.
       /// \return A pointer to the view.
       private: template<typename ...ComponentTypeTs>
-          detail::View<ComponentTypeTs...> *FindView() const;
+          detail::View *FindView() const;
 
       /// \brief Find a view based on the provided component type ids.
       /// \param[in] _types The component type ids that serve as a key into

include/ignition/gazebo/detail/BaseView.hh

@@ -18,7 +18,6 @@
 #define IGNITION_GAZEBO_DETAIL_BASEVIEW_HH_
 
 #include <cstddef>
-#include <memory>
 #include <set>
 #include <unordered_map>
 #include <vector>
@@ -69,7 +68,7 @@ struct ComponentTypeHasher
 class IGNITION_GAZEBO_VISIBLE BaseView
 {
   /// \brief Destructor
-  public: virtual ~BaseView() = default;
+  public: virtual ~BaseView();
 
   /// \brief See if an entity is a part of the view
   /// \param[in] _entity The entity

include/ignition/gazebo/detail/EntityComponentManager.hh

@@ -372,6 +372,50 @@ void EntityComponentManager::EachNoCache(typename identity<std::function<
   }
 }
 
+namespace detail
+{
+/// \brief Helper template to call a callback function with each of the
+/// components in the _data vector expanded as arguments to the callback
+/// function.
+/// \tparam ComponentTypeTs The actual types of each of the components.
+/// \tparam FuncT The type of the callback function.
+/// \tparam BaseComponentT Either "BaseComponent" or "const BaseComponent"
+/// \tparam Is Index sequence that will be used to iterate through the vector
+/// _data.
+/// \param[in] _f The callback function
+/// \param[in] _entity The entity associated with the components.
+/// \param[in] _data A vector of component pointers that will be expanded to
+/// become the arguments of the callback function _f.
+/// \return The value of return by the function _f.
+template <typename... ComponentTypeTs, typename FuncT, typename BaseComponentT,
+          std::size_t... Is>
+constexpr bool applyFunctionImpl(const FuncT &_f, const Entity &_entity,
+                       const std::vector<BaseComponentT *> &_data,
+                       std::index_sequence<Is...>)
+{
+  return _f(_entity, static_cast<ComponentTypeTs *>(_data[Is])...);
+}
+
+/// \brief Helper template to call a callback function with each of the
+/// components in the _data vector expanded as arguments to the callback
+/// function.
+/// \tparam ComponentTypeTs The actual types of each of the components.
+/// \tparam FuncT The type of the callback function.
+/// \tparam BaseComponentT Either "BaseComponent" or "const BaseComponent"
+/// \param[in] _f The callback function
+/// \param[in] _entity The entity associated with the components.
+/// \param[in] _data A vector of component pointers that will be expanded to
+/// become the arguments of the callback function _f.
+/// \return The value of return by the function _f.
+template <typename... ComponentTypeTs, typename FuncT, typename BaseComponentT>
+constexpr bool applyFunction(const FuncT &_f, const Entity &_entity,
+                   const std::vector<BaseComponentT *> &_data)
+{
+  return applyFunctionImpl<ComponentTypeTs...>(
+      _f, _entity, _data, std::index_sequence_for<ComponentTypeTs...>{});
+}
+}  // namespace detail
+
 //////////////////////////////////////////////////
 template<typename ...ComponentTypeTs>
 void EntityComponentManager::Each(typename identity<std::function<
@@ -385,7 +429,8 @@ void EntityComponentManager::Each(typename identity<std::function<
   // function.
   for (const Entity entity : view->Entities())
   {
-    if (!std::apply(_f, view->EntityComponentConstData(entity)))
+    const auto &data = view->EntityComponentData(entity);
+    if (!detail::applyFunction<const ComponentTypeTs...>(_f, entity, data))
     {
       break;
     }
@@ -405,7 +450,8 @@ void EntityComponentManager::Each(typename identity<std::function<
   // function.
   for (const Entity entity : view->Entities())
   {
-    if (!std::apply(_f, view->EntityComponentData(entity)))
+    const auto &data = view->EntityComponentData(entity);
+    if (!detail::applyFunction<ComponentTypeTs...>(_f, entity, data))
     {
       break;
     }
@@ -434,7 +480,8 @@ void EntityComponentManager::EachNew(typename identity<std::function<
   // function.
   for (const Entity entity : view->NewEntities())
   {
-    if (!std::apply(_f, view->EntityComponentData(entity)))
+    const auto &data = view->EntityComponentData(entity);
+    if (!detail::applyFunction<ComponentTypeTs...>(_f, entity, data))
     {
       break;
     }
@@ -455,7 +502,8 @@ void EntityComponentManager::EachNew(typename identity<std::function<
   // function.
   for (const Entity entity : view->NewEntities())
   {
-    if (!std::apply(_f, view->EntityComponentConstData(entity)))
+    const auto &data = view->EntityComponentData(entity);
+    if (!detail::applyFunction<const ComponentTypeTs...>(_f, entity, data))
     {
       break;
     }
@@ -476,7 +524,8 @@ void EntityComponentManager::EachRemoved(typename identity<std::function<
   // function.
   for (const Entity entity : view->ToRemoveEntities())
   {
-    if (!std::apply(_f, view->EntityComponentConstData(entity)))
+    const auto &data = view->EntityComponentData(entity);
+    if (!detail::applyFunction<const ComponentTypeTs...>(_f, entity, data))
     {
       break;
     }
@@ -485,15 +534,15 @@ void EntityComponentManager::EachRemoved(typename identity<std::function<
 
 //////////////////////////////////////////////////
 template<typename ...ComponentTypeTs>
-detail::View<ComponentTypeTs...> *EntityComponentManager::FindView() const
+detail::View *EntityComponentManager::FindView() const
 {
   auto viewKey = std::vector<ComponentTypeId>{ComponentTypeTs::typeId...};
 
   auto baseViewMutexPair = this->FindView(viewKey);
   auto baseViewPtr = baseViewMutexPair.first;
   if (nullptr != baseViewPtr)
   {
-    auto view = static_cast<detail::View<ComponentTypeTs...>*>(baseViewPtr);
+    auto view = static_cast<detail::View*>(baseViewPtr);
 
     std::unique_ptr<std::lock_guard<std::mutex>> viewLock;
     if (this->LockAddingEntitiesToViews())
@@ -527,7 +576,7 @@ detail::View<ComponentTypeTs...> *EntityComponentManager::FindView() const
   }
 
   // create a new view if one wasn't found
-  detail::View<ComponentTypeTs...> view;
+  detail::View view(std::set<ComponentTypeId>{ComponentTypeTs::typeId...});
 
   for (const auto &vertex : this->Entities().Vertices())
   {
@@ -547,8 +596,8 @@ detail::View<ComponentTypeTs...> *EntityComponentManager::FindView() const
   }
 
   baseViewPtr = this->AddView(viewKey,
-      std::make_unique<detail::View<ComponentTypeTs...>>(view));
-  return static_cast<detail::View<ComponentTypeTs...>*>(baseViewPtr);
+      std::make_unique<detail::View>(std::move(view)));
+  return static_cast<detail::View *>(baseViewPtr);
 }
 
 //////////////////////////////////////////////////