Prevent a segfault when updating ComponentInspector

[srmainwaring]

🦟 Bug fix

Summary

This PR fixes a segfault observed on macOS that occurs when selecting different object types in the EntityTree.

The issue arises in the method ComponentInspector::Update in the loop that removes unused components from the component list. The loop iterator may be invalidated by the erase operation in the loop body - this can cause a segfault (the b-tree rebalance fails inside std::map). The error was observed on macOS, it may be dependent on the implementation of the std libraries.

Reproduce the error

1. Open an ignition gazebo session:

   $ ign gazebo -v4 shapes.sdf

   (NB: on macOS the server and gui must be started in different terminals because the approach to forking used in the ignition ruby script is not supported).

2. Load the component inspector and entity tree (if not already configured)

3. Select a different component type in the entry tree (eg. select a light if a shape is already selected)

4. At this point the application may segfault.

Fix

The approach taken is to first build a list of items to remove, then remove them in a second step. This ensures that at no point can a loop iterator be invalidated. There are other approaches that involve using a second iterator in the loop over the component items, however it was found that these may also fail because the erase step is called using a QMetaObject::invokeMethod (so the problem may be a threading / mutex issue rather than a invalid iterator). Either way, ensuring that the object being iterated over at the removal step is not the container having items removed prevents this problem.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Not all tests pass on macOS (the environment is still experimental). However the failures do not appear to be connected to this change:

97% tests passed, 5 tests failed out of 196

Total Test time (real) = 499.63 sec

The following tests FAILED:
	 47 - UNIT_ModelCommandAPI_TEST (SEGFAULT)
	 91 - INTEGRATION_examples_build (Failed)
	117 - INTEGRATION_level_manager_runtime_performers (Subprocess aborted)
	131 - INTEGRATION_multiple_servers (Subprocess aborted)
	165 - INTEGRATION_user_commands (SEGFAULT)

Note to maintainers: Remember to use Squash-Merge

[chapulina]

Thanks, LGTM. Just waiting for CI to finish

[srmainwaring]

Thanks, LGTM. Just waiting for CI to finish

Thanks @chapulina. I've fixed a missing include needed by code check. The PR needs to be updated with the branch - how do you prefer this to be done: merge (using the github button) or rebase and force push. I'm used to the ardupilot workflow where they do the latter but will run with whatever the OSRF house rules are.

[chapulina]

merge (using the github button) or rebase and force push

I think we usually use the github button, because rebase can be disruptive for people's local clones in case they're testing this PR. But often rebasing is ok if no one else is actively reviewing the PR. I'll just click the github button for expediency now.

[codecov]

Codecov Report

Merging #1167 (e8bf33e) into ign-gazebo6 (827fc3f) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head e8bf33e differs from pull request most recent head 2ad995a. Consider uploading reports for the commit 2ad995a to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo6    #1167      +/-   ##
===============================================
- Coverage        62.16%   62.15%   -0.01%     
===============================================
  Files              256      256              
  Lines            20395    20398       +3     
===============================================
  Hits             12679    12679              
- Misses            7716     7719       +3     

Impacted Files	Coverage Δ
.../plugins/component_inspector/ComponentInspector.cc	5.21% <0.00%> (-0.04%)	⬇️

---

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 827fc3f...2ad995a. Read the comment docs.