MAYA-123579 fix crash when deleting a stage #2393
Conversation
The loop that retrieved all the stage was calling a function that could modify the container that was iterated over. Fix this by first caching the items we want to iterate. It only crashed in debug build due to the luck that the meory was still present and accessible in release build. Added a unit test that crashed in debug and now works with the fix.
| std::vector<Ufe::Path> paths; | ||
| paths.reserve(fPathToObject.size()); | ||
| for (const auto& pair : fPathToObject) { | ||
| stages.insert(stage(pair.first)); | ||
| paths.emplace_back(pair.first); | ||
| } | ||
|
|
There was a problem hiding this comment.
You shouldn't need to copy the vector yourself. There are both copy constructor and assignment operator. So you can just do:
auto paths(fPathToObject);
There was a problem hiding this comment.
The source container is a std::map, so there are no copy constructor for its keys.
There was a problem hiding this comment.
Oh sorry I missed that. I guess in that case I would have made a copy of the map (to a map) and loop over the map copy. But your way is also fine.
The unit test needs a new API to run, so disable it for older UFE versions.
|
The only PF failure is on Maya 2020 Windows platform, a single test, testUsdImportDisplacement timed out, which is unrelated to the change. |
|
|
||
| StageSet stages; | ||
| for (const auto& path : paths) { | ||
| stages.insert(stage(path)); |
There was a problem hiding this comment.
I think this code will append an empty/invalid Stage to the set when processing a deleted stage (but only once since all deleted stages will return the same hash). Would it be better to test that the stage is valid before adding it to the StageSet? The function is called allStages.
There was a problem hiding this comment.
I had the same reflection, but the old code did not check the validity either. The stage map holds weak pointers, so the validity can change at any moment anyway...
The loop that retrieved all the stage was calling a function that could modify the container that was iterated over. Fix this by first caching the items we want to iterate.
It only crashed in debug build due to the luck that the meory was still present and accessible in release build.
Added a unit test that crashed in debug and now works with the fix.