Skip to content

fix(jni): keep a wrapper reachable while its handle is in a native call - #635

Merged
andiwand merged 1 commit into
mainfrom
fix/jni-handle-reachability
Jul 30, 2026
Merged

fix(jni): keep a wrapper reachable while its handle is in a native call#635
andiwand merged 1 commit into
mainfrom
fix/jni-handle-reachability

Conversation

@andiwand

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Stacked on #633 — review that first, this branch only adds the commit on top.

Fallout from reviewing the close()-races-listen() finding on #633: that one
is about explicit close, this is the other way a handle can go while it is in
use — the collector.

The rule that already exists

jni/AGENTS.md:

GC safety: natives that use a handle are instance methods (the this
local reference keeps the wrapper — and its owner chain — reachable for the
duration of the call); only factories and destroy are static.

That is exactly right, and it is what makes 199 of the natives safe. Six calls
did not follow it.

Handle in receiver position — five of them

Logger.willLog / log / flush, Html.edit, new DecodedFile(File) all
passed the handle of the object they belong to into a static native:

public boolean willLog(LogLevel level) {
  return willLogNative(handle(), level.toNative());   // static native
}

Once handle() returns, nothing on the stack refers to this. The JIT may
treat it as dead, the collector enqueues the phantom reference, the reaper calls
destroy(handle) — and willLogNative is running on freed memory. The fix is
the rule: the natives move onto their owner.

  • Logger's three simply lose static (jclassjobject in the JNI).
  • Html.edit(document, diff) → delegates to a package-private
    Document.edit(diff); Java_…_Html_editDocumentJava_…_Document_editNative.
  • new DecodedFile(File file)this(file.decode());
    Java_…_DecodedFile_createFromFileJava_…_File_decodeNative. Constructor
    chaining, so this one had to be restructured rather than fenced.

Public API is unchanged — Html.edit and the DecodedFile(File) constructor
still exist and still do the same thing.

Handle in argument position — the kind the rule does not cover

a.fooNative(handle(), b.handle()) holds a through the receiver but nothing
holds b. Five sites:

DocumentPath.join, Element.isSame, Element.navigatePath,
HttpServer.connectService, Odr.open(path, logger).

There is no receiver to move them to, so they get NativeResource.keepAlive()
in a finally. The three Html.translate overloads look like the same shape and
are fine by accident — the owner is the second constructor argument, evaluated
after the native call returns, so it is live across it.

Why keepAlive and not reachabilityFence

Reference.reachabilityFence is the API for this and is unusable here: android
has it from API 28, android/build.gradle.kts sets minSdk = 26, and core
library desugaring does not cover java.lang.ref — the same wall the
PhantomReference reaper already hit with Cleaner.

keepAlive() is synchronized (this) {}, which is what reachabilityFence was
in the JDK before it became an intrinsic. Lock elision requires the object to be
provably confined; one that the reference queue can see is not.

Honesty about severity

Nothing here is an observed crash. The window needs a collection to land inside
a native call on an object the caller has no further use for. Html.translate
and Odr.open(path, logger) are the calls long enough to make that plausible;
Element.isSame is theory. It is cheap to be right, and the rule now covers the
case that was missing.

Tests

The two renamed JNI symbols would otherwise fail only on first call, with
UnsatisfiedLinkError, and neither path was covered: FileTest.decodeAnOpenFile
and DocumentTest.editAppliesADiff. DocumentTest.documentPath gains
join/navigatePath, the argument-position pair that had no test either. 35
JUnit tests pass; gtest and pytest unaffected and re-run green.

jni/AGENTS.md extended with the argument-position case and the API-28 trap.

@andiwand
andiwand force-pushed the fix/jni-handle-reachability branch from f6989bd to e01f042 Compare July 30, 2026 21:07
Base automatically changed from fix/http-server-teardown to main July 30, 2026 21:19
`jni/AGENTS.md` already asks for it - "natives that use a handle are instance
methods" - because the JNI frame's reference to the receiver is what stops the
collector from enqueuing the wrapper, and the reaper from freeing the handle,
while the call is still running. Six calls did not follow it.

Five passed the handle of the object they belong to into a *static* native, so
nothing referred to the wrapper for the duration: `Logger.willLog`/`log`/
`flush`, `Html.edit` and `new DecodedFile(File)`. The natives move onto their
owner - `Logger`'s three lose `static`, `Html.edit` delegates to
`Document.edit`, and the `DecodedFile(File)` constructor to `File.decode` -
which puts the wrapper in the JNI frame where the rule intends it.

The sixth kind the rule does not cover: a handle in *argument* position,
`a.fooNative(handle(), b.handle())`, has no receiver to hold it. `Html.translate`
gets away with it by accident, passing the file to the HtmlService constructor
afterwards; `DocumentPath.join`, `Element.isSame`, `Element.navigatePath`,
`HttpServer.connectService` and `Odr.open(path, logger)` do not. Those get
`NativeResource.keepAlive()` in a `finally`.

`keepAlive` is `synchronized (this) {}`, which is what
`Reference.reachabilityFence` was before it became an intrinsic: android has
that method only from API 28, this artifact's floor is 26, and core library
desugaring does not cover `java.lang.ref` - the same wall the `PhantomReference`
reaper hit with `Cleaner`. Lock elision needs the object to be provably
confined, and one the reference queue can see is not.

Nothing here is an observed crash; the window needs a collection to land inside
a native call. `Html.translate` and `Odr.open(path, logger)` are the ones long
enough to make that plausible.

Tests for the two renamed symbols, which would otherwise fail only at first call
with UnsatisfiedLinkError, and for the argument-position pair that had none:
`File.decodeAnOpenFile`, `Document.editAppliesADiff`, and join/navigatePath in
`Document.documentPath`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEvpyHCo15bkwhKgsEviGi
@andiwand
andiwand force-pushed the fix/jni-handle-reachability branch from e01f042 to 64af916 Compare July 30, 2026 21:19
@andiwand
andiwand merged commit b33b070 into main Jul 30, 2026
26 checks passed
@andiwand
andiwand deleted the fix/jni-handle-reachability branch July 30, 2026 21:32
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.

1 participant