bugGNUstep - Bugs: bug #25612, NSMatrix Segmentation Fault when...

Group
 
 

bug #25612: NSMatrix Segmentation Fault when clicking a cell

Submitter:  Doug Simons <theeggcamefirst>
Submitted:  Tue 17 Feb 2009 07:18:17 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
   

Wed 18 Feb 2009 11:09:58 AM UTC, comment #4: 

I made the change in the initWithCoder: method. This should fix the underlying problem. I am still interested to learn what the original NIB file was setting.

Fred Kiefer <FredKiefer>
Group Member
Wed 18 Feb 2009 08:27:08 AM UTC, comment #3: 

Your change highlights the similarities between the code in the two methods _selectCell:atRow:column: and setState:atRow:column:. These methods surely should share more code as in some cases they do almost the same thing.

I would say that your patch works around the current problem, we wont have a segmentation fault any more, but the internal inconsistency still exists for some time (From the NIB loading until the mouse down). The problem with this approach is that we may just face the same issue in other places and when we start to protect all of them from this inconsistency our code may get cluttered.

For this reason I would prefere to stick with the current invariant:

((_selectedCell == nil) && (_selectedRow == -1) && (_selectedColumn == -1)) ||
(_selectedCell == [self cellAtRow: _selectedRow column: _selectedColumn])

This condition will be implicitly true after any of the methods on NSMatrix has run. (And apart from the init methods also before all methods)

The code you changed was correct. The _selectCell:atRow:column: already checked that _selectedCell isn't nil so it "knows" that _selectedRow and _selectedColumn aren't nil!

What we have to guarranty in our code is that this invariant is really valid all the time and the only possible violation I found was in initWithCoder:, so we need to fix that. With the original NIB file at hand it would be easy to see what is going wrong here. Without it we just need to make sure we handle every possible case correctly. This is best done by calling selectCellAtRow:column: from initWithCoder: with the values read from the NIB file.


Fred Kiefer <FredKiefer>
Group Member
Wed 18 Feb 2009 04:52:44 AM UTC, comment #2: 

I wasn't aware of this bug before I made the following change...

I have added code to NSMatrix.m's method:

- (void) _selectCell: (NSCell *)aCell atRow: (int)row column: (int)column

This should prevent our attempting to set the value as such:

  _selectedCells[_selectedRow][_selectedColumn] = NO;

when _selectedRow and _selectedColumn are -1.  The code is here at line 1188:
         
-->       if (_mode == NSRadioModeMatrix && _selectedRow > -1 && _selectedColumn > -1)
            {

It is the similar to the code at line 1034:

  if (_mode == NSRadioModeMatrix)
    {
      if (value)
{
-->   if (_selectedRow > -1 && _selectedColumn > -1)
    {

Please take a look at the code and let me know if there's a problem.  I will also look at nib loading to see if there's a problem there.

Thanks, GC

Gregory John Casamento <gcasa>
Group administrator
Tue 17 Feb 2009 09:35:24 PM UTC, comment #1: 

That's an interesting problem. How could the selected cell be set without the two seletion indexes pointing at the same cell?

I went through all code that sets the ivar _selectedCell and I would say that all of them are correct, whenever we set the _selectedCell _selectedColumn and _selectedRow also get proper values assigned.

This means it must be the other way around. We set the the indexes somewhere to -1 and the cell isn't unset.
Now here you already hint to a place where this may happen: NIB decoding. We first select all the cells where the state is on and then separately set the indexes. When these indexes in your NIB file are -1 then things will go wrong.

We could now argue that the NIB file doesn't keep with our internal invariant condition, but there is no use to it, we cannot force Apple to do any better programming, we have to live with what they do.
Best we change the lines in initWithCoder: that set the indexes to call selectCellAtRow:column:

Any differing opinion?

Fred Kiefer <FredKiefer>
Group Member
Tue 17 Feb 2009 07:18:17 PM UTC, original submission:  

I'm consistently getting a Segmentation fault when I click a field in a matrix. Here's the backtrace:

#0  0xb7cef6ee in -[NSMatrix _selectCell:atRow:column:] (self=0x92d88c8, _cmd=0xb7ec6b60,
    aCell=0x92e17d8, row=0, column=0) at NSMatrix.m:1190
#1  0xb7cf3d09 in -[NSMatrix mouseDown:] (self=0x92d88c8, _cmd=0xb7f184d0, theEvent=0x84f8a68)
    at NSMatrix.m:2518
#2  0xb7dc8533 in -[NSWindow sendEvent:] (self=0x92f3728, _cmd=0xb7e7e400, theEvent=0x84f8a68)
    at NSWindow.m:3586
#3  0xb7c3278e in -[NSApplication sendEvent:] (self=0x842e428, _cmd=0xb7e7e340, theEvent=0x84f8a68)
    at NSApplication.m:1951
#4  0xb7c3128a in -[NSApplication run] (self=0x842e428, _cmd=0xb7e73980) at NSApplication.m:1455
#5  0xb7c12ec1 in NSApplicationMain (argc=1, argv=0xbfd5ae04) at Functions.m:74
#6  0x0804fd32 in main (argc=1474660693, argv=0xec835356) at main.m:13

Line 1190 in NSMatrix.m looks like this:

    _selectedCells[_selectedRow][_selectedColumn] = NO;

Since both _selectedRow and _selectedColumn have a value of -1, I'm guessing this is the cause of the crash. ;-)

I'm not sure what the correct change to the logic should be, but if I'm not mistaken, it's pretty normal to have -1 for selected values when nothing is selected. This interface was freshly loaded from a nib file with nothing selected. Let me know if you need any more information. I'm running the latest from svn (rev 27904).


Doug Simons <theeggcamefirst>

 

(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 gcasa (Posted a comment)
  • -email is unavailable- added by FredKiefer (Posted a comment)
  • -email is unavailable- added by theeggcamefirst (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
    2009-07-20 FredKiefer StatusReady For Test Fixed
        Open/ClosedIn Test Closed
    2009-02-18 FredKiefer StatusNone Ready For Test
        Assigned toNone FredKiefer
        Open/ClosedOpen In Test

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code