search

gnustep / libs-gui

The GNUstep gui library is a library of graphical user interface classes written completely in the Objective-C language; the classes are based upon Apple's Cocoa framework (which came from the OpenStep specification). *** Larger patches require copyright assignment to FSF. please file bugs here. ***
http://www.gnustep.org
GNU General Public License v3.0
279 stars 103 forks source link

Tabview crash fix #261

Open williameveretteggplant opened 7 months ago

williameveretteggplant commented 7 months ago

I'm seeing a crash from _original_nextKeyView in NSTabView becoming deallocated and becoming a zombie pointer.

The problem is that setNextKeyView: keeps a pointer to the nextKeyView without retaining it. So the view could become deallocated but the pointer is still there. And then when selectTabViewItem: is called it tries to use this pointer and crashes.

When a View becomes deallocated it sets the keyView of its previousKeyView to nil, which would take care of this, except for NSTabView the setNextKeyView: method sometimes calls setNextKeyView: on its _selected view rather than itself, so it will not be the previousKeyView.

This change retains the _original_nextKeyView as long as the pointer value is still being used. Note that the NSView dealloc calls [self setNextKeyView: nil] which has the effect of releasing this object, so it is not necessary to RELEASE _original_nextKeyView in the dealloc method of NSTabView.

williameveretteggplant commented 7 months ago

Would it be possible for you to look at this, @fredkiefer ?

fredkiefer commented 7 months ago

I do understand what problem you are trying to address here. And accessing an object after it was released is not just annoying but also very dangerous. On the other hand is the solution not ideal. Adding a retain to the key view loop is also dangerous. It might result in some sort of retain loop and views never being properly cleaned up. This surely would fix your issue and definitely won't that easily break a program. If possible I would prefer another solution. I have been thinking about removing this variable completely. The idea there would be to instead use the next key view from the super implementation. Before proposing this idea I wanted more time to think about it in detail and maybe experiment with it a little. But somehow the time for this never materialized. I am sorry, so all I have is this untested idea. Maybe you could think it over and perhaps come up with something working?

williameveretteggplant commented 7 months ago

I understand your concern. I can tell you in the branch we have been using, NSTabView.m : 288 [self setNextKeyView: firstResponder]; has been replaced with [super setNextKeyView: firstResponder]; This does seem to prevent the crash, but I hesitate to suggest it as a solution because I don't understand the full ramifications of such a change. Fundamentally, I don't really get the complexity of the next/previous key view logic and don't really know what changes to make that will preserve the functionality while eliminating the problem from the unretained pointer.

gcasa commented 7 months ago

I understand your concern. I can tell you in the branch we have been using, NSTabView.m : 288 [self setNextKeyView: firstResponder]; has been replaced with [super setNextKeyView: firstResponder]; This does seem to prevent the crash, but I hesitate to suggest it as a solution because I don't understand the full ramifications of such a change. Fundamentally, I don't really get the complexity of the next/previous key view logic and don't really know what changes to make that will preserve the functionality while eliminating the problem from the unretained pointer.

Apologies for putting in my $0.02 here, but I am wondering...

Is the issue you are running into here in the application code? Several other apps also use NSTabView in the way you describe and don't have any issues. Gorm, is one example, as well as GWorkspace, Preferences, etc. As we have both witnessed there are instances where GNUstep is a bit stricter (and more correct) than macOS. One example of this is XML parsing. The other is the handling of NSLock. Is this one of those instances?

Are there any places in the code of the application where there is an extraneous release or autorelease?

fredkiefer commented 6 months ago

I would not put the blame on application code, although this is always an option. There are cases where I could see that the current GNUstep code for NSTab might get us into trouble. This involves complicated operations that are not that easy to reproduce. We will need multiple views in the NSTabView, switch between these views and then somehow release the view that is the next in the key view loop after the NSTabView. Doing these operations in the right order might trigger the issue.

My idea to address this would be to replace the value _original_nextKeyView line 286 in NSTabView.m with something like [super nextKeyView] . Of course this might require some other adjustments in the file and the variable could then be removed completely. The hard bit is to test many possible scenarios whether this results in the expected behaviour.