ReviewOS

also looking at this

craft-native/craft

fix(android): make a dropped script visible, instead of reporting it delivered

#238
Merged glennmichael123 wants to merge fix/android-deliver-reports-drop into main
6 files +131 -16
9 days ago

Closes #231.

What was wrong

fun deliver(script: String) {
    runCatching { deliverer?.invoke(script) }
}

deliverer is null for every moment before the WebView exists and after it is torn down. The script went nowhere, deliver returned normally, deliverThrough succeeded, Zig's caller reported success, and the native returned JNI_TRUE. The page was left waiting on a promise nothing would settle.

android_events.zig already keeps a dropped counter for exactly this class of failure — no VM, no attachable thread, no holder class. This drop was not one of them: it never reached the catch, so even droppedCount() read zero.

What it does now

Kotlin answers. deliver returns whether a deliverer took the script. False for no deliverer, and false for one that threw — still no throw back into Zig, which would arrive as a pending exception on a device thread with nowhere to report it.

Zig counts it. The descriptor moves from (Ljava/lang/String;)V to …)Z, and a false becomes error.NoDeliverer, which the existing catch in evaluate counts like the rest.

And says it once. Counting alone left this invisible in practice: nothing on a device reads the counter, and the runtime E2E leg reads the log. So the first drop of a process logs craft: deliver could not reach the page (NoDeliverer) — the third of the three phrases androidDeclines already matches — and every later one is counted, not logged.

That last part is the whole of the design tension in this file, and the comment on dropped states it: the paths that drop are ones a device takes in bulk, so a line each would drown the thing being diagnosed. One line per process cannot, and silence was worse than either.

warn rather than err, matching fellThrough: a drop before the WebView exists is ordinary, one after it is not, and this side cannot tell which it is looking at. The scan matches the phrase, not the level, and fails the runtime leg on either.

A seam nothing was checking

deliver is the only Kotlin method Zig calls back. #235 compares every descriptor in the natives table with its external fun, but that table is the other direction. A mismatch here is a NoSuchMethodError on the first reply, long after load, on whichever thread a callback arrived on — so the conformance test now pins this one too.

jni_runtime.zig gains callStaticBooleanMethodA; CallStaticBooleanMethodA was already in the function table, just unwrapped.

Tests that fail without the fix

mutationfails
the boolean result ignored, so a refusal reads as success again2 host tests, on the drop count
Zig back to the V descriptorthe new conformance test
Kotlin back to returning nothingthe new conformance test
the first drop never announcedthe announcement test

The host tests use the existing fake JNI table, which now answers deliver with a boolean under fake_deliverer_installed. Two cases are added: a refused delivery is counted, and a run of five drops is announced once, counted five times, and followed by a delivery that still arrives.

What this does not change

Callers of evaluate still cannot see a drop — it returns void, and threading a result through ~40 call sites is a different change. droppedCount() and the log line are the diagnosis, as before; they are just no longer blind to the most likely drop of all.

Verification

zig build test, zig build test:android, bun run test, bun run typecheck, bun run verify:packages, Pickier (0 errors), and the android package tests (98).

One thing to watch on the emulator: if a normal runtime leg now reports this line, that is the drop #231 predicted — a script handed over before installNativeDeliverer() — and worth its own issue rather than a quieter log.

6 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.