Opened 5 years ago

Closed 5 years ago

#15189 closed defect (fixed)

wxRichTextCtrlTable: deleting the focused cell crashes

Reported by: dghart Owned by:
Priority: normal Milestone: 2.9.5
Component: wxRichText Version: stable-latest
Keywords: wxRichTextCtrlTable delete cell crash Cc:
Blocked By: Blocking:
Patch: yes


The attached diff to the 'richtext' sample makes running the 'About' dialog delete the last column of the table. Run the sample, click in one of the cells in that column, then do Help > About. The sample will segfault. The exact details vary depending with the calling code (and probably the machine and platform) but it occurs because the focus object is no longer valid when later calls (e.g. setting the caret) use it as 'container'. This happens in the current svn trunk but also in 2.9.4, so it's not the result of recent patches.

I don't know how to fix this. I don't think there's any way for later calls to predict that the object is disappearing. Always setting focus outside the table would probably work, but isn't user-friendly. The correct answer would be to detect that the focused cell is about to be killed and set focus elsewhere in the table. However afaict there's no way to tell which cell _has_ focus: the cell itself doesn't know, and e.g. GetCaretPosition() returns only the position within the cell. I hope I'm missing something...

Attachments (3)

patch1.diff download (6.6 KB) - added by dghart 5 years ago.
patch2.diff download (6.3 KB) - added by dghart 5 years ago.
richtext.diff download (2.9 KB) - added by dghart 5 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 5 years ago by dghart

  • Patch set

I hope I'm missing something...

Indeed I was. It's possible to do this by comparing each cell with GetFocusObject(), and I've added a new function, wxRichTextCtrlTable::GetFocusedCell. Using this it's easy to set focus to a cell that will still exist, and I've supplied two alternative patches that implement this.

patch1.diff changes the focused cell as above, but if all the columns or rows are to be deleted it tries to set focus outside the table. Debugging shows that this works at the time. However there follows a call to wxRichTextAction::Do -> wxRichTextAction::UpdateAppearance, which starts by setting focus back to the container. As a result crashes may still occur though, as it's a race condition, often they don't.

In any case I don't think it's sensible to allow the last column or row in a table to be deleted. It leaves the table looking very odd, and it's unlikely to be what the user actually wants. So my preferred alternative is patch2.diff which prevents this happening. It also adds an assert to warn the programmer, though you may feel that's excessive.

I've updated richtext.diff to show things working. Now running the 'About' dialog deletes the column containing the focused cell, or the last column if none are. Similarly Insert > Symbol deletes a row. Note that crashes may still occur due to #15186.

Changed 5 years ago by dghart

Changed 5 years ago by dghart

Changed 5 years ago by dghart

comment:2 Changed 5 years ago by vadz

Julian, same question: do you have any objections to applying patch1.diff and patch2.diff?

comment:3 Changed 5 years ago by juliansmart

  • Resolution set to fixed
  • Status changed from new to closed

Sorry about the delay, I've now applied patch2.diff (I believe it was meant to supercede patch1.diff). Many thanks!

Note: See TracTickets for help on using tickets.