From 64af9163f7ec06669e9ff1a7f7157f017da81798 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 30 Jul 2026 21:34:26 +0200 Subject: [PATCH] fix(jni): keep a wrapper reachable while its handle is in a native call `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) Claude-Session: https://claude.ai/code/session_01PEvpyHCo15bkwhKgsEviGi --- jni/AGENTS.md | 21 ++++++++++++--- .../app/opendocument/core/DecodedFile.java | 4 +-- jni/java/app/opendocument/core/Document.java | 7 +++++ .../app/opendocument/core/DocumentPath.java | 6 ++++- jni/java/app/opendocument/core/Element.java | 12 +++++++-- jni/java/app/opendocument/core/File.java | 7 +++++ jni/java/app/opendocument/core/Html.java | 4 +-- .../app/opendocument/core/HttpServer.java | 6 ++++- jni/java/app/opendocument/core/Logger.java | 6 ++--- .../app/opendocument/core/NativeResource.java | 26 +++++++++++++++++++ jni/java/app/opendocument/core/Odr.java | 6 ++++- jni/src/jni_document.cpp | 12 +++++++++ jni/src/jni_file.cpp | 18 +++++++------ jni/src/jni_html.cpp | 8 ------ jni/src/jni_logger.cpp | 6 ++--- .../app/opendocument/core/DocumentTest.java | 19 ++++++++++++++ jni/tests/app/opendocument/core/FileTest.java | 9 +++++++ 17 files changed, 140 insertions(+), 37 deletions(-) diff --git a/jni/AGENTS.md b/jni/AGENTS.md index 9d96bd712..5092869f9 100644 --- a/jni/AGENTS.md +++ b/jni/AGENTS.md @@ -29,9 +29,20 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings - **Keep-alive**: navigation results carry an `owner` reference (`Element.owner()` → the `Document`) so the GC cannot free the root while handles into it are alive — the JNI analogue of pyodr's `keep_alive`. -- **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. +- **GC safety**: natives that use a handle are *instance* methods **of the + object that owns it** (the `this` local reference keeps the wrapper — and its + owner chain — reachable for the duration of the call); only factories and + `destroy` are static. A static native taking someone else's handle is a bug + even where it reads fine: once `handle()` has returned nothing refers to the + wrapper, so the collector may enqueue it and the reaper free the handle while + the call is still running. Put the native on the owner and let the entry point + delegate — `Html.edit` → `Document.edit`, `new DecodedFile(file)` → + `File.decode`, and `Logger`'s own natives. +- **Handles in argument position** — `a.fooNative(handle(), b.handle())` — have + no receiver holding them, so `b` needs `NativeResource.keepAlive()` in a + `finally` after the call. That is what `Reference.reachabilityFence` is for; + android only has it from API 28 (see the API floor below), so `keepAlive` is + the JDK's own pre-intrinsic fallback instead. - **Enums cross as ordinals**: Java enum constant order must match the C++ enum declaration; `-1` encodes an absent `std::optional`. - **Strings**: use `odr_jni::to_string`/`to_jstring` (real UTF-8 ↔ UTF-16), @@ -60,7 +71,9 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings `--release 17` compiler accepts. Anything newer fails at runtime, on device only, with `NoClassDefFoundError`/`NoSuchMethodError`. Known traps, all of which android added far later than the JDK: `java.lang.ref.Cleaner` (API 33, - hence the `PhantomReference` reaper), `List.of`/`Set.of`/`Map.of` (API 34), + hence the `PhantomReference` reaper), + `java.lang.ref.Reference.reachabilityFence` (API 28, hence `keepAlive`), + `List.of`/`Set.of`/`Map.of` (API 34), `Optional.isEmpty` (API 33), `java.time` (API 26 only in part). Core library desugaring does not cover `java.lang.ref`, and an app cannot shim a `java.*` class, so the fix always has to happen here. This is no longer only a rule to diff --git a/jni/java/app/opendocument/core/DecodedFile.java b/jni/java/app/opendocument/core/DecodedFile.java index ab4a75705..2bdf4d179 100644 --- a/jni/java/app/opendocument/core/DecodedFile.java +++ b/jni/java/app/opendocument/core/DecodedFile.java @@ -23,7 +23,7 @@ public DecodedFile(String path, FileType as) { } public DecodedFile(File file) { - this(createFromFile(file.handle())); + this(file.decode()); } public File file() { @@ -120,8 +120,6 @@ public FontFile asFontFile() { private static native long createAs(String path, int as); - private static native long createFromFile(long fileHandle); - static native void destroy(long handle); private native long fileNative(long handle); diff --git a/jni/java/app/opendocument/core/Document.java b/jni/java/app/opendocument/core/Document.java index b05690842..ba4c4a502 100644 --- a/jni/java/app/opendocument/core/Document.java +++ b/jni/java/app/opendocument/core/Document.java @@ -43,8 +43,15 @@ public Filesystem asFilesystem() { return new Filesystem(asFilesystemNative(handle()), this); } + /** Applies a diff; what {@link Html#edit} calls. */ + void edit(String diff) { + editNative(handle(), diff); + } + private static native void destroy(long handle); + private native void editNative(long handle, String diff); + private native boolean isEditableNative(long handle); private native boolean isSavableNative(long handle, boolean encrypted); diff --git a/jni/java/app/opendocument/core/DocumentPath.java b/jni/java/app/opendocument/core/DocumentPath.java index 2bc49756d..9d873dbb5 100644 --- a/jni/java/app/opendocument/core/DocumentPath.java +++ b/jni/java/app/opendocument/core/DocumentPath.java @@ -23,7 +23,11 @@ public DocumentPath parent() { } public DocumentPath join(DocumentPath other) { - return new DocumentPath(joinNative(handle(), other.handle())); + try { + return new DocumentPath(joinNative(handle(), other.handle())); + } finally { + other.keepAlive(); + } } @Override diff --git a/jni/java/app/opendocument/core/Element.java b/jni/java/app/opendocument/core/Element.java index b5d6a924c..f07e44a85 100644 --- a/jni/java/app/opendocument/core/Element.java +++ b/jni/java/app/opendocument/core/Element.java @@ -49,7 +49,11 @@ public boolean isEditable() { /** Whether this and {@code other} refer to the same element. */ public boolean isSame(Element other) { - return isSameNative(handle(), other.handle()); + try { + return isSameNative(handle(), other.handle()); + } finally { + other.keepAlive(); + } } public DocumentPath documentPath() { @@ -57,7 +61,11 @@ public DocumentPath documentPath() { } public Element navigatePath(DocumentPath path) { - return wrap(navigatePathNative(handle(), path.handle())); + try { + return wrap(navigatePathNative(handle(), path.handle())); + } finally { + path.keepAlive(); + } } public List children() { diff --git a/jni/java/app/opendocument/core/File.java b/jni/java/app/opendocument/core/File.java index 3bf585f4c..303db0e06 100644 --- a/jni/java/app/opendocument/core/File.java +++ b/jni/java/app/opendocument/core/File.java @@ -36,10 +36,17 @@ public void copy(String path) { copyNative(handle(), path); } + /** Decodes this file; the handle for {@link DecodedFile#DecodedFile(File)}. */ + long decode() { + return decodeNative(handle()); + } + private static native long create(String path); private static native void destroy(long handle); + private native long decodeNative(long handle); + private native int locationNative(long handle); private native long sizeNative(long handle); diff --git a/jni/java/app/opendocument/core/Html.java b/jni/java/app/opendocument/core/Html.java index 5df74e729..983bcef4f 100644 --- a/jni/java/app/opendocument/core/Html.java +++ b/jni/java/app/opendocument/core/Html.java @@ -68,7 +68,7 @@ public static HtmlService translate(Filesystem filesystem, String cachePath, Htm /** Applies a diff (produced by the browser-side editor) to a document. */ public static void edit(Document document, String diff) { - editDocument(document.handle(), diff); + document.edit(diff); } private static native long translateFile(long fileHandle, String cachePath, HtmlConfig config); @@ -78,6 +78,4 @@ private static native long translateDocument( private static native long translateFilesystem( long filesystemHandle, String cachePath, HtmlConfig config); - - private static native void editDocument(long documentHandle, String diff); } diff --git a/jni/java/app/opendocument/core/HttpServer.java b/jni/java/app/opendocument/core/HttpServer.java index 5fd15b5e9..7be89e670 100644 --- a/jni/java/app/opendocument/core/HttpServer.java +++ b/jni/java/app/opendocument/core/HttpServer.java @@ -40,7 +40,11 @@ public HttpServer(Config config) { /** Hosts the service under {@code //}. */ public void connectService(HtmlService service, String prefix) { - connectServiceNative(handle(), service.handle(), prefix); + try { + connectServiceNative(handle(), service.handle(), prefix); + } finally { + service.keepAlive(); + } } /** diff --git a/jni/java/app/opendocument/core/Logger.java b/jni/java/app/opendocument/core/Logger.java index 487f95552..ce847d39f 100644 --- a/jni/java/app/opendocument/core/Logger.java +++ b/jni/java/app/opendocument/core/Logger.java @@ -48,11 +48,11 @@ public void flush() { private static native long createFromSink(ILogger sink); - private static native boolean willLogNative(long handle, int level); + private native boolean willLogNative(long handle, int level); - private static native void logNative(long handle, int level, String message); + private native void logNative(long handle, int level, String message); - private static native void flushNative(long handle); + private native void flushNative(long handle); private static native void destroy(long handle); } diff --git a/jni/java/app/opendocument/core/NativeResource.java b/jni/java/app/opendocument/core/NativeResource.java index 22c131741..c5ebc9ced 100644 --- a/jni/java/app/opendocument/core/NativeResource.java +++ b/jni/java/app/opendocument/core/NativeResource.java @@ -81,6 +81,32 @@ final boolean isClosed() { return closed; } + /** + * A use of this object that the optimiser cannot drop, so it stays reachable + * across whatever ran before it. + * + *

{@link #handle()} outlives the wrapper it came from: once nothing refers to + * the wrapper any more the collector may enqueue it and the reaper may free the + * handle, and the just-in-time compiler is free to decide that while a native + * call using it is still running. A native that takes the handle of the object it + * is called on is safe without this - the JNI frame holds the receiver - which is + * why every one of them is an instance method of its owner. This is for the + * handles that go in as arguments, where there is no receiver to hold them. + * + *

{@code java.lang.ref.Reference.reachabilityFence} is the API for it, and is + * unusable here: android has it from API level 28, {@code android/build.gradle.kts} + * sets {@code minSdk = 26}, and core library desugaring does not cover + * {@code java.lang.ref} - the same wall {@link java.lang.ref.Cleaner} hit above. + * Taking the monitor of an object the reference queue can also see is the fallback + * the JDK itself used before that method existed: lock elision needs the object to + * be provably confined, and this one is not. + */ + final void keepAlive() { + synchronized (this) { + // empty on purpose - taking the monitor is the use + } + } + /** Frees the native object early. Idempotent. */ @Override public void close() { diff --git a/jni/java/app/opendocument/core/Odr.java b/jni/java/app/opendocument/core/Odr.java index 6f17b713a..0e497aaad 100644 --- a/jni/java/app/opendocument/core/Odr.java +++ b/jni/java/app/opendocument/core/Odr.java @@ -107,7 +107,11 @@ public static DecodedFile open(String path) { /** Opens and decodes a file, reporting diagnostics to {@code logger}. */ public static DecodedFile open(String path, Logger logger) { - return new DecodedFile(openWithLoggerNative(path, logger.handle())); + try { + return new DecodedFile(openWithLoggerNative(path, logger.handle())); + } finally { + logger.keepAlive(); + } } /** Opens and decodes a file as the given file type. */ diff --git a/jni/src/jni_document.cpp b/jni/src/jni_document.cpp index 7046c3ec9..8add6671f 100644 --- a/jni/src/jni_document.cpp +++ b/jni/src/jni_document.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include @@ -53,6 +54,17 @@ Java_app_opendocument_core_Document_destroy(JNIEnv *, jclass, jlong handle) { destroy_handle(handle); } +// odr::html::edit, but it belongs to Document: a native that takes a handle has +// to be an instance method of whatever owns it, or the wrapper can be collected +// - and the handle freed - while the call is still running +extern "C" JNIEXPORT void JNICALL +Java_app_opendocument_core_Document_editNative(JNIEnv *env, jobject, + jlong handle, jstring diff) { + guarded(env, [&] { + odr::html::edit(*from_handle(handle), to_string(env, diff)); + }); +} + extern "C" JNIEXPORT jboolean JNICALL Java_app_opendocument_core_Document_isEditableNative(JNIEnv *env, jobject, jlong handle) { diff --git a/jni/src/jni_file.cpp b/jni/src/jni_file.cpp index db89078bd..31d9c6388 100644 --- a/jni/src/jni_file.cpp +++ b/jni/src/jni_file.cpp @@ -76,6 +76,16 @@ extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_File_copyNative( [&] { from_handle(handle)->copy(to_string(env, path)); }); } +// yields a DecodedFile but belongs to File: a native that takes a handle has to +// be an instance method of whatever owns it, or the wrapper can be collected - +// and the handle freed - while the call is still running +extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_File_decodeNative( + JNIEnv *env, jobject, jlong handle) { + return guarded(env, [&] { + return make_handle(odr::DecodedFile(*from_handle(handle))); + }); +} + // app.opendocument.core.DecodedFile // // Handles hold a heap `odr::DecodedFile` (the typed C++ subclasses are sliced @@ -97,14 +107,6 @@ Java_app_opendocument_core_DecodedFile_createAs(JNIEnv *env, jclass, }); } -extern "C" JNIEXPORT jlong JNICALL -Java_app_opendocument_core_DecodedFile_createFromFile(JNIEnv *env, jclass, - jlong file_handle) { - return guarded(env, [&] { - return make_handle(odr::DecodedFile(*from_handle(file_handle))); - }); -} - extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_DecodedFile_destroy(JNIEnv *, jclass, jlong handle) { destroy_handle(handle); diff --git a/jni/src/jni_html.cpp b/jni/src/jni_html.cpp index 288b66d43..6c070a199 100644 --- a/jni/src/jni_html.cpp +++ b/jni/src/jni_html.cpp @@ -171,14 +171,6 @@ Java_app_opendocument_core_Html_translateFilesystem(JNIEnv *env, jclass, }); } -extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Html_editDocument( - JNIEnv *env, jclass, jlong document_handle, jstring diff) { - guarded(env, [&] { - odr::html::edit(*from_handle(document_handle), - to_string(env, diff)); - }); -} - // app.opendocument.core.HtmlService extern "C" JNIEXPORT void JNICALL diff --git a/jni/src/jni_logger.cpp b/jni/src/jni_logger.cpp index 192645513..2571f4b97 100644 --- a/jni/src/jni_logger.cpp +++ b/jni/src/jni_logger.cpp @@ -198,7 +198,7 @@ Java_app_opendocument_core_Logger_createFromSink(JNIEnv *env, jclass, } extern "C" JNIEXPORT jboolean JNICALL -Java_app_opendocument_core_Logger_willLogNative(JNIEnv *env, jclass, +Java_app_opendocument_core_Logger_willLogNative(JNIEnv *env, jobject, jlong handle, jint level) { return guarded(env, [&] { return static_cast(from_handle(handle)->will_log( @@ -207,7 +207,7 @@ Java_app_opendocument_core_Logger_willLogNative(JNIEnv *env, jclass, } extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Logger_logNative( - JNIEnv *env, jclass, jlong handle, jint level, jstring message) { + JNIEnv *env, jobject, jlong handle, jint level, jstring message) { guarded(env, [&] { from_handle(handle)->log(static_cast(level), to_string(env, message)); @@ -215,7 +215,7 @@ extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Logger_logNative( } extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Logger_flushNative( - JNIEnv *env, jclass, jlong handle) { + JNIEnv *env, jobject, jlong handle) { guarded(env, [&] { from_handle(handle)->flush(); }); } diff --git a/jni/tests/app/opendocument/core/DocumentTest.java b/jni/tests/app/opendocument/core/DocumentTest.java index 503b785b4..8e52b44e6 100644 --- a/jni/tests/app/opendocument/core/DocumentTest.java +++ b/jni/tests/app/opendocument/core/DocumentTest.java @@ -85,5 +85,24 @@ void documentPath() throws IOException { DocumentPath path = first.documentPath(); assertNotNull(path.toString()); assertEquals(path, first.documentPath()); + + // join() and navigatePath() take another wrapper's handle as an argument + DocumentPath rejoined = path.parent().join(path); + assertTrue(path.parent().empty()); + assertEquals(path, rejoined); + assertTrue(document.rootElement().navigatePath(rejoined).isSame(first)); + } + + @Test + void editAppliesADiff() throws IOException { + Document document = openDocument(); + assertTrue(document.isEditable()); + + Element paragraph = document.rootElement().firstChild(); + DocumentPath text = paragraph.firstChild().documentPath(); + + Html.edit(document, "{\"modifiedText\":{\"" + text + "\":\"edited by the diff\"}}"); + + assertTrue(walkText(document.rootElement()).contains("edited by the diff")); } } diff --git a/jni/tests/app/opendocument/core/FileTest.java b/jni/tests/app/opendocument/core/FileTest.java index 6ed5e756d..f42da8304 100644 --- a/jni/tests/app/opendocument/core/FileTest.java +++ b/jni/tests/app/opendocument/core/FileTest.java @@ -58,6 +58,15 @@ void fileReadMatchesSize() throws IOException { } } + @Test + void decodeAnOpenFile() throws IOException { + Path odt = TestFiles.odtFile(tempDir); + try (File file = new File(odt.toString()); + DecodedFile decoded = new DecodedFile(file)) { + assertEquals(FileType.OPENDOCUMENT_TEXT, decoded.fileType()); + } + } + @Test void fileMeta() throws IOException { Path odt = TestFiles.odtFile(tempDir);