Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash in the follow_actor example #958

Merged
merged 9 commits into from
Aug 12, 2021
Merged

Conversation

luca-della-vedova
Copy link
Member

This PR fixes a sneaky segfault introduced in #858, while the fix works I'm open to better solutions if anything is recommended (it makes the function arguments a bit bulky).

The issue was that the previous version of the code was relying on an unfortunate coincidence related to the variable names for the entityPoses and trajectoryPoses data structures, specifically when updating them here the variables would refer to the local variables introduced in the beginning of the Update() function, here.
However, when during the refactor the code was moved into a new function, the Implementation class variables (i.e. this and this) with the same name are being silently used behind the scenes, the problem is that at the beginning of the Update() function there is a std::move call that moves from the member data structures into the local variables, hence it becomes a case of using a data structure after it has been moved, which causes a segfault for a null pointer exception in the data pointer.

To reproduce the bug / check the PR just run the follow_actor demo with and without the PR:
ign gazebo follow_actor.sdf

Signed-off-by: Luca Della Vedova <[email protected]>
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Aug 6, 2021
@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #958 (0e24087) into ign-gazebo4 (e95f17c) will increase coverage by 0.93%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4     #958      +/-   ##
===============================================
+ Coverage        66.13%   67.06%   +0.93%     
===============================================
  Files              243      243              
  Lines            18205    18207       +2     
===============================================
+ Hits             12039    12210     +171     
+ Misses            6166     5997     -169     
Impacted Files Coverage Δ
src/rendering/RenderUtil.cc 48.67% <100.00%> (+6.59%) ⬆️
src/gui/plugins/entity_tree/EntityTree.cc 9.83% <0.00%> (-3.28%) ⬇️
src/SimulationRunner.cc 93.04% <0.00%> (-1.05%) ⬇️
src/rendering/SceneManager.cc 37.73% <0.00%> (+15.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34eb437...0e24087. Read the comment docs.

@luca-della-vedova luca-della-vedova changed the base branch from ign-gazebo5 to ign-gazebo4 August 6, 2021 09:41
@azeey azeey removed their request for review August 6, 2021 16:40
iche033 added 2 commits August 6, 2021 11:12
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, I can reproduce the crash and this fixes it for me too.

Adding a sensors system and a camera to the follow_actor integration test also causes the test to crash. I've added it in the fix/follow_actor_crash_test branch so we can test the rendering side as part of the integration test in the future. Can you take a look and merge if that looks good to you?

@chapulina chapulina added 🔮 dome Ignition Dome and removed 🏢 edifice Ignition Edifice labels Aug 6, 2021
@adlarkin
Copy link
Contributor

adlarkin commented Aug 9, 2021

Thanks @luca-della-vedova for catching this! Sorry I didn't notice this when working on #858 😬

Adding a sensors system and a camera to the follow_actor integration test also causes the test to crash

Do we need to fix this as well?

@iche033
Copy link
Contributor

iche033 commented Aug 9, 2021

Do we need to fix this as well?

oh I meant adding it to the test causes to crash before the changes in this PR, and confirmed that the test no longer crashes after this PR.

Signed-off-by: Luca Della Vedova <[email protected]>
@luca-della-vedova
Copy link
Member Author

nice, I can reproduce the crash and this fixes it for me too.

Adding a sensors system and a camera to the follow_actor integration test also causes the test to crash. I've added it in the fix/follow_actor_crash_test branch so we can test the rendering side as part of the integration test in the future. Can you take a look and merge if that looks good to you?

Done!

Signed-off-by: Luca Della Vedova <[email protected]>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@chapulina chapulina enabled auto-merge (squash) August 12, 2021 04:48
@chapulina chapulina merged commit fbe137e into ign-gazebo4 Aug 12, 2021
@chapulina chapulina deleted the fix/follow_actor_crash branch August 12, 2021 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants