fix(datagrid): resolve header comments from the header view to stop a sort crash (#1869)#1870
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1869.
Clicking a column header crashes the app with
EXC_BAD_ACCESS(SIGSEGV) at a garbage address, inside-[NSTextFieldCell dealloc]duringNSDisplayCycleFlush. It reproduces on any table whose last column has a comment longer than 15 UTF-8 bytes.Root cause
SortableHeaderCellis the repo's onlyNSCellsubclass. In 0.56.2 (#1842) it gainedheaderComment: String?, its first reference-counted stored property.NSCell.copyWithZone:is a legacyNSCopyObjectshallow copy: it bit-copies subclass ivars and re-retains only the ones the Objective-C runtime can see.class_getIvarLayoutreturns NULL for Swift classes, so the Swift ivar is pointer-copied with no retain, while the copy's ARC.cxx_destructstill releases it on dealloc.AppKit copies the header cell while drawing (
NSTableHeaderView._preparedHeaderFillerCell, reached fromdrawRect:and from vibrancy updates). A header click dirties the header, the copy is made and autoreleased, and it dies atobjc_autoreleasePoolPopinside the display cycle, releasing a string it never retained. The real cell is then left pointing at freed memory.Comments shorter than 16 UTF-8 bytes live inline in the
Stringwith no heap object, which is why this only fires on tables with real column comments. Apple'sNSTableColumndocs require a customheaderCellto "properly implement-copyWithZone:"; we never did.This is live in 0.57.0 as well, and #1863 makes comments populate earlier, so it fires more readily there than in 0.56.2.
The fix
Rather than compensating for the broken copy with a retain, the comment moves off the cell entirely, so the bug class is gone:
SortableHeaderCellnow holds only trivial state (a raw-value enum,Int?,Bools). With nothing reference-counted,NSCopyObjecthas nothing to get wrong no matter how often AppKit copies it.SortableHeaderView(anNSView, not subject toNSCopyObject) owns the comments keyed by column identifier. The cell resolves its comment fromcontrolViewat draw time.DataGridColumnPoolpublishes comments to the header view, and now bakes the comment into the header's accessibility label (it already built the tooltip from the same value), so moving the comment off the cell costs no accessibility.A copied cell belongs to no column, so it resolves no comment and the overflow filler cell cannot draw a stray one.
Tests
The existing tests could not have caught this: they assigned comments from string literals, which are immortal and never reference-counted. The new tests build the comment at runtime so a real heap object is exercised, then copy the cell the way AppKit does.
All 26 tests across
SortableHeaderCellTestsandDataGridColumnPoolTestspass;swiftlint lint --strictis clean on the changed files.Verification note
Tests cover comment resolution and copy safety, but not drawing. Worth one manual pass on a table with commented columns to confirm the comment still renders in the header and the header still grows to fit it.
https://claude.ai/code/session_01P7qP2VjoTKfq4Nh19roxtu