Uh oh!
There was an error while loading. Please reload this page.
[native] Fix a JNI reference leak and drop new[] from jstring_array_wrapper - #12568
Conversation
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 | src/native/common/include/runtime-base/jni-wrappers.hh — 🤖 ❌ error Native memory — len * sizeof (jstring_wrapper) can overflow on 32-bit ABIs (since… |
What changed in this PR
This PR updates the native JNI wrapper utilities to (1) correctly release JNI local references even when UTF chars were never requested, and (2) remove new[]/delete[] usage from jstring_array_wrapper to help reduce C++ runtime symbol dependencies as part of the “drop-libc++” effort.
Changes:
- Fix
jstring_wrapper::release()so it always releases the JNI reference, independently of whether UTF chars were fetched. - Replace
new[]/delete[]allocation injstring_array_wrapperwithmalloc()+ placement-new construction + explicit destruction. - Add
<new>include to support placement new.
| File | Description |
|---|---|
src/native/common/include/runtime-base/jni-wrappers.hh | Fixes JNI local ref lifetime handling and replaces new[] with malloc() + placement new to reduce C++ runtime dependencies. |
Uh oh!
There was an error while loading. Please reload this page.
…rapper `jstring_array_wrapper::operator[]` fetches the element's JNI reference on first access, but the UTF characters behind it are only fetched later, when something calls `get_cstr ()`. `jstring_wrapper::release ()` bailed out early whenever `cstr` was null, so an element that was indexed but never read kept its local reference until control returned to Java. Release the characters and the reference independently instead. The overflow storage used `new jstring_wrapper[]`/`delete[]`, which is where `operator new[]` and `operator delete[]` entered `host.cc`. Allocate the array with `malloc ()` and run the constructors and destructors explicitly. Placement new is a compile-time construct, so it does not pull anything in from libc++. This removes `_Znam` and `_ZdaPv` from `host.cc`, taking the CoreCLR libc++ reference count from 23 down to 21. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
212b1fb to
98e08b1Comparee808135 to
54a7fa6CompareCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reject invalid array lengths and out-of-range access instead of returning a mutable sentinel that cannot release assigned JNI references. Document reference and storage ownership and simplify reassignment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
simonrozsival
commented
Sep 3, 2026
/review |
✅ Android PR Reviewer completed successfully!
|
There was a problem hiding this comment.
✅ LGTM — 0 errors, 0 warnings, 1 suggestion. The reference and UTF-character lifetimes are now correctly independent, overflow is checked before heap allocation, explicit construction/destruction is balanced, and all 44 CI checks passed. I left one inline suggestion for direct regression coverage of the lazy-reference and heap-storage paths.
Generated by Android PR Reviewer for #12568 · gpt56 · 73.7 AIC · ⌖ 9.03 AIC · ⊞ 25.7K
Comment /review to run again
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Part of the drop-libc++ effort.
Split out of #12560, where it had been sitting alongside the DSO loader changes despite being unrelated to them.
jstring_array_wrapperhad two problems.A JNI local reference leak
jstring_wrapper::release ()bailed out early when it had no UTF chars to release:But
jstring_array_wrapper::operator[]fetches the array element's reference eagerly, whilecstris only populated on the firstget_cstr ()call. So any element that was indexed but never read kept its local reference until the frame was popped. The reference and the UTF chars have independent lifetimes, so they are now released independently.new[]/delete[]The wrapper allocated its elements with
new jstring_wrapper[len], which is where_Znamand_ZdaPvinhost.cc.ocame from. It now usesmalloc()with explicit placement construction and destruction.jstring_wrapperis not an implicit-lifetime type — it has a user-provided destructor — somalloc()alone cannot begin its lifetime; placement new is required. Its default constructor is private, withjstring_array_wrapperas a friend, which is what makes that legal here.Results
host.cc.orefsAll three runtime lanes build clean.