Page MenuHomePhabricator

Scene file loading still crashes (Windows, debug)
Closed, ResolvedPublic

Description

See T:\temp\Stefan\AB.mitk.

File -> Open: (90% crash)
Drag & Drop: Loads but progress bar hangs at 85%.

Reproduced by me and Caspar.

Event Timeline

User kislinsk has pushed new remote branch:

bug-18286-FixSceneFileReading

This was a tricky one as it was a combination of three bugs:

Two bugs are related to calling std::find() on empty containers. Always check with empty() first as in that case begin() is invalid but find requires a valid iterator for the beginning of the range.

The other bug is related to the trial of keeping the old data storage entries to compare them with the new ones. Instead of keeping the DataStorage::SetOfObjects itself it was casted to a STL container in which case the reference count is lost and the SetOfObjects is released while the STL container still points to this possibly corrupted memory location.

As said during the bug-squashing event today, Keno worked exactly on these issues. Please consult with him to avoid duplicated efforts (which probably happened already).

This is still happening AFTER ther fixes of Keno.

Oh, you are right. :-) I was told (not by Keno) that Keno's fixes were already merged into the master. However, this branch fixes also two other bugs, e.g., during scene loading when the Project was closed. So I guess it is a more complete fix. Talking with Keno tomorrow.

The std::find statement is not true. Calling std::find on a [end, end) range is perfectly valid.

Could you please test the fix by just increasing the reference count of the data storage entries without the IOUtil and other container fixes? The fix would then probably reduce to the identical fix by Keno.

Hm, the standard might say that this is valid (?), but the library implementation of MSVC unfortunately don't care. :-/ I can definitely reproduce the exceptions in debug mode when calling find on the empty containers even they do not occur in 100%. Had this issue already when implementing the property services where I was using some maps. Anyway, it makes the code in this bugfix more efficient as the execution of some other lines of code is avoided in most of the cases.

Regarding the reference count: What I did was to completely remove the (unnecessary) conversation/cast to a STL container for the sake of iterating over it, as these functionality is directly available in SetOfObjects. As I keep a reference to it, it is the nice way of keeping the reference count up as long as necessary. It affects the same line as in Keno's solution, yes, but works without the extra copy.

In a nutshell: I would strongly recommend to keep the fix as it is. :-)

[ba54d3]: Merge branch 'bug-18286-FixSceneFileReading'

Merged commits:

2014-10-16 10:22:19 Stefan Kislinskiy [5c6449]
Do not call std find on empty containers anymore and fixed accessing of already released object.

User kislinsk has pushed new remote branch:

bug-18286-FixSceneFileReading2

Split core changes from module changes.