fix: stop leaking a JNI global ref per ArrayBuffer passed to Java

[dpwhittaker]

Description

When a JS ArrayBuffer, SharedArrayBuffer or typed array crosses to Java for the first time, JsArgConverter::ConvertArg wraps it in a direct ByteBuffer, then calls env.NewGlobalRef(buffer) and never deletes the result. JsArgToArrayConverter::ConvertArg has the same code, and is used for Java array elements, overload resolution and values returned to Java. GetOrCreateObjectId already registers the buffer in the runtime's strong instances for as long as the JS object lives, so the extra reference does nothing except pin the buffer.

Two consequences:

• Every distinct buffer stays in the JNI global reference table for the life of the process. ART aborts once the table holds 51,200 entries: JNI ERROR (app bug): global reference table overflow (max=51200), and the dump shows ~50,000 java.nio.DirectByteBuffer entries.
• The ArrayBuffer behind each pinned buffer is never freed either. JSObjectFinalizer keeps a JS object alive while its Java counterpart is alive, and the pinned DirectByteBuffer never dies.

Passing the same ArrayBuffer again doesn't leak, because the existing link is reused. So the leak is one reference per distinct buffer. That matters for any app that hands Java a fresh buffer per event. We hit it in an app that passes each display frame (pixels plus glyph data) to Java as new ArrayBuffers. It aborted after 2.75 to 10.6 hours of use, at 1.4 GB RSS.

The fix removes the NewGlobalRef call in both converters.

Does your pull request have unit tests?

Yes. Two tests in byte-buffer-test.js each pass 60,000 fresh buffers to Java, more than the table can hold: one as method arguments (alternating ArrayBuffer and Uint8Array), one as Java array elements.

Run on a Galaxy Z Fold7 (arm64, Android 17) with the runtime built from this branch:

	Unpatched	This branch
New tests	test app aborts with the overflow (50,398 DirectByteBuffer global refs)	pass, ~0.5 s each
Full test-app suite	1242 tests, 0 failures (without the new tests)	1244 tests, 0 failures

Also checked in the affected app, by heap-dumping it and counting JNI global roots:

• Fresh buffers: after passing 1,000 fresh buffers to Java, there are no new JNI-global DirectByteBuffers. Unpatched, there were +1,000, and they survived a forced gc().
• Collection: after JS and Java GC cycles, the 1,000 buffers are collected. Unpatched, they never were.
• Contents: 60,000 fresh buffers with known contents hash correctly on the Java side.
• Kept buffers: a buffer kept alive in JS still hashes correctly after the GC cycles.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of large numbers of JavaScript buffers passed to Java, preventing them from exhausting the runtime’s reference limit.
  • Buffers remain accessible to Java while their JavaScript objects are still in use.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9a041bc9-9928-4d0b-a0dc-64bfbdd8a735

📥 Commits

Reviewing files that changed from the base of the PR and between 9b12329 and 4918314.

📒 Files selected for processing (3)

• test-app/app/src/main/assets/app/tests/byte-buffer-test.js
• test-app/runtime/src/main/cpp/JsArgConverter.cpp
• test-app/runtime/src/main/cpp/JsArgToArrayConverter.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

Both JavaScript-to-Java buffer conversion paths now register buffers with ObjectManager without first creating JNI global references. New stress tests exercise repeated buffer conversions and check the resulting Java-held values.

Changes

Buffer JNI reference handling

Layer / File(s)	Summary
Buffer registration without extra JNI references test-app/runtime/src/main/cpp/JsArgConverter.cpp, test-app/runtime/src/main/cpp/JsArgToArrayConverter.cpp, test-app/app/src/main/assets/app/tests/byte-buffer-test.js	Both conversion paths pass buffers to GetOrCreateObjectId without first creating a JNI global reference. Tests pass 60,000 alternating ArrayBuffer and Uint8Array values to a Java holder, then assign 60,000 distinct buffers to a Java array and check the final values.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to 49183

This change stops buffers passed to Java from piling up as JNI global references, so the global-reference table should no longer overflow. No merge-blocking risk was found.

Architecture Summary

Architecture risk: 🔵 Low · up to 49183

The change affects 1 system.

Changed systems: test-app

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — test-app (service) was modified; 3 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in test-app/app/src/main/assets/app/tests/byte-buffer-test.js: Added JNI-reference stress tests: one passes 60,000 alternating ArrayBuffer and Uint8Array instances to a Java holder and checks the held value; the other stores 60,000 distinct buffers successively in a Java array and checks its final entry. The comment states ART’s global-reference table limit is 51,200.
• observed — Modified behavior in test-app/runtime/src/main/cpp/JsArgConverter.cpp: Removed NewGlobalRef(buffer) before object registration. The buffer is now passed directly to GetOrCreateObjectId; comments state the manager retains it until JS collection and that a global reference would not be deleted.
• observed — Modified behavior in test-app/runtime/src/main/cpp/JsArgToArrayConverter.cpp: Removed the NewGlobalRef call for the Java buffer. The comments now describe its reachability through GetOrCreateObjectId and the pinning and JNI global-reference-table effects of retaining an undeleted global reference.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: removing a leaked JNI global reference for each ArrayBuffer passed to Java.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit hops past buffers bright
Sixty thousand bound for flight
No extra global refs to keep
Java holds what it must reap
The last one waits, ears upright}

_{Comment @coderabbitai help to get the list of available commands.}