Skip to content

Fix DOM parse() lifetime on the Vector backend (B17)#30

Merged
devcrocod merged 1 commit into
masterfrom
fix/b17-dom-eager-materialization
Jul 6, 2026
Merged

Fix DOM parse() lifetime on the Vector backend (B17)#30
devcrocod merged 1 commit into
masterfrom
fix/b17-dom-eager-materialization

Conversation

@devcrocod

Copy link
Copy Markdown
Owner

Summary

Audit finding B17: on the Vector backend, parse() returned lazy JsonObject/JsonArray views over the parser's single reused Tape + stringBuffer. A subsequent parse()/iterate() on the same parser overwrote those buffers in place, silently corrupting previously returned DOM values (reproduced: ClassCastException, stale size) — while the JsonValue KDoc promises "Immutable and safe to share across threads" and the parser KDoc recommends reuse. JNI/Native already materialized eagerly, so the contract also diverged across backends.

Changes

  • Eager materialization: TapeValueMaterializer moves commonMain → jvmMain and recursively builds list-backed JsonObject(entries)/JsonArray(elements) — the same shapes JNI/Native produce. TapeBuilder.createJsonValue() unchanged.
  • Dual-mode removal: JsonObject/JsonArray lose the tape-view representation and become plain list wrappers; public surface (size, get, keys, contains, iterator) unchanged. Net −38 lines.
  • Tape.kt moves commonMain → jvmAndAndroidMain (its only users are TapeBuilder/TapeValueMaterializer in jvmMain and NumberParser in jvmAndAndroidMain).
  • KDoc: both parse() overloads now state the guarantee — the returned tree is fully materialized and independent of the parser, valid after subsequent parse/iterate/close. On-Demand (iterate()) keeps its documented borrow semantics.
  • Regression tests (commonTest, run on every backend): deep re-read of an old result after reparse, DOM survival across iterate() (string root deliberately clobbers the shared stringBuffer), DOM survival after close().

Testing

  • Red phase confirmed: the new reuse/iterate tests failed on Vector (NPE / assertion) and passed on JNI before the fix.
  • Green: jvmTest 536/536, jvmTest128 536/536, jvmTestJni, macosArm64Test, testAndroidHostTest, :simdjson-kotlin-serialization:allTests — all passing locally; Linux/iOS native covered by CI.

Trade-off: Vector DOM parse now allocates the object graph (the cost JNI/Native already pay; Jackson-tree equivalent). The zero-alloc "read a few fields" use case remains served by iterate().

The Vector backend returned lazy tape-backed views over parser-owned
buffers, so reusing the parser silently corrupted previously returned
JsonObject/JsonArray. Materialize the DOM eagerly (as JNI/Native already
do), drop the dual tape/list representation from JsonObject/JsonArray,
move Tape out of commonMain, document the materialization guarantee in
parse() KDoc, and lock the contract with cross-backend regression tests.
Copilot AI review requested due to automatic review settings July 6, 2026 11:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness/lifetime issue in the Vector (pure Kotlin) backend where parse() previously returned lazy DOM views over a parser-reused tape + string buffer, causing previously returned JsonObject/JsonArray values to become silently corrupted after subsequent parse()/iterate() calls. The change aligns Vector DOM behavior with JNI/Native by eagerly materializing an independent object graph and updates tests/KDoc to enforce the intended immutability and parser-reuse contract.

Changes:

  • Eagerly materialize Vector DOM results into list-backed JsonObject/JsonArray structures so results survive parser reuse, iterate(), and close().
  • Remove the tape-view representation from JsonObject/JsonArray (DOM now consistently list-backed across backends).
  • Add cross-backend regression tests covering parser reuse, DOM survival across iterate(), and DOM survival after close(); update parse() KDoc to state the guarantee.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
simdjson-kotlin/src/jvmMain/kotlin/io/github/devcrocod/simdjson/TapeValueMaterializer.kt New JVM Vector materializer that recursively builds list-backed DOM nodes from the tape + string buffer.
simdjson-kotlin/src/jvmAndAndroidMain/kotlin/io/github/devcrocod/simdjson/Tape.kt Moves Tape to the JVM/Android source set to match actual usage and support the new JVM materialization flow.
simdjson-kotlin/src/commonTest/kotlin/io/github/devcrocod/simdjson/DomParsingTest.kt Adds/expands regression tests to ensure DOM results remain valid across reuse, iterate, and close.
simdjson-kotlin/src/commonMain/kotlin/io/github/devcrocod/simdjson/TapeValueMaterializer.kt Removes common materializer that previously produced tape-backed JsonObject/JsonArray views.
simdjson-kotlin/src/commonMain/kotlin/io/github/devcrocod/simdjson/SimdJsonParser.kt Updates parse() KDoc to explicitly guarantee returned DOM independence from the parser lifecycle.
simdjson-kotlin/src/commonMain/kotlin/io/github/devcrocod/simdjson/JsonValue.kt Simplifies JsonObject/JsonArray to plain list wrappers (no tape-view mode).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (_entries != null) return _entries.iterator()
return TapeObjectIterator()
}
override fun iterator(): Iterator<Pair<String, JsonValue>> = entries.iterator()
Comment on lines +48 to 49
override fun iterator(): Iterator<JsonValue> = elements.iterator()
}
@devcrocod devcrocod merged commit 47263f5 into master Jul 6, 2026
10 checks passed
@devcrocod devcrocod deleted the fix/b17-dom-eager-materialization branch July 6, 2026 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants