From 57884ebdeda86513cca1dd082705931471163e9e Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 26 Jul 2026 13:41:20 +0200 Subject: [PATCH] fix(jni): keep the java API loadable on android API 26 OpenDocument.droid has `minSdk = 26`, and android's `java.*` is far behind what the `--release 17` compiler accepts. Two APIs in the bindings only exist on much newer android, and both fail on device rather than at build time: - `java.lang.ref.Cleaner` (android API 33) sits in `NativeResource`'s static initializer, so *every* handle type - DecodedFile, Document, HtmlService, HttpServer, Element - dies with `NoClassDefFoundError: java.lang.ref. Cleaner` the first time it is touched. Core library desugaring does not cover `java.lang.ref`, and an app cannot supply a `java.*` class itself, so this can only be fixed here. - `List.of` (android API 34) in `Html`, on the offline-output path. `NativeResource` now does what a Cleaner does internally: a `PhantomReference` per resource, enqueued on a shared `ReferenceQueue` and drained by a daemon thread. The pending references are held in a set, because a phantom reference that is collected itself never gets enqueued and would leak the native object instead of freeing it. `close()` and the post-mortem path both go through the same `compareAndSet`, so a resource is still destroyed exactly once and `close()` stays idempotent - no change to the public contract. `Html` builds its unmodifiable lists with `Collections.unmodifiableList( Arrays.asList(...))` instead. AGENTS.md gains the API 26 floor as an explicit design rule, with the traps that are easy to reach for (`Cleaner`, `List.of`/`Set.of`/`Map.of`, `Optional.isEmpty`). Verified by building the patched java API into the artifact odr.droid consumes and running its instrumented suite on a Pixel 6 Pro AVD (API 31): 12/12 green, where the Cleaner failure previously took down every test that touches the core. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016jT7GGUHvUnDvf8UMcQSLk --- jni/AGENTS.md | 14 +++- jni/java/app/opendocument/core/Html.java | 6 +- .../app/opendocument/core/NativeResource.java | 81 ++++++++++++++++--- 3 files changed, 88 insertions(+), 13 deletions(-) diff --git a/jni/AGENTS.md b/jni/AGENTS.md index 182be13a2..fc82c47dd 100644 --- a/jni/AGENTS.md +++ b/jni/AGENTS.md @@ -18,8 +18,9 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings - **Handle model**: a Java wrapper owns a heap-allocated copy of the C++ value handle (`odr::DecodedFile`, `odr::Element`, ...) referenced by a `long`. - `NativeResource` frees it via `Cleaner`/`AutoCloseable`; each class has a - static `destroy(long)` native deleting the concrete C++ type. + `NativeResource` frees it via a `PhantomReference` reaper thread and + `AutoCloseable`; each class has a static `destroy(long)` native deleting the + concrete C++ type. - **Typed views are re-derived per call**: element/file handles always point to the *base* type (`odr::Element`, `odr::DecodedFile`); natives of typed Java classes call `as_paragraph()`/`as_text_file()` etc. on each call. Never store @@ -46,6 +47,15 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings don't marshal it unconditionally. - New public C++ API? Extend the matching `jni_*.cpp` + Java class and add a JUnit test. +- **Android API level 26 is the java floor**: OpenDocument.droid consumes this + artifact with `minSdk = 26`, and android ships a much older `java.*` than the + `--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), + `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. - C++ sources follow the repo clang-format; Java follows the google-java-format style (2-space indent), no enforced formatter yet. - Tests must stay hermetic: build inputs inline in `tests/.../TestFiles.java`; diff --git a/jni/java/app/opendocument/core/Html.java b/jni/java/app/opendocument/core/Html.java index f65190887..5df74e729 100644 --- a/jni/java/app/opendocument/core/Html.java +++ b/jni/java/app/opendocument/core/Html.java @@ -1,5 +1,7 @@ package app.opendocument.core; +import java.util.Arrays; +import java.util.Collections; import java.util.List; /** @@ -16,7 +18,7 @@ public final class Html { Html(HtmlConfig config, HtmlPage[] pages) { this.config = config; - this.pages = List.of(pages); + this.pages = Collections.unmodifiableList(Arrays.asList(pages)); } public HtmlConfig config() { @@ -34,7 +36,7 @@ public static final class Content { Content(String html, LocatedResource[] resources) { this.html = html; - this.resources = List.of(resources); + this.resources = Collections.unmodifiableList(Arrays.asList(resources)); } } diff --git a/jni/java/app/opendocument/core/NativeResource.java b/jni/java/app/opendocument/core/NativeResource.java index 675345196..5e08bddf7 100644 --- a/jni/java/app/opendocument/core/NativeResource.java +++ b/jni/java/app/opendocument/core/NativeResource.java @@ -1,29 +1,67 @@ package app.opendocument.core; -import java.lang.ref.Cleaner; +import java.lang.ref.PhantomReference; +import java.lang.ref.ReferenceQueue; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.LongConsumer; /** * Owns a handle to a heap-allocated native object. The native object is freed * on {@link #close()} or, at the latest, when the garbage collector reclaims - * this wrapper (via {@link Cleaner}). + * this wrapper. * *

