diff --git a/jni/AGENTS.md b/jni/AGENTS.md index 9d96bd71..5092869f 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 ab4a7570..2bdf4d17 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 b0569084..ba4c4a50 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 2bc49756..9d873dbb 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 b5d6a924..f07e44a8 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 3bf585f4..303db0e0 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 5df74e72..983bcef4 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 5fd15b5e..7be89e67 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 487f9555..ce847d39 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 22c13174..c5ebc9ce 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 6f17b713..0e497aaa 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 7046c3ec..8add6671 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 db89078b..31d9c638 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 288b66d4..6c070a19 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 19264551..2571f4b9 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 503b785b..8e52b44e 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 6ed5e756..f42da830 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);