bugGNUstep - Bugs: bug #61727, Premature cleanup in...

Group
 
 

bug #61727: Premature cleanup in NSPopUpButtonCell -dealloc crashes application

Submitter:  Yavor Doganov <yavor>
Submitted:  Thu 23 Dec 2021 03:40:10 PM UTC
   
 
Category:  Gui/AppKit Severity:  3 - Normal
Item Group:  Bug Status:  Fixed
Privacy:  Public Assigned to:  FredKiefer
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 26 Dec 2021 11:03:59 PM UTC, comment #8: 

I confirm it works, thanks very much!

 Yavor Doganov <yavor>
Sun 26 Dec 2021 10:54:02 PM UTC, comment #7: 

I hacked in a workaround for the reported problem. I even have the feeling that I suggested this additional hack when working on the original change together with Riccardo. But at that time tests seemed to show that this was not needed.
Could you please try and report back if this workaround is sufficient for all known issues?

 Fred Kiefer <FredKiefer>
Group Member
Sun 26 Dec 2021 02:59:12 PM UTC, comment #6: 

Please ignore my comment. It was affected by a dirty gdb environment .

 Sergei Golovin <svg>
Sun 26 Dec 2021 04:08:17 AM UTC, comment #5: 

It is a "document-based" application. On start it opens an "untitled"
document of a default type. The default type is "XML TAMS Project" for
which the "myProject" is the NSDocumentClass. The only instance of the
"myProject" created from the Nib-file has been already broken when its
window is showed. So closing application/window leads to fault.

I didn't follow the source code deeply and can't say where an extra
release happens.


> A workaround in the app would be to retain self in myProject -init but that's a memory leak and not really a bug fix.


Not a bad idea. The application is full of memory leaks anyway.


Sergei Golovin <svg>
Fri 24 Dec 2021 10:12:59 AM UTC, comment #4: 

Actually I think the real goal of the commit was not to mess with notifications. Unluckily I did not track the problematic application in the comment.
A lot of other projects have nspopupbuttons... why don't I get a crash there? More than a year passed and I tested all applications
We need Fred.
Also, it would be best to use the current github bug tracker, thisone here is just to cleanup the list of legacy bugs.

 Riccardo Mottola <rmottola>
Group Member
Fri 24 Dec 2021 07:06:59 AM UTC, comment #3: 

I always suspect a bug in the application, especially if it's old code with dead upstream as is the case.  I spent some time studying the code, trying to fix some obvious bugs, etc.  It took me a while to come to the conclusion that the crash is caused by that commit.  Running with NSZombieEnabled confirms the theory from the code comment:


$ NSZombieEnabled=YES GTAMSAnalyzer
2021-12-24 08:58:13.907 GTAMSAnalyzer[57081:57081] NSDocumentClass Document not found
2021-12-24 08:58:18.922 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.922 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.923 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.923 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.925 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.925 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.925 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0
2021-12-24 08:58:18.925 GTAMSAnalyzer[57081:57081] *** -[myProject respondsToSelector:]: message sent to deallocated instance 0x557026edb7d0


Closing the project window deallocates it but it also updates the menus which in turn try to access the target that no longer exists, exactly as the comment says.  A workaround in the app would be to retain self in myProject -init but that's a memory leak and not really a bug fix.

 Yavor Doganov <yavor>
Fri 24 Dec 2021 01:28:52 AM UTC, comment #2: 

The commit was heavily analyzed with Fred - it was causing a bad crash, but I don't remember where, to be honest, but the fix should be a good one, it is also heavily tested in a plethora of applications, I have no crashes.
It could be an issue in gtamsanalyzer.app?

Right now gui has a very strange bug in popupbuttons, but I don't think it is related at all. Maybe also Fred can chime in here?

 Riccardo Mottola <rmottola>
Group Member
Thu 23 Dec 2021 07:54:53 PM UTC, comment #1: 

Commit viewable in web UI: https://github.com/gnustep/libs-gui/commit/b7f5fb2edf138fbccfa3c65724279674c314dcee

Riccardo, can you take a look?

 Ivan Vučica <ivucica>
Group Member
Thu 23 Dec 2021 03:40:10 PM UTC, original submission:  

GTAMSAnalyzer crashes with GUI 0.29; backtrace at https://bugs.debian.org/1001537.  Cannot reproduce with earlier GUI versions.  Relevant valgrind output:


==6853== Process terminating with default action of signal 11 (SIGSEGV)
==6853==  Access not within mapped region at address 0xDEADFB0E
==6853==    at 0x569CD55: objc_msg_lookup (sendmsg.c:442)
==6853==    by 0x4AD1DBA: _i_NSApplication__targetForAction_to_from_ (NSApplication.m:2294)
==6853==    by 0x4B93B67: _i_NSMenu___autoenableItem_ (NSMenu.m:1179)
==6853==    by 0x4B98D36: _i_NSMenu__update (NSMenu.m:1255)
==6853==    by 0x4BBE5E0: _i_NSPopUpButtonCell__setMenuItem_ (NSPopUpButtonCell.m:640)
==6853==    by 0x4BBEDEB: _i_NSPopUpButtonCell__synchronizeTitleAndSelectedItem (NSPopUpButtonCell.m:842)
==6853==    by 0x4BBFA1A: _i_NSPopUpButtonCell__dealloc (NSPopUpButtonCell.m:152)
==6853==    by 0x4B2B1C0: _i_NSControl__dealloc (NSControl.m:125)
==6853==    by 0x4C46BDB: _i_NSView__removeSubview_ (NSView.m:965)
==6853==    by 0x4C55B6F: _i_NSView__dealloc (NSView.m:745)
==6853==    by 0x4C46BDB: _i_NSView__removeSubview_ (NSView.m:965)
==6853==    by 0x4C55B6F: _i_NSView__dealloc (NSView.m:745)


If I revert commit b7f5fb2, the problem goes away.  I think what is happening is exactly as described in the code comment which was deleted in that commit:


/*
 * We don't use methods here to clean up the selected item, the menu
 * item and the menu as these methods internally update the menu,
 * which tries to access the target of the menu item (or of this cell).
 * When the popup is relases this target may already have been freed,
 * so the local reference to it is invalid and will result in a
 * segmentation fault.
 */


 Yavor Doganov <yavor>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by FredKiefer (Posted a comment)
  • -email is unavailable- added by svg (Posted a comment)
  • -email is unavailable- added by rmottola (Posted a comment)
  • -email is unavailable- added by ivucica (Updated the item)
  • -email is unavailable- added by yavor (Submitted the item)

    •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-27 FredKiefer StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-12-26 FredKiefer StatusNone Ready For Test
        Assigned tormottola FredKiefer
    2021-12-23 ivucica Assigned toNone rmottola

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.14-3b9d.
    Corresponding source code