Garbage collector can delete objects twice if a field is unregistered before cleared.
This issue was created automatically from an original Mantis Issue. Further discussion may take place here.
I have recently run into some behavior from the garbage collector that I consider erroneous. Consider two VTK objects, A and B, that are garbage collected and point to each other. Furthermore, assume that the only reference to B is from A (whereas A has multiple references to it). Consider what happens when a method in A executes the following code:
A->PointerToB->UnRegister(this);
A->PointerToB = NULL;
The intent of the code is obviously meant for A to drop its reference to B. However, it causes problems. When UnRegister is called on B, its reference count drops to 0 and its destructor is called. We can assume that B is a well behaved object that unregisters all of its references in its destructor. When it unreferences A, the garbage collector is run. The garbage collector picks up A's reference to B (which has not yet been obliterated) and bad things happen. In one instance I had the garbage collector code fail an assert. In another instance I have had the "B" object destructed twice. In either case (particularly the latter) the problem code is not immediately apparent. I have attached some test code that exercises this problem.
I think the garbage collector can be corrected by maintaining a set of objects that are in the process of destruction. Whenever a link to an object in the process of destruction is encountered, it can be ignored with the assumption that it is about to be obliterated as in the code above. I can think of no down side to this approach.
There are those that will probably argue that this is technically not a bug (which is why I have not yet submitted a bug report). Technically, the garbage collector is doing its job correctly, and the user has made the mistake of keeping a pointer around after it has been destroyed and then reporting it to the garbage collector. The "correct" code would be
vtkObjectBase *tmp = A->PointerToB;
A->PointerToB = NULL;
tmp->UnRegister(this);
However, I find this latter construction less clear (from a human readability standpoint) than the original code. Also, as a programmer, I will never write the second construction unless I am actively thinking about which variables are garbage collected (which I am not most of the time).