Navigation results (e.g. document elements) keep the object they * originate from reachable through {@code owner}, so a root object is not * collected while handles into it are alive. + * + *

The post-mortem free is a {@link PhantomReference} drained by a daemon + * thread rather than a {@link java.lang.ref.Cleaner}, which is exactly what a + * Cleaner does internally. Cleaner is a JDK 9 API that android only ships from + * API level 33, and OpenDocument.droid targets API 26; referencing it made the + * whole class fail to load there with a {@code NoClassDefFoundError}, and core + * library desugaring does not cover {@code java.lang.ref}. */ public abstract class NativeResource implements AutoCloseable { - private static final Cleaner CLEANER = Cleaner.create(); + private static final ReferenceQueue QUEUE = new ReferenceQueue<>(); + + /** + * Keeps the pending {@link Destroyer}s reachable. A phantom reference that is + * itself collected never gets enqueued, so the native object behind it would + * leak instead of being freed. + */ + private static final Set PENDING = ConcurrentHashMap.newKeySet(); + + static { + Thread reaper = + new Thread( + () -> { + while (true) { + try { + ((Destroyer) QUEUE.remove()).destroy(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } catch (RuntimeException | Error e) { + // one broken destructor must not stop the others from running + } + } + }, + "odr-core-native-resource-reaper"); + reaper.setDaemon(true); + reaper.start(); + } private final long handle; private final Object owner; - private final Cleaner.Cleanable cleanable; + private final Destroyer destroyer; private volatile boolean closed; NativeResource(long handle, Object owner, LongConsumer destroyer) { this.handle = handle; this.owner = owner; - this.cleanable = CLEANER.register(this, new Destroyer(handle, destroyer)); + this.destroyer = new Destroyer(this, handle, destroyer); } /** The native handle; valid only while this object is open. */ @@ -42,12 +80,37 @@ final Object owner() { @Override public void close() { closed = true; - cleanable.clean(); + destroyer.destroy(); } - private record Destroyer(long handle, LongConsumer destroy) implements Runnable { - @Override - public void run() { + /** + * Frees one native object, at most once, whether it is reached through + * {@link #close()} or through the reference queue. + * + *

Holds no reference to the {@link NativeResource} other than the phantom + * one - a strong one would keep the wrapper alive forever. + */ + private static final class Destroyer extends PhantomReference { + private final long handle; + private final LongConsumer destroy; + private final AtomicBoolean destroyed = new AtomicBoolean(); + + Destroyer(NativeResource referent, long handle, LongConsumer destroy) { + super(referent, QUEUE); + + this.handle = handle; + this.destroy = destroy; + + PENDING.add(this); + } + + void destroy() { + if (!destroyed.compareAndSet(false, true)) { + return; + } + + PENDING.remove(this); + clear(); destroy.accept(handle); } }