From 327cde7609c679d46a31806de3a4fab86666b6ce Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Tue, 10 Nov 2020 19:44:30 -0600 Subject: [PATCH] Fix segfault in the Breadcrumb system when associated model is unloaded (#454) The segfault occurs when a model containing a Breadcrumbs system gets unloaded by the level manager. Since systems don't get unloaded, the Breadcrumbs system continues to operate assuming that the model it is associated with is valid. Signed-off-by: Addisu Z. Taddese --- src/systems/breadcrumbs/Breadcrumbs.cc | 15 ++++ test/integration/breadcrumbs.cc | 120 +++++++++++++++++++++++++ test/worlds/breadcrumbs_levels.sdf | 120 +++++++++++++++++++++++++ 3 files changed, 255 insertions(+) create mode 100644 test/worlds/breadcrumbs_levels.sdf diff --git a/src/systems/breadcrumbs/Breadcrumbs.cc b/src/systems/breadcrumbs/Breadcrumbs.cc index 7ab227136e..8629837606 100644 --- a/src/systems/breadcrumbs/Breadcrumbs.cc +++ b/src/systems/breadcrumbs/Breadcrumbs.cc @@ -74,6 +74,12 @@ void Breadcrumbs::Configure(const Entity &_entity, _sdf->Get("allow_renaming", this->allowRenaming).first; this->model = Model(_entity); + if (!this->model.Valid(_ecm)) + { + ignerr << "The Breadcrumbs system should be attached to a model entity. " + << "Failed to initialize." << std::endl; + return; + } if (!_sdf->HasElement("breadcrumb")) { @@ -160,6 +166,15 @@ void Breadcrumbs::PreUpdate(const ignition::gazebo::UpdateInfo &_info, std::back_inserter(cmds)); this->pendingCmds.clear(); } + // Check that the model is valid before continuing. This check is needed + // because the model associated with the Breadcrumbs might have been + // unloaded by the level manager. Ideally, this system would have been + // unloaded along with the model, but that is not currently the case. See + // issue #113 + if (!this->model.Valid(_ecm)) + { + return; + } auto poseComp = _ecm.Component(this->model.Entity()); diff --git a/test/integration/breadcrumbs.cc b/test/integration/breadcrumbs.cc index 35ba3c8003..40e1402da5 100644 --- a/test/integration/breadcrumbs.cc +++ b/test/integration/breadcrumbs.cc @@ -20,12 +20,15 @@ #include #include +#include + #include #include #include #include +#include "ignition/gazebo/Entity.hh" #include "ignition/gazebo/Server.hh" #include "ignition/gazebo/SystemLoader.hh" #include "ignition/gazebo/components/Name.hh" @@ -547,6 +550,123 @@ TEST_F(BreadcrumbsTest, AllowRenaming) EXPECT_TRUE(this->server->HasEntity("B1_0_1")); } +///////////////////////////////////////////////// +/// Return a list of model entities whose names match the given regex +std::vector ModelsByNameRegex( + const gazebo::EntityComponentManager &_ecm, const std::regex &_re) +{ + std::vector entities; + _ecm.Each( + [&](const Entity _entity, const components::Model *, + const components::Name *_name) + { + if (std::regex_match(_name->Data(), _re)) + { + entities.push_back(_entity); + } + return true; + }); + + return entities; +} + +// The test checks that models containing Breadcrumbs can be unloaded and loaded +// safely +TEST_F(BreadcrumbsTest, LevelLoadUnload) +{ + // Start server + this->LoadWorld("test/worlds/breadcrumbs_levels.sdf", true); + + test::Relay testSystem; + transport::Node node; + auto deploy = + node.Advertise("/model/tile_1/breadcrumbs/B1/deploy"); + + const std::size_t iterTestStart = 1000; + const std::size_t nIters = iterTestStart + 10000; + + std::regex reTile1{"tile_1"}; + std::regex reBreadcrumb{"B1_.*"}; + testSystem.OnPostUpdate( + [&](const gazebo::UpdateInfo &_info, + const gazebo::EntityComponentManager &_ecm) + { + // Ensure that tile_1 is loaded at the start, deploy a breadcrumb + if (_info.iterations == iterTestStart) + { + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(1u, tiles.size()); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + // Check if the breadcrumb has been deployed + else if (_info.iterations == iterTestStart + 1000) + { + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(1u, breadcrumbs.size()); + } + // Move the performer (sphere) outside the level. This should cause + // tile_1 to be unloaded + else if (_info.iterations == iterTestStart + 1001) + { + msgs::Pose req; + req.set_name("sphere"); + req.mutable_position()->set_x(30); + msgs::Boolean rep; + bool result; + node.Request("/world/breadcrumbs_levels/set_pose", req, 1000, rep, + result); + EXPECT_TRUE(result); + } + // Check that tile_1 is unloaded, then try deploying breadcrumb. This + // should fail because the model associated with the breadcrumb system + // is unloaded. + else if (_info.iterations == iterTestStart + 2000) + { + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(0u, tiles.size()); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + // Check that no new breadcrumbs have been deployed + else if (_info.iterations == iterTestStart + 3000) + { + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(1u, breadcrumbs.size()); + } + // Move the performer (sphere) back into the level. This should load + // tile_1 and a new Breadcrumbs system. + else if (_info.iterations == iterTestStart + 3001) + { + msgs::Pose req; + req.set_name("sphere"); + req.mutable_position()->set_x(0); + msgs::Boolean rep; + bool result; + node.Request("/world/breadcrumbs_levels/set_pose", req, 1000, rep, + result); + EXPECT_TRUE(result); + } + // Check that tile_1 is loaded, then try deploying breadcrumb. + else if (_info.iterations == iterTestStart + 4000) + { + auto tiles = ModelsByNameRegex(_ecm, reTile1); + EXPECT_EQ(1u, tiles.size()); + EXPECT_TRUE(deploy.Publish(msgs::Empty())); + } + // Check that only one additional breadcrumb has been deployed even + // though there are two Breadcrumbs systems associated with the model. + // The original Breadcrumbs system should now be invalid because the + // model gets a new Entity ID. + else if (_info.iterations == iterTestStart + 5000) + { + auto breadcrumbs = ModelsByNameRegex(_ecm, reBreadcrumb); + EXPECT_EQ(2u, breadcrumbs.size()); + } + }); + + this->server->AddSystem(testSystem.systemPtr); + this->server->Run(true, nIters, false); +} + ///////////////////////////////////////////////// /// Main int main(int _argc, char **_argv) diff --git a/test/worlds/breadcrumbs_levels.sdf b/test/worlds/breadcrumbs_levels.sdf new file mode 100644 index 0000000000..d0f83e52b3 --- /dev/null +++ b/test/worlds/breadcrumbs_levels.sdf @@ -0,0 +1,120 @@ + + + + + + 0.001 + 1.0 + + + + + + + + + + 0 0 0.1 0 0 0 + true + + + + + 10 10 0.2 + + + + + + + 10 10 0.2 + + + + 0.8 0.8 0.8 1 + 0.0 0.8 0.0 1 + 0.8 0.8 0.8 1 + + + + + 3 + 15 + true + + + + 0 0 5 0 0 0 + + + + + 0.3 0.3 0.5 + + + + + + + 0.3 0.3 0.5 + + + + + + + + + + + + 0 0 2 0 0 0 + true + + + + + 1.0 + + + + + + + 1.0 + + + + + + + + + sphere + + + 4 4 2 + + + + + 0 0 2.5 0 0 0 + + + 10 10 10 + + + 2 + tile_1 + + + + + +