setNextKeyView: dangling pointers

[Caba Conti]

The doubly linked list that keeps track of the next/previous key views
in NSView can be messed up easily, leaving behind dangling pointers
to deallocated views.
(I was using a CVS version from october 19th).

Here is a test program:
-----------------------
#include <AppKit/AppKit.h>

@interface MyView : NSView
- (void)dealloc;
- (void)setNextKeyView:(id)v;
- (void)setPreviousKeyView:(id)v;
@end

static MyView *tab1[5];
static char tab2[5] = { 'a', 'b', 'c', '0', '?' };

static char name_of(id v)
{
 int i;
 tab1[4] = v;

 for (i = 0; ; ++i)
   if (tab1[i] == v)
     return tab2[i];
}

static void pr(MyView *v)
{
 NSLog(@"%c <-- %c --> %c",
   name_of([v previousKeyView]),
   name_of(v),
   name_of([v nextKeyView]));
}

@implementation MyView
- (void)dealloc
{
 NSLog(@"dealloc %c", name_of(self));
 [super dealloc];
}
- (void)setNextKeyView:(NSView*)v;
{
 NSLog(@"setNextKeyView of %c to %c", name_of(self), name_of(v));
 [super setNextKeyView:v];
}
- (void)setPreviousKeyView:(NSView*)v;
{
 NSLog(@"setPreviousKeyView of %c to %c", name_of(self), name_of(v));
 [super setPreviousKeyView:v];
}
@end

int main(int argc, char *argv[])
{
 CREATE_AUTORELEASE_POOL(pool);
 MyView *a = tab1[0] = [MyView new];
 MyView *b = tab1[1] = [MyView new];
 MyView *c = tab1[2] = [MyView new];

 NSZombieEnabled = YES;
 [a setNextKeyView:c];
 [b setNextKeyView:c];
 pr(a); pr(b); pr(c);
 RELEASE(c);
 pr(a); pr(b);;
 RELEASE(b);
 pr(a);
 /* test for dangling pointer */
 if ([a nextKeyView] == c)
   {
     NSLog(@"Your program could crash when you release its views!!");
     NSLog(@"The deallocated view c will be sent a message now!");
   }
 RELEASE(a);
 NSLog(@"This is the end.");
 RELEASE(pool);
}
-----------------------

And its output:
-----------------------
2002-10-20 16:13:48.578 MyTest[17295] setNextKeyView of a to c
2002-10-20 16:13:48.595 MyTest[17295] setPreviousKeyView of c to a
2002-10-20 16:13:48.595 MyTest[17295] setNextKeyView of b to c
2002-10-20 16:13:48.596 MyTest[17295] setPreviousKeyView of c to b
2002-10-20 16:13:48.596 MyTest[17295] 0 <-- a --> c
2002-10-20 16:13:48.597 MyTest[17295] 0 <-- b --> c
2002-10-20 16:13:48.597 MyTest[17295] b <-- c --> 0
2002-10-20 16:13:48.598 MyTest[17295] dealloc c
2002-10-20 16:13:48.598 MyTest[17295] setNextKeyView of b to 0
2002-10-20 16:13:48.599 MyTest[17295] 0 <-- a --> c
2002-10-20 16:13:48.600 MyTest[17295] 0 <-- b --> 0
2002-10-20 16:13:48.600 MyTest[17295] dealloc b
2002-10-20 16:13:48.601 MyTest[17295] 0 <-- a --> c
2002-10-20 16:13:48.601 MyTest[17295] Your program could crash when you release 
its views!!
2002-10-20 16:13:48.604 MyTest[17295] The deallocated view c will be sent a 
message now!
2002-10-20 16:13:48.604 MyTest[17295] dealloc a
2002-10-20 16:13:48.606 MyTest[17295] Deallocated MyView (0x80e9ec8) sent 
setPreviousKeyView:
2002-10-20 16:13:48.606 MyTest[17295] This is the end.
-----------------------

As you can see, the list structure is inconsistent, resulting in a message sent 
to the deallocated view c.
I suggest to rewrite the setNextKeyView/setPreviousKeyView methods
in a way that they never produce inconsistent lists.

Here is a patch to NSView.m
The code is a little bit tricky, it tries to assign the right
values to the right fields at the right time, so that the recursive
message sends finally terminate.
(Please check whether this code is really right, before you apply the patch.)
-----------------------
- (void) setNextKeyView: (NSView *)aView
{
 NSView *old;

 if (_nextKeyView == aView || self == aView)
   return;

 if (!aView || [aView isKindOfClass: viewClass])
   {
     // As an exception, we do not retain aView, to avoid retain loops
     // (the simplest being a view retaining and being retained
     // by another view), which prevents objects from being ever
     // deallocated.  To understand how we manage without retaining
     // _nextKeyView, see [NSView -dealloc].
     old = _nextKeyView;
     _nextKeyView = nil;

     if (old)
       [old setPreviousKeyView:nil];

     _nextKeyView = aView;

     if (aView && [aView previousKeyView] != self)
        [aView setPreviousKeyView: self];
   }
}
- (void) setPreviousKeyView: (NSView *)aView
{
 NSView *old;

 if (_previousKeyView == aView || self == aView)
   return;

 if (!aView || [aView isKindOfClass: viewClass])
   {
     // As an exception, we do not retain aView, to avoid retain loops
     // (the simplest being a view retaining and being retained
     // by another view), which prevents objects from being ever
     // deallocated.  To understand how we manage without retaining
     // _previousKeyView, see [NSView -dealloc].
     old = _previousKeyView;
     _previousKeyView = nil;

     if (old)
       [old setNextKeyView:nil];

     _previousKeyView = aView;

     if (aView && [aView nextKeyView] != self)
        [aView setNextKeyView: self];
   }
}
-----------------------

The output after the patch:
-----------------------
2002-10-20 16:43:11.575 MyTest[17896] setNextKeyView of a to c
2002-10-20 16:43:11.590 MyTest[17896] setPreviousKeyView of c to a
2002-10-20 16:43:11.590 MyTest[17896] setNextKeyView of b to c
2002-10-20 16:43:11.591 MyTest[17896] setPreviousKeyView of c to b
2002-10-20 16:43:11.591 MyTest[17896] setNextKeyView of a to 0
2002-10-20 16:43:11.592 MyTest[17896] setPreviousKeyView of c to 0
2002-10-20 16:43:11.592 MyTest[17896] 0 <-- a --> 0
2002-10-20 16:43:11.592 MyTest[17896] 0 <-- b --> c
2002-10-20 16:43:11.593 MyTest[17896] b <-- c --> 0
2002-10-20 16:43:11.593 MyTest[17896] dealloc c
2002-10-20 16:43:11.594 MyTest[17896] setNextKeyView of b to 0
2002-10-20 16:43:11.594 MyTest[17896] setPreviousKeyView of c to 0
2002-10-20 16:43:11.595 MyTest[17896] setNextKeyView of b to 0
2002-10-20 16:43:11.595 MyTest[17896] 0 <-- a --> 0
2002-10-20 16:43:11.596 MyTest[17896] 0 <-- b --> 0
2002-10-20 16:43:11.598 MyTest[17896] dealloc b
2002-10-20 16:43:11.600 MyTest[17896] 0 <-- a --> 0
2002-10-20 16:43:11.601 MyTest[17896] dealloc a
2002-10-20 16:43:11.601 MyTest[17896] This is the end.
-----------------------
This looks OK to me now.


-Caba