Skip to content

fix(json2): preserve values when type allocation fails - #7977

Open
Ondra97 wants to merge 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-json2-type-allocation
Open

Ondra97 wants to merge 1 commit into
boykopovar:mainfrom
Ondra97:codex/fix-json2-type-allocation

Conversation

@Ondra97

@Ondra97 Ondra97 commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

What

Value::set(ValueType) clears the existing value and publishes the requested type before allocating its empty String, Array or Object storage. If allocation throws, the destination loses its previous contents and retains a type with null storage.

Construct the replacement storage before clearing the destination, then transfer it while preserving the per-value handlers. Extend the existing JsonCopyFailure fixture with the three type setter paths, checking that failures preserve the old type and serialized value and release all tracked allocations.

The author of #7477 confirmed that either merge order works. I verified that its current head 2a1ca278eefad1e84aff2801e44442fdb406d12d leaves SetType and JsonCopyFailure.cpp identical to main and combines cleanly with this patch.

The overlap tool reports #7215 as a shared file it cannot compare because that branch conflicts with main. I compared its diff manually: it changes numeric conversion in RealText and Parser::number, plus includes, and does not change SetType or the allocation failure fixture. There is no dependency on either PR.

Tested

Tested revision: 51e75dec2591d1b884c541f48cdfdd2431bdf0eb, based on d047690c6391577c9add655ae1dc2ed20df177a8. Full native Windows configuration: Windows 11 Pro 10.0.26300, WinLibs MinGW-w64 GCC 15.2.0, Release/O3, BUILD_TESTING=ON.

Hardware: Ryzen 9 7900X, 32 GB RAM, GeForce RTX 4070 Ti SUPER.

  • Full Windows build passed after rebasing.
  • guest_json, guest_json_copy_failure and patched_libraries: 3/3 passed, no skips.
  • The allocation failure fixture completed 20 operations and 312 injected failures with no tracked leaks; 352 patched libraries loaded, 0 failed.
  • The same regression source failed on the original production code at set empty array budget=0: failed setter changed the destination type (base bcdee66d109008334b07c0ff73a27e845708675f). The JSON implementation and original fixture are byte-identical in that base and the rebased base.
  • python3 tools/check_conventions.py --base origin/main passed.
  • python3 tools/pr_overlap.py --branch codex/fix-json2-type-allocation completed; its remaining fix(json2): keep numeric conversion independent of host locale #7215 warning was reviewed as described above.

Linux, PRAGMATA gameplay and the complete CTest suite were not run.

Reproduction commands, using build as the build directory and the documented toolchain on PATH:

cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++
cmake --build build --parallel 2
ctest --test-dir build -R '^(guest_json|guest_json_copy_failure|patched_libraries)$' -V --output-on-failure --no-tests=error
python3 tools/check_conventions.py --base origin/main

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed the exception-safety path at the current head. SetType now validates before mutation, constructs the replacement payload before clearing the old node, and transfers the existing handlers after Clear preserves them. The new allocation-failure cases check both the previous type and serialized value for string, array, and object setters, which covers the destructive failure mode. I did not independently run the reported Windows build/tests; the PR description identifies the tested revision and notes Linux/gameplay were not run.

@github-actions

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 7215 core/libs/prx/libSceJson2/Export.cpp

@oneandonlydean oneandonlydean self-assigned this Oct 20, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants