Re: [PATCH] Minor fix in NSWindowController
Quentin Mathé wrote: > I have done new tests with other applications today like Notebook, Ink > and ToolbarExample (which is document based). With the two last ones, > the result is a segmentation fault on a document window close :-/. It's > retain/release issue. Now I think that's the extra 'retain' I removed > in NSWindowController is just covering another bug (may be more). I > would say it's probably related to - isReleasedWhenClosed value not > properly ignored everywhere when NSWindowController is used in > NSDocument architecture context. What's your take on this ? > Most likely this segmentation fault was the reason I introduced the retain call in the first place. Now with the new explanation from Apple we need to change some code. I would insist, that what ever you change it should not break existing applications. Apart from that feel free to bring our code closer to what should be done. From my point of view the whole code in [NSWindowController _windowWillClose:] could go away, but I am not deep enough into this any more. Cheers Fred ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: [PATCH] Minor fix in NSWindowController
Le 19 avr. 06 à 02:36, Fred Kiefer a écrit : Quentin Mathé wrote: I think you wrote the comments, here is the ChangeLog entry which introduced this 'retain' line : 2003-08-30 Fred Kiefer <[EMAIL PROTECTED]> * Source/NSWindowController.m Changed [setWindow:] to manage the notification connection to the window. [initWithWindow:], [dealloc] and [_windowWillClose:] now use [setWindow:]. [setDocument:] now no longer retains the document, as this is already retaining the window controller. Simplified [loadWindow] by using the method [windowNibPath]. Oops, I must admit that I don't have any memory of this change any more. If your new code works for document based applications as well as for others, then it should be fine to submit it. Could you please test with Ink as well? I have done new tests with other applications today like Notebook, Ink and ToolbarExample (which is document based). With the two last ones, the result is a segmentation fault on a document window close :-/. It's retain/release issue. Now I think that's the extra 'retain' I removed in NSWindowController is just covering another bug (may be more). I would say it's probably related to - isReleasedWhenClosed value not properly ignored everywhere when NSWindowController is used in NSDocument architecture context. What's your take on this ? The previous comment makes a bit more sense (unlike the most recent one) : /* * If the window is set to isReleasedWhenClosed, it will release * itself, so nil out our reference so we don't release it again * * Apple's implementation doesn't seem to deal with this case, and * crashes if isReleaseWhenClosed is set. */ */ _window = nil; However I think it's not needed because there is no 'retain' or 'release' calls on _window in NSWindowController.m (not even in - dealloc method). This is not completly true, the retaining and releasing of _window gets done in setWindow:. Yes, I just forgot to look for 'ASSIGN' in the code. This seems to have been the core of that patch, to move all code which changes the _window ivar into one place. This change was related to the bug report #4840, which was about a similar problem as the one you are facing now. If I only could remember... You are right, both problems are quite similar, I think bug 4840 isn't reintroduced by this extra retain call because _window ivar is set to nil and the code is probably checking this ivar (or the related NSDocument instance) in order to know whether any documents are still open (before displaying the 'save' alert). In the bug report, you made the following comment : 'This behaviour is actually correct, as long as the window hasn't set isReleasedWhenClosed. Then the document and the window controller and the window will hang around invisible. This does not make any sense to me, but is acording to the spec.' Interestingly it seems Apple finally takes in account this border case. In the latest Cocoa documentation, it is stated that - isReleasedWhenClosed value is ignored when you are using a window controller together with NSDocument and related classes. Quentin. -- Quentin Mathé [EMAIL PROTECTED] ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: [PATCH] Minor fix in NSWindowController
Hi Quentin, Quentin Mathé wrote: > Le 28 mars 06 à 00:57, Fred Kiefer a écrit : >> >> I am rather unsure about this patch.I don't quite remember, who did >> put in these commetn lines on retaining the window, but I would >> expect that there were reasons for it at that time. Still they may >> have been the now redundant solution of an already resolved >> problem. > > I think you wrote the comments, here is the ChangeLog entry which > introduced this 'retain' line : 2003-08-30 Fred Kiefer > <[EMAIL PROTECTED]> > > * Source/NSWindowController.m Changed [setWindow:] to manage the > notification connection to the window. [initWithWindow:], [dealloc] > and [_windowWillClose:] now use [setWindow:]. [setDocument:] now no > longer retains the document, as this is already retaining the window > controller. Simplified [loadWindow] by using the method > [windowNibPath]. > Oops, I must admit that I don't have any memory of this change any more. If your new code works for document based applications as well as for others, then it should be fine to submit it. Could you please test with Ink as well? > > The previous comment makes a bit more sense (unlike the most recent > one) : > > /* * If the window is set to isReleasedWhenClosed, it will release * > itself, so nil out our reference so we don't release it again * * > Apple's implementation doesn't seem to deal with this case, and * > crashes if isReleaseWhenClosed is set. */ */ _window = nil; > > However I think it's not needed because there is no 'retain' or > 'release' calls on _window in NSWindowController.m (not even in - > dealloc method). > This is not completly true, the retaining and releasing of _window gets done in setWindow:. This seems to have been the core of that patch, to move all code which changes the _window ivar into one place. This change was related to the bug report #4840, which was about a similar problem as the one you are facing now. If I only could remember... Fred ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: [PATCH] Minor fix in NSWindowController
Le 28 mars 06 à 00:57, Fred Kiefer a écrit : Hi Quentin, I am rather unsure about this patch.I don't quite remember, who did put in these commetn lines on retaining the window, but I would expect that there were reasons for it at that time. Still they may have been the now redundant solution of an already resolved problem. Hi Fred, I think you wrote the comments, here is the ChangeLog entry which introduced this 'retain' line : 2003-08-30 Fred Kiefer <[EMAIL PROTECTED]> * Source/NSWindowController.m Changed [setWindow:] to manage the notification connection to the window. [initWithWindow:], [dealloc] and [_windowWillClose:] now use [setWindow:]. [setDocument:] now no longer retains the document, as this is already retaining the window controller. Simplified [loadWindow] by using the method [windowNibPath]. You can take a look at the related diff with this link : svn.gna.org/viewcvs/gnustep/libs/gui/trunk/Source/ NSWindowController.m?r2=17574&rev=17574&r1=16917&dir_pagestart=150> The previous comment makes a bit more sense (unlike the most recent one) : /* * If the window is set to isReleasedWhenClosed, it will release * itself, so nil out our reference so we don't release it again * * Apple's implementation doesn't seem to deal with this case, and * crashes if isReleaseWhenClosed is set. */ */ _window = nil; However I think it's not needed because there is no 'retain' or 'release' calls on _window in NSWindowController.m (not even in - dealloc method). Did you do sufficent tests with your patch? As there aren't that many document based applications for GNUstep, we really shoud try all of them, before releasing the patch. I have done far more extensive tests right now and encountered no issues at all with applications like ProjectCenter, Project Manager, Gorm (going to try a few more). Thanks, Quentin. -- Quentin Mathé [EMAIL PROTECTED] ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
Re: [PATCH] Minor fix in NSWindowController
Hi Quentin, I am rather unsure about this patch.I don't quite remember, who did put in these commetn lines on retaining the window, but I would expect that there were reasons for it at that time. Still they may have been the now redundant solution of an already resolved problem. Did you do sufficent tests with your patch? As there aren't that many document based applications for GNUstep, we really shoud try all of them, before releasing the patch. Cheers Fred Quentin Mathé wrote: > > Here is a little patch to have proper window controller deallocation > when used in document architecture context. It is based on an initial > bug report by Saso Kiselkov. > > I'm going to briefly summarize Cocoa documentation… > When you are using a window controller within the document > achitecture context (NSDocument + NSDocumentController), - > -releasedWhenClosed value isn't taken in account : when a window is > closed, the window is always released by the window controller, > itself released by the document object. That's not true for a window > controller used alone. > > To enter in the details, in our case, the current NSWindowController > code includes a dubious retain call on its window ivar (within > _windowWillClose: private method). This prevents the window to be > correctly deallocated unlike its window controller. > > The -retain call currently presents code is unbalanced, that results in > a memory leak and sometimes in a segmentation fault. > For example… You decide to use a toolbar with your document window and > the document object plays the role of the toolbar delegate. When the > document window is closed, the window controller and the document > object are released, but the document window is not (and its toolbar > neither). > The next time you are going to open or create an identical document, a > new document window will be created with a fresh toolbar. When you are > going to manipulate this toolbar, it will try to propagate its state to > other toolbars still in use with the same identifier. At this moment, > the undeallocated toolbar will try to ask its delegate for items, but > the delegate (document object) has already been deallocated, this > invalid pointer access will then trigger a segmentation fault. > > The fix just consists to remove the -retain call, then everything is > properly deallocated on document window closing. Since I don't really > understand the explanation in the comment related to the offending > line), I'm perhaps missing an important detail in with this patch, yet > I have experienced no issue in my tests with this fix. ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
[PATCH] Minor fix in NSWindowController
Hi, Here is a little patch to have proper window controller deallocation when used in document architecture context. It is based on an initial bug report by Saso Kiselkov. I'm going to briefly summarize Cocoa documentation… When you are using a window controller within the document achitecture context (NSDocument + NSDocumentController), - -releasedWhenClosed value isn't taken in account : when a window is closed, the window is always released by the window controller, itself released by the document object. That's not true for a window controller used alone. To enter in the details, in our case, the current NSWindowController code includes a dubious retain call on its window ivar (within _windowWillClose: private method). This prevents the window to be correctly deallocated unlike its window controller. The -retain call currently presents code is unbalanced, that results in a memory leak and sometimes in a segmentation fault. For example… You decide to use a toolbar with your document window and the document object plays the role of the toolbar delegate. When the document window is closed, the window controller and the document object are released, but the document window is not (and its toolbar neither). The next time you are going to open or create an identical document, a new document window will be created with a fresh toolbar. When you are going to manipulate this toolbar, it will try to propagate its state to other toolbars still in use with the same identifier. At this moment, the undeallocated toolbar will try to ask its delegate for items, but the delegate (document object) has already been deallocated, this invalid pointer access will then trigger a segmentation fault. The fix just consists to remove the -retain call, then everything is properly deallocated on document window closing. Since I don't really understand the explanation in the comment related to the offending line), I'm perhaps missing an important detail in with this patch, yet I have experienced no issue in my tests with this fix. Thanks, Quentin. WindowController.patch Description: Binary data -- Quentin Mathé [EMAIL PROTECTED] ___ Gnustep-dev mailing list Gnustep-dev@gnu.org http://lists.gnu.org/mailman/listinfo/gnustep-dev
