Re: r27812 - in /libs/gui/trunk: ChangeLog Source/NSBundleAdditions.m

[Fred Kiefer]

Am 02.03.2010 09:24, schrieb Wolfgang Lux:
>> Author: fredkiefer
>> Date: Sun Feb  8 13:54:21 2009
>> New Revision: 27812
>>
>> URL: http://svn.gna.org/viewcvs/gnustep?rev=27812&view=rev
>> Log:
>> Use KVC call setValue:forKey: to establish the outlet connection.
>> This will result in ivars being properly retained.
>>
>> Modified:
>>     libs/gui/trunk/ChangeLog
>>     libs/gui/trunk/Source/NSBundleAdditions.m
> 
> it's been a while since you've committed this change, but I just got
> round yesterday evening to track down a major space leak to exactly this
> change: Windows (and apparently other entities) loaded from a nib file
> never get released and deallocated. As an example, try this with Ink:
> - Open the memory panel from the info panel.
> - Open a new document window.
> - Update the memory panel. The panel now shows one (new) NSWindow, one
> (new) Document, etc.
> - Close the document window.
> - Update the memory panel. The Document and NSWindowController instance
> are gone, the NSWindow is still alive.
> If you revert your change, the NSWindow and its views are properly
> released.
> 
> At first, I was assuming that your change is correct and the nib loading
> code was improperly retaining the top-level entities. However, the
> longer I think about it the more I am convinced that it is your change
> that is wrong. In fact we do *not* want to retain ivars in general. It
> is well documented by Apple that only ivars that reference top-level
> entities of a nib are owned by the receiver (and this is already handled
> properly by the nib loading code), other ivars are not owned. An example
> for this is the tv attribute of Ink's Document objects. This attribute
> must not be retained when the connection is instantiated. Otherwise, the
> text view would not be released even when the window were released and
> deallocated when its document is closed. [1]
> 
> Wolfgang
> 
> [1] You can trivially test this without reverting your change by adding
> the statement [[aController window] autorelease] to the Document
> -windowControllerDidLoadNib: method. Now the window is deallocated when
> it is closed, but its text view still remains alive.
> 

Thank you for looking into this change. My main problem with it is that
it is already a year ago that I made it, so my memory is getting week on
this.
As far as I remember this change was about replacing the usage of
GNUstep base private functions with a proper official interface. Looking
at the diff this seems to be the case. Instead of having

       NSString *selName;
       SEL sel;         
        
       selName = [NSString stringWithFormat: @"address@hidden@:",       
                           [[_tag substringToIndex: 1] uppercaseString],        
                           [_tag substringFromIndex: 1]];       
       sel = NSSelectorFromString(selName);     
        
       if (sel && [_src respondsToSelector: sel])       
         {      
           [_src performSelector: sel withObject: _dst];        
         }      
       else     
         {      
           const char *nam = [_tag cString];    
           const char *type;    
           unsigned int size;   
           int offset;  
        
           /*   
            * Use the GNUstep additional function to set the instance   
            * variable directly.        
            * FIXME - need some way to do this for libFoundation and    
            * Foundation based systems.         
            */  
           if (GSObjCFindVariable(_src, nam, &type, &size, &offset))    
             {  
               GSObjCSetVariable(_src, offset, size, (void*)&_dst);     
             }  
         }


we now have
               [_src setValue: _dst forKey: _tag];

Even the old code was calling setter methods instead of setting an ivar
directly so retains could happen there as well. The main difference is
in the case where we have an object ivar and there is no accessor
method. There the old code would just set the ivar to the value and the
new code now does a retain in GSObjCSetVal().

I see this as a general problem of KVC, as long as GNUstep doesn't
supports ways to tell the compiler which ivars need to be retained when
setting their value, we need another mechanism to get this correct. The
only thing I can think of at the moment is to add a setter method for
these cases that wont use ASSIGN.

I also found some discussion on the dev mailing list triggered by change
r27706. I already recommended the same solution at that time, but never
implemented it myself :-(