0003768: Slicer randomly hangs when closing or opening scene - MantisBT

ID	Project	Category	View Status	Date Submitted	Last Update

0003768	Slicer4	Core: MRML	public	2014-07-13 17:29	2018-03-02 11:06

Reporter	lassoan	Assigned To	alexy
Priority	normal	Severity	minor	Reproducibility	random
Status	closed	Resolution	fixed
Product Version
Target Version	Slicer 4.4.0	Fixed in Version	Slicer 4.4.0

Summary	0003768: Slicer randomly hangs when closing or opening scene
Description	Slicer randomly hangs when closing or opening scene (create some nodes, save the scene, load the saved scene a few times, close the scene, load the saved scene a few times). The hang is reproducible in debug mode when closing a scene in vtkMRMLNode::RemoveAllNodeReferenceIDs(const char* referenceRole) in this loop: while(this->GetNumberOfNodeReferences(referenceRole) > 0) { this->RemoveNthNodeReferenceID(referenceRole, 0); } The problem is most probably related to node references, as when a node reference is deleted it's not really removed but the referenced node ID is set to 0. This may cause many problems throughout the Slicer core and extensions. If the behavior of RemoveNodeReference is kept and invalid references are kept then all the places where node references are used have to be reviewed.
Tags	No tags attached.

jcfr 2014-07-14 06:44 administrator ~0012176	@Alex: Looking at the file vtkMRMLNode.cxx before any major changes (r23187), the code snippet pointed by Andras was already there: https://github.com/Slicer/Slicer/blob/bca1b5c2/Libs/MRML/Core/vtkMRMLNode.cxx#L1002-1005 I think the current state is now very similar to what we had before, that said having other eye looking at the code would be very helpful, to make it easier to track the changes related to vtkMRMLNode.cxx, I just created a gist where I copied the file at different revision: [1] https://gist.github.com/jcfr/99ffcecb75b52a8cb49b/revisions It has 3 revisions: Initial: Before any major changes (r23187 , Slicer/Slicer@bca1b5c2) Introduction of smart pointer: (r23191 , Slicer/Slicer@7b76f67) See also https://github.com/Slicer/Slicer/commit/7b76f67cc03b22e09c3a0eac3dfb09a8bcdde7c1 Current revision - after reverting all changes related to set nth reference .. r23415 , Slicer/Slicer@b5b6e3be While the webpage could be useful [1], you can also checkout the gist and see the change at each major stage: git clone https://gist.github.com/99ffcecb75b52a8cb49b.git The change that would need to be reviewed is this one: https://github.com/Slicer/Slicer/commit/7b76f67cc03b22e09c3a0eac3dfb09a8bcdde7c1#commitcomment-6997842 @Alex: Would be great if you could comment.

jcfr 2014-07-15 14:06 administrator ~0012181	Fixed in r23435 See http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=23435

Import 2017-06-07 23:51:09: master 82a6a18e 2014-07-15 17:56:02 jcfr Details Diff	BUG: Fix infinite loop when opening or closing a scene This commit fixes a regression introduced by commit r23191. Commit r23191 simplified the code introducing smart pointer, typedefs and also updated "SetNthNodeReferenceID" and "SetAndObserveNthNodeReferenceID" so that they have have similar code. It turns out that after this update new reference where systematically created with an ID being an empty string instead of being a NULL string. Commits following r23191 also updated the MRML reference system and attempted to fix what was most likely an issue with the "SetNth" methods. See r23192, r23196]. These commits fixes the behavior of the SetNth methods so that setting a reference N was always returning the associated reference if the reference n-1 was removed. It turned out that these changes caused more trouble than expected and to fully support the new behavior, major and backward incompatible changes in the displayable and storable nodes and also in the scene serialization would have occur. For that reason, commit r23395 and r23414 have been integrated to revert all commit updating the behavior but keeping the "smart pointer" and "typedefs" updates around. It is only by "reverting" these last commit that the issue fixed by this commit was revealed. Note for later: * In method almost doing the same thing, do not comment code without clearly mentioning the reason. * If very similar functions are doing almost the same thing, re-factoring the code is key to reduce the maintenance burden. Finally, this commit also update method "UpdateNthNodeReference" to set the ReferencingNode (it was done before the commit introducing smart pointers), the questions are "Why is this needed ?" and "What is causing the reference to be dissociated from a referencing node?" Fixes 0003754 Fixes 0003768 git-svn-id: http://svn.slicer.org/Slicer4/trunk@23435 3bd1e089-480b-0410-8dfb-8563597acbee
	mod - Libs/MRML/Core/vtkMRMLNode.cxx		Diff File

Date Modified	Username	Field	Change
2014-07-13 17:29	lassoan	New Issue
2014-07-13 17:29	lassoan	Status	new => assigned
2014-07-13 17:29	lassoan	Assigned To	=> alexy
2014-07-14 06:15	jcfr	Target Version	=> Slicer 4.4.0
2014-07-14 06:44	jcfr	Note Added: 0012176
2014-07-15 14:06	jcfr	Note Added: 0012181
2014-07-15 14:06	jcfr	Status	assigned => resolved
2014-07-15 14:06	jcfr	Fixed in Version	=> Slicer 4.4.0
2014-07-15 14:06	jcfr	Resolution	open => fixed
2017-06-10 08:51	jcfr	Changeset attached	=> Slicer master 82a6a18e
2018-03-02 11:06	jcfr	Status	resolved => closed