fix(core): fix iterator in backspace handling

[ermshiperete]

This change replaces the reverse iterator loop that holds a stale iterator across pop_back() calls with a pattern that directly accesses context.back() on each iteration. This avoids undefined behavior from iterator invalidation when mutating the list or vector.

While so far the previous code didn't show problems, it might still access released memory depending on the implementation. The documentation for pop_back() says "References and iterators to the erased element are invalidated", so the previous implementation was clearly wrong.

Test-bot: skip

[keymanapp-test-bot]

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

• macOS
  • Keyman for macOS - build : all tests passed (no artifacts on BuildLevel "build")
  • Keyman for macOS (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
• Web
  • KeymanWeb Test Home - build : all tests passed (no artifacts on BuildLevel "build")
• Windows
  • Keyman for Windows - build : all tests passed (no artifacts on BuildLevel "build")
  • FirstVoices Keyboards for Windows - build : all tests passed (no artifacts on BuildLevel "build")
  • FirstVoices Keyboards for Windows (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
  • Keyman for Windows (old PRs) - build : all tests passed (no artifacts on BuildLevel "build")
  • Text Editor (ARM64) - build : all tests passed (no artifacts on BuildLevel "build")
  • Text Editor (x64) - build : all tests passed (no artifacts on BuildLevel "build")
  • Text Editor (x86) - build : all tests passed (no artifacts on BuildLevel "build")

[darcywong00]

The previous code was similar to the code used in production (ldml_process.cpp, ldml_event_state::emit_backspace()). However, in production we use a list where this works, but here in the tests we use a vector where the behavior is undefined.

For reference, I found the code in production:

keyman/core/src/ldml/ldml_processor.cpp

Lines 230 to 246 in 6540886

	void ldml_event_state::emit_backspace() {
	// this is called from user-initiated backspace, not internal backspacing.
	// Find out what the last actual character was and remove it.
	// attempt to get the last char
	// TODO-LDML: emoji backspace
	auto end = state->context().rbegin();
	while (end != state->context().rend()) {
	if (end->type == KM_CORE_CT_CHAR) {
	actions.code_points_to_delete++;
	state->context().pop_back();
	return;
	}
	// else loop again
	assert(end->type != KM_CORE_CT_END); // inappropriate here.
	state->context().pop_back();
	}

Would it be better if tests closely mirrored production? (Could the tests use a list where this works?)

[mcdurdin]

@ermshiperete I am wondering how you found this?

[ermshiperete]

@ermshiperete I am wondering how you found this?

While reading through the code I stumbled upon ldml_event_state::emit_backspace() and wondered if that is correct. I had AI take a look, and it found the similar code in the tests and concluded that wouldn't work since it's using vectors instead of lists. That sounded reasonable since the code works. Turns out I initially misread the code and missed that we're using the reverse iterator.

However, in checking the facts (which I neglected to do initially), it turns out that the code in ldml_event_state::emit_backspace() isn't correct either because popping the list will invalidate the iterator. The pop_back documentation says "References and iterators to the erased element are invalidated."

I'll rework the PR.

[ermshiperete]

Would it be better if tests closely mirrored production? (Could the tests use a list where this works?)

Done. Using list now.

[ermshiperete]

Fixed loop in ldml_event_state::emit_backspace() - which makes it similar to the test loop again 😄