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

[Fred Kiefer]

I am not that sure about this.
When I changed to the current code it was to get the Beans application
working with GNUstep. There we had the case the components loaded via
NIB where released to early by GNUstep.

We rather should make sure we handle all cases as similar as Apple as
possible.
Richard already added a simple test for KVC to see, if our behaviour
there matches Cocoa. We will need a similar check for the
NSNibOutletConnector.

Fred

Am 02.03.2010 11:22, schrieb address@hidden:
> I remember having stumbled over this piece of code being the source of a
> memory management issue of mine. Basically I renamed my setter methods
> so the 'GSObjCSetVariable' branch would get used, all my problems
> disappeared that way. I will dig out my project which had this issue in
> the evening.
> But wouldn't the proper way be to set the variables using
> 'GSObjCSetVariable' only?
> 
> Just my 2 cents...
> 
> Cheers,
> TOM
> 
> Zitat von Fred Kiefer <address@hidden>:
> 
>> 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 :-(