Fix seg fault in SetRemovedComponentsMsgs when TransformControl tool is used

[mrushyendra]

_msg.mutable_entities() need not necessarily contain the _entity as a key when SetRemovedComponentsMsgs() is called, for instance in AddEntityToMessage(). In the case where _entity has no components in entityComponents that changed, but does have some removed Components in removedComponents, this could lead to a seg fault when AddEntityToMessage calls SetRemovedComponentsMsgs.

Secondly, this is only tangentially related, but it looks like AddEntityToMessage also takes a list of component types _types:
https://github.com/ignitionrobotics/ign-gazebo/blob/2f7f33d4fa34657badec81558d5953fe7643769f/src/EntityComponentManager.cc#L849-L851

But this is currently ignored in the call to SetRemovedComponentMsgs. I was wondering if that was intentional, or if it should be modified as well?

Fixes #483

[iche033]

confirmed that this fixed the issue.

As for the second point, is the idea for passing in _types to skip adding components to the msg if they are not in _types?

[mrushyendra]

As for the second point, is the idea for passing in _types to skip adding components to the msg if they are not in _types?

Yeah, that's what I meant

[luca-della-vedova]

Just for another data point it also fixes the issue in our simulations

[iche033]

As for the second point, is the idea for passing in _types to skip adding components to the msg if they are not in _types?

Yeah, that's what I meant

I see. I'm not sure if we should apply the filter to removed components or not actually. Maybe @chapulina or @azeey may have a better idea?

[azeey]

SetRemovedComponentMsgs was added as a refactor of the component removal code in AddEntityToMessage. While the code was inside AddEntityToMessage, the entity would have been added to the message by the time it got to add removed components. So the fix in this PR is appropriate.

But this is currently ignored in the call to SetRemovedComponentMsgs. I was wondering if that was intentional, or if it should be modified as well?

I don't believe it was intentional. I think adding the filter would be a good idea.

[mrushyendra]

Just added the filter as well

[iche033]

the latest changes look good to me

[chapulina]

All changes look good to me. I hope no users were expecting the _types filter not to be applied to the removed components, because that would break their expectation.

Do you think you could add some tests to check that the removed types are filtered correctly, @mrushyendra ? It would also be great to add a test for the situation where SetRemovedComponentsMsgs is called on an entity that doesn't have changed components.

Thanks!

[azeey]

LGTM! I agree with @chapulina, tests would be great!

[mrushyendra]

Added a couple of tests

[azeey]

CI fails on macOS with the error

9: /Users/jenkins/workspace/ignition_gazebo-ci-pr_any-homebrew-amd64/ign-gazebo/src/EntityComponentManager_TEST.cc:2337: Failure
9: Expected equality of these values:
9:   e1Msg.components().size()
9:     Which is: 0
9:   1u
9:     Which is: 1
  9/156 Test   #9: UNIT_EntityComponentManager_TEST .....................***Exception: SegFault 14.53 sec
test 10

I've left comments to change EXPECT_EQ to ASSERT_EQ, but I'm not sure why the test failed.

[mrushyendra]

The tests should be passing now. Had to change the removedComponents traversal to make use of equal_range, instead of using find, since the latter may not return an iterator to the first matching value in the multimap:
https://github.com/ignitionrobotics/ign-gazebo/blob/a82ab6d2c82b1b8858bab746540b4fc21b9aacf9/src/EntityComponentManager.cc#L794-L795

[iche033]

#482 and #614 are affected by the crash so it would be nice to get this in

[adlarkin]

All tests are passing now (the only failure for macOS is for INTEGRATION_scene_broadcaster_system, which is unrelated to this PR). I am merging this.