[vtk-developers] Corrupted observer lists in vtkObject

[Moreland, Kenneth]

While tracking down a crash in ParaView, I found an anomaly that can
cause the InvokeEvent mechanism in vtkObject (actually vtkSubjectHelper)
to follow a corrupted linked list of observers.

The problem happens when an observer deletes itself or another observer
while being invoked.  That in itself is not enough.  vtkSubjectHelper
already uses an ivar, ListModified, to track when the linked list is
changed.  The real problem occurs when the invoked observer first
modifies the linked list and then invokes another event.  On the
recursive call to InvokeEvent, the ListModified flag is reset, and when
control returns to the original InvokeEvent modification, it is unaware
that its linked list is corrupt.

The fix is fairly simple: save the incoming state of ListModified on the
stack in InvokeEvent, and then restore it before leaving.  I listed a
patch below.  It is quite small and I'm pretty sure it fixes this
problem without breaking anything else.  If no one objects, I will check
in this fix tomorrow.

-Ken

   ****      Kenneth Moreland
    ***      Sandia National Laboratories
***********  
*** *** ***  email: kmorel at sandia.gov
**  ***  **  phone: (505) 844-8919
    ***      fax:   (505) 845-0833

--- vtkObject.cxx       4 Oct 2006 11:10:19 -0000       1.98
+++ vtkObject.cxx       10 Jan 2008 20:00:19 -0000
@@ -465,8 +465,22 @@
 {
   int focusHandled = 0;
 
+  // When we invoke an event, the observer may add or remove observers.
To make
+  // sure that the iteration over the observers goes smoothly, we
capture any
+  // change to the list with the ListModified ivar.  However, an
observer may
+  // also do something that causes another event to be invoked in this
object.
+  // That means that this method will be called recursively, which
means that we
+  // will obliterate the ListModified flag that the first call is
relying on.
+  // To get around this, save the previous ListModified value on the
stack and
+  // then restore it before leaving.
+  int saveListModified = this->ListModified;
   this->ListModified = 0;
 
+  // This clearing of the Visited flag can be problematic if this
method is
+  // being called recursively.  It means that the call further up the
stack will
+  // probably call some observers multiple times.  In the worst case
scenario
+  // this might cause an infinite loop, but I think that is pretty
unlikely
+  // in practice.
   vtkObserver *elem = this->Start;
   while (elem)
     {
@@ -511,6 +525,7 @@
         if(command->GetAbortFlag())
           {
           command->UnRegister();
+          this->ListModified = saveListModified;
           return 1;
           }
         command->UnRegister();
@@ -550,6 +565,7 @@
         if(command->GetAbortFlag())
           {
           command->UnRegister();
+          this->ListModified = saveListModified;
           return 1;
           }
         command->UnRegister();
@@ -566,6 +582,7 @@
       }
     }
 
+  this->ListModified = saveListModified;
   return 0;
 }