Code review: no deterministic handle release (native OOM risk), double-free race, JNI local-ref-table aborts

> **AI-assisted review.** Filed by agent driven by @soloturn via [GDD](https://siliconsaga.github.io/yggdrasil/gdd/).

Reviewed both source trees — the Java wrapper classes (`lib/src/main/java/org/kiwix/{libkiwix,libzim}/*.java`) and the JNI glue implementing their native methods (`lib/src/main/cpp/{libkiwix,libzim}/*.cpp`, `utils.h`, `macros.h`) — for performance, memory consumption, simplification, and error-proneness, weighted toward the first two.

## Memory (highest severity)

1. **Every handle class relies solely on `finalize()` for release; `Entry`/`Item` have no public release path at all.** All wrapper classes (`Entry`, `Item`, `Blob`, `Archive`, `EntryIterator`, `SearchIterator`, `SuggestionIterator`, `Search`, `Searcher`, `Library`, `Book`, `Filter`, etc.) use `protected void finalize() { dispose(); }` only. `Entry.dispose()`/`Item.dispose()` are declared `private native`, so a consumer cannot free them deterministically even if it wants to — no class implements `Closeable`/`AutoCloseable`. Each `EntryIterator.next()` heap-allocates a native `zim::Entry` plus a heap `shared_ptr`, while the Java-side object is ~16 bytes, so the GC sees almost no pressure and doesn't collect promptly — native RSS grows unbounded while the Java heap looks fine. Finalizable objects also go through the single `FinalizerDaemon` thread with a watchdog timeout, so even when GC runs, reclamation is late and batched. A full-ZIM scan (e.g. in kiwix-android) can OOM natively long before the Java heap notices. Fix: implement `AutoCloseable` with a public `close()`, make `dispose()` public, replace `finalize()` with `java.lang.ref.Cleaner` (or keep finalize only as a backstop).
2. **`dispose()`/`finalize()` can race into a double-free.** `utils.h:120-130` reads the handle `long` field, `delete`s it, then clears the field — three unsynchronized steps, and no method is `synchronized`. `this` can become unreachable mid-`dispose()` (ART may collect it), letting the finalizer thread concurrently `delete` the same pointer, or two app threads sharing a handle can both call `dispose()`. Heap-corruption crash far from the actual cause. Fix: null the field *before* deleting, make the read-and-clear atomic, or synchronize `dispose()`.
3. **Local JNI references leaked inside caller-controlled-length loops → hard process abort.** `spellingdb.cpp:46-48`, `utils.h:361-365` (`jni2c`), `searcher.cpp:47-51`, `archive.cpp:211-238` (incl. `jni2fdInput`), `book.cpp:95-99` — none call `DeleteLocalRef` per iteration. Android's local-ref-table cap is 512 per frame; exceeding it is a hard `ReferenceTable overflow` abort, not a catchable exception. `SpellingsDB.getSpellingCorrections(word, 600)` aborts the process outright. `library.cpp:128`/`book.cpp:98` already show the correct pattern elsewhere in the same codebase. Fix: `DeleteLocalRef` each per-iteration ref (hoist `FindClass` out of loops as `library.cpp:119` already does), or `PushLocalFrame`/`PopLocalFrame`.
4. `utils.h:85-86`, `library.cpp:125-127` — a heap `shared_ptr` (and the native object it owns) is allocated *before* `SetLongField`/`NewObject`; if either fails (OOM, missing field id), the handle is leaked with no path to reclaim it.
5. `blob.cpp:39` → `utils.h:308-313` — `NewByteArray`'s result is passed straight to `SetByteArrayRegion` with no NULL check anywhere in the codebase (zero `ExceptionCheck`/`ExceptionOccurred` calls). On a large item under memory pressure, `NewByteArray` legitimately returns NULL with a pending `OutOfMemoryError`; the immediate `SetByteArrayRegion(NULL, ...)` then aborts (CheckJNI) or segfaults, instead of the catchable exception Java code could otherwise handle.
6. `utils.h:83` — the one leak guard that exists (`assert(GetLongField(...) == 0)` before overwriting a handle) is compiled out under `NDEBUG`, i.e. exactly in Android release builds.

## Performance

7. **No JNI id caching anywhere** — `getPtr` (`utils.h:106-116`) does `GetObjectClass` + `GetFieldID` on every call; `newObject`/`newObject2` (`utils.h:55-72`) do `FindClass` + `GetMethodID` on every wrapper construction. No `JNI_OnLoad`, no cached `jclass`/`jfieldID`/`jmethodID` anywhere (grep-verified). `EntryIterator.next()` costs ~8 string-keyed runtime lookups per entry — on a full-ZIM walk (millions of entries) this can dominate over the actual libzim work. Fix: `JNI_OnLoad` caching global-ref `jclass`es and `jfieldID`s into statics.
8. `entry_iterator.cpp:69,74,79` — `EntryIterator.hasNext()` deep-copies the whole iterator (constructing an `Entry`, bumping `shared_ptr` refcounts) on every call; `search_iterator.cpp:61` proves the copy is unnecessary by comparing in place directly.
9. `SearchIterator.java:27-36` — 7 separate native accessors read the current position (each paying the #7 lookup cost plus a fresh `jstring` allocation+copy); a consumer rendering one result makes 6 calls plus `next()`. There's also an undocumented, unguarded ordering contract (accessors must run before `next()`). Fix: a single `nextResult()` returning one value object.
10. `Item.getData()` → `blob.cpp:38-40` — `cArray2jni` always fully copies the item into a fresh `byte[]`; no ranged/streaming read API. The native `zim::Blob` (and the whole decompressed cluster it references, see #memory-leaks list below) stays alive until finalization. `blob.cpp:39` also calls `THIS` (a `getPtr`, #7) twice for one method.
11. `item.cpp:50-55` — `getDirectAccessInformation()` writes its result object twice (the first call is entirely dead — allocations discarded immediately).
12. `library.cpp:53` — a debug `std::cout` print left in `getArchiveById`, pure cost with no benefit (invisible on Android).

## Error-proneness

13. **`jni2c` (`utils.h:358-365`) silently corrupts data.** Sizes a vector with `type_t v(length)` (default-constructing `length` empty strings), then `push_back`s each converted element on top — doubling the vector, first half empty. The only consumers are `Filter.acceptTags`/`rejectTags` (via the `FORWARDA` macro), with zero test coverage, so `["", "", "tag1", "tag2"]` has been silently reaching libkiwix. Fix: `v.reserve(length)` + `push_back`, or index-assign into a pre-sized vector; add a test.
14. `utils.h:346-352` — any `null` `String` argument (`Archive.getMetadata(null)`, `new Query(null)`, `Filter.lang(null)`, `Manager.readFile(null)`, etc.) hits `GetStringUTFChars(NULL)` unchecked → fatal JNI abort of the whole process, not a `NullPointerException` the caller could catch.
15. `kiwixserver.cpp:39-41` — a `catch(std::exception&)` runs *before* the project's own `CATCH_EXCEPTION()` macro, short-circuiting its specific handlers (`ZimFileFormatError`, `EntryNotFound`, `NativeHandleDisposedException`, `ios_base::failure`) and discarding `e.what()`. Every server-creation failure surfaces as the same opaque `"Error creating the server"` regardless of actual cause.
16. `utils.h:421-423` — `throwException` calls `env->ThrowNew(env->FindClass(exception), message)` with no NULL check on `FindClass`. If the exception class is missing (stripped by R8/minification, or renamed), `FindClass` returns NULL with a pending `NoClassDefFoundError`, and `ThrowNew(NULL, ...)` is itself a fatal JNI error. `ZimFileFormatException`/`EntryNotFoundException` are referenced only from C++, making them prime strip candidates — and the AAR ships **no `consumerProguardFiles`**, so a minified consuming app has no keep rules for any JNI-visible member (`nativeHandle`/`nativeHandleEnd`/`order` fields, `DirectAccessInfo` fields, `BookmarkMigrationResult` fields, `(J)V` constructors). After minification, `GetFieldID` returns NULL and the subsequent `GetLongField` aborts. Fix: null-check `FindClass` with a fallback to `java/lang/Error`; ship a `consumer-rules.pro`.
17. `utils.h:328` — `CType::type_t` is defined as `long`, which is 32 bits on `armeabi-v7a`/`x86` while `jlong` is 64 bits; `lib/build.gradle` sets no `abiFilters`, so those ABIs are built. `Filter.maxSize(long)` truncates silently; `archive.cpp:74,77` read an embedded ZIM's `offset`/`size` into a truncatable `long` before constructing `zim::FdInput` — an embedded ZIM past the 2GB mark inside a larger container file gets a silently truncated/corrupted offset on 32-bit devices. `archive.cpp:148` also passes `jlong offset, jlong size` into `zim::Archive(fd, offset, size)` with no negativity check.
18. `book.cpp:79-83` — `Book.getTagStr` uses a bare `catch(...) { return "" }` instead of `CATCH_EXCEPTION`, masking a use-after-dispose call as an empty result instead of the `IllegalStateException` every other method in the file correctly produces via `NativeHandleDisposedException`.
19. No thread-safety anywhere — no `synchronized`, no mutex, no documented threading contract on any wrapper (grep-verified). Two threads sharing an `Archive`/`Searcher`/`SearchIterator` handle race on one C++ object with no guard; the iterators are especially exposed since `hasNext()`/`next()` mutate the handle in place non-atomically.

## Simplification

- `entry_iterator.cpp:46-128` — `dispose`/`hasNext`/`next` each triplicate identical logic across a 3-way `order` switch, each paying an extra `GetFieldID("order")` lookup to pick the branch. Extract a function template parameterized on the iterator type.
- `archive.cpp:354-412` — the same 8-line EntryIterator-construction block repeated 5 times (`iterByPath`, `iterByTitle`, `iterEfficient`, `findByPath`, `findByTitle`). One `makeEntryIterator()` helper.
- `illustration.cpp:32-35` — a hand-written `dispose` that's byte-for-byte what the existing `DISPOSE` macro (`macros.h:42`) already produces.
- `utils.h` — several zero-call-site dead functions: `getHandleField` (:132-137), `getStringObjValue` (:372-378), `setIntObjValue`/`setBoolObjValue` (:388-400), `NEW_OBJECT2` (:73); a commented-out `c2jni` overload (:279-290); a shadowed `size_t index` local (:270-271).

## Minor (verified, lower impact)

- `utils.h:395-400` — `setBoolObjValue` writes a `"Z"` (boolean) field via `SetIntField`, a type mismatch — currently harmless only because the function is dead code (see Simplification).
- `search_iterator.cpp:56`, `manager.cpp:82` — `CATCH_EXCEPTION(0)` on `jstring`-returning methods; compiles fine as a null pointer constant but inconsistent with sibling methods using `nullptr`.
- `library.cpp:122`, `book.cpp:95` — `for (auto x : container)` copies each element then moves the copy; should be `auto&`.
- `archive.cpp:303-304,315-316,323-324` — `getEntryByPath(int)`/`getEntryByTitle(int)`/`getEntryByClusterOrder(int)` pass a signed `jint` straight into libzim's unsigned index type with no range check; relies entirely on libzim's internal bounds check.
- `utils.h:139-143,55-60` — `createArray`/`newObject` never check `FindClass`/`GetMethodID` for NULL — same failure class as the `throwException` finding above.
- `archive.cpp:296` — a bare `DEPRECATED` token before `GETTER(jlongArray, getIllustrationSizes)` that's defined nowhere in this repo; the build depends on it leaking in from a libzim header.

The review spans Java wrappers under lib/src/main/java/org/kiwix and JNI sources under lib/src/main/cpp, including utils.h, macros.h, archive.cpp, and iterator files. Start by selecting one numbered finding and tracing its named code path; the issue lists several expected outcomes, but no single test or scoped completion criterion is provided.

Written by the indexing model from the issue text.