Fix crash on launch from strict plist field reads - #43
Merged
Conversation
rave rebuilt its ninja file whenever a watched directory changed, so adding a source or a test file was picked up by the next build. CMake's file(GLOB) runs once, at configure time, and nothing rechecks it: a new file is silently ignored until someone re-runs cmake by hand. That is worse than an inconvenience for tests, where the failure is invisible. A test file added to Frameworks/<name>/tests is simply not compiled into the runner, and the suite reports success without it — the suite passes because the test is not there. CONFIGURE_DEPENDS makes ninja recheck each glob and re-run cmake when the matches change. Claude-Session: https://claude.ai/code/session_01Gi3HR9ioH4wfrn4BWvJUDA
TextMate aborted on launch while restoring a session: an uncaught
std::bad_variant_access escaped NSApplicationMain, and main()'s handler calls
abort(). It came from parse::grammar_t reading a grammar whose keys do not hold
the types parse::rule_t maps them to.
Nothing validates a plist before it reaches a schema, and grammars and themes
in the wild do carry a numeric `name` or a string `disabled`. TextMate has
always coerced those rather than rejecting the file. The coercion lived in
plist::get<T>(any_t const&), which ran a converting visitor and yielded a
default when no conversion applied; its ASSERT was debug-only, so release
builds carried on.
Moving any_t off boost::variant renamed that function to plist::convert<T> and
gave plist::get<T> to the strict, boost::get-compatible accessors. Call sites
that had relied on the lenient behaviour kept the old spelling and silently
became strict, so a type mismatch that used to coerce now throws. This restores
plist::convert<T> at each of them: the schema field handlers, theme colour and
font reads, and the `disabled` check in parse::convert_plist.
Two related repairs, both consequences of the same rename:
- The strict overload taking any_t const& is gone. Every read of a const
any_t wants convert<T>, and returning a reference into an any_t temporary —
which several tests did, via plist::parse_ascii(...) — is a dangling
reference. Making the const case a compile error forces the choice to be
explicit; the sites that genuinely need strict access take the pointer
overload and check for null, which is the boost idiom anyway.
- delta.cc and item.cc re-read a value strictly having already proved its
type with the pointer form. They now reuse that pointer.
Frameworks/plist/tests/t_schema.cc covers the schema paths. It builds the
mismatched types directly, because the ASCII plist parser reads every unquoted
token as a string and so cannot produce them — the real ones arrive from XML
and binary plists through CFPropertyList. Reverting the schema change alone
fails it with bad_variant_access.
Claude-Session: https://claude.ai/code/session_01Gi3HR9ioH4wfrn4BWvJUDA
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.
The build from
mainaborts on launch:main()catchesstd::exceptionescapingNSApplicationMainand callsabort(). Under lldb the exception isstd::bad_variant_access, thrown fromplist::schema_t<parse::rule_t>::variant_field_t::handlewhileparse::grammar_treads a grammar during session restore.Cause
Nothing validates a plist before it reaches a schema, and grammars and themes in the wild carry keys whose types don't match what the schema maps them to — a numeric
name, a stringdisabled. TextMate has always coerced those rather than rejecting the file.The coercion lived in
plist::get<T>(any_t const&), which ran a converting visitor and yielded a default when no conversion applied. ItsASSERTwas debug-only, so release builds carried on:Moving
any_toffboost::variant(in "Remove boost, google-sparsehash and ragel build dependencies") renamed that function toplist::convert<T>and handed the nameplist::get<T>to the strict,boost::get-compatible accessors. Call sites relying on the lenient behaviour kept the old spelling and silently became strict — so a mismatch that used to coerce now throws.This restores
plist::convert<T>at each of them: the two schema field handlers, theme colour and font reads, and thedisabledcheck inparse::convert_plist.Two related repairs
Both fall out of the same rename:
any_t const&is removed. Every read of a constany_twantsconvert<T>, and returning a reference into anany_ttemporary — which several tests did, viaplist::parse_ascii(...)— is a dangling reference. Making the const case a compile error forces the choice to be explicit. Sites that genuinely need strict access take the pointer overload and check for null, which is the boost idiom anyway. Removing it immediately surfaced four more call sites.delta.ccanditem.ccre-read a value strictly having already proved its type with the pointer form. They now reuse that pointer.Regression test
Frameworks/plist/tests/t_schema.ccbuilds the mismatched types directly, because the ASCII plist parser reads every unquoted token as a string and so cannot produce them — the real ones arrive from XML and binary plists throughCFPropertyList. My first attempt went throughparse_asciiand passed against the broken code, which is why the test is written at this level.Reverting the schema change alone fails it:
Second commit: globs
While writing that test I found it wasn't running.
file(GLOB)runs once at configure time, and nothing rechecks it, so a new test file is not compiled into the runner and the suite reports success without it — the suite passes because the test isn't there. rave regenerated its ninja file whenever a watched directory changed.CONFIGURE_DEPENDSon all 75 globs makes ninja recheck them and re-run cmake when the matches change.Verification
Clean configure and build, full CTest run: the same six suites fail as before (
cf,io,buffer,scm,file,network), all environmental and failing identically under rave. No new failures. The built app launches and restores the session without crashing.