fix: support 16 KB Android pages#1107
Conversation
This comment has been minimized.
This comment has been minimized.
coreyphillips
left a comment
There was a problem hiding this comment.
CI is green here, but green CI never exercises the release path. Four blockers, plus a question that may let most of this PR be dropped.
Blockers
1. create-native-debug-symbols.sh fails on every release
$output is this script's own output name, but this PR turns it into a required input:
output="app/build/outputs/native-debug-symbols/$variant/native-debug-symbols-$build_number.zip"
...
if [ ! -f "$output" ]; then
echo "AGP native debug symbols archive not found at '$output'." >&2AGP 8.13.2 writes native-debug-symbols.zip with no version suffix (MergeNativeDebugMetadataTask -> withName("native-debug-symbols.zip"), confirmed against the pinned jar). The only file that has ever existed at the build-numbered name is this script's own prior output, and the Justfile runs rm -f "$symbols_dir"/native-debug-symbols*.zip before the build, so no stale copy survives either. Result: just release and both release workflows abort after a full build.
agp_archive="$output_dir/native-debug-symbols.zip"
if [ ! -f "$agp_archive" ]; then
echo "AGP native debug symbols archive not found at '$agp_archive'." >&2
exit 1
fi
copy_archive_symbols "$agp_archive" "$tmp_dir" "$application_required_libs"2. check-16kb-compat.sh aborts on macOS with no error message
validate_apk and validate_aab both do:
unzip -q "${artifact}" -d "${extracted_root}"Release APKs contain hundreds of AAPT2 entries that differ only by case (res/-b.xml vs res/-B.xml), 348 in the current release APK. On case-insensitive APFS these collide, unzip drops into its interactive replace ...? prompt and exits 1. Under set -euo pipefail the script dies there, so fail() never runs and there is no diagnostic at all: just release just stops, having validated nothing.
unzip -q -o "${artifact}" 'lib/*/*.so' 'base/lib/*/*.so' -d "${extracted_root}"That also matches what the existing "Verify native libraries are stripped" step already does.
3. The 16 KB gate is applied to 32-bit ABIs, and that is the only thing forcing the ML Kit swap
validate_archive_libraries walks every .so regardless of ABI. Android's 16 KB requirement is 64-bit only: https://developer.android.com/guide/practices/page-sizes states the Play requirement as "must support 16 KB page sizes on 64-bit devices", and its verification step inspects "arm64-v8a or x86_64" shared libraries.
I pulled barcode-scanning-17.3.0.aar from dl.google.com and read the ELF program headers. On arm64-v8a, libbarhopper_v3.so has PT_LOAD p_align = 0x4000 and PT_GNU_RELRO end 0x4b0000, which is 16 KB aligned. It passes this script's own gate on the ABI that matters. Only the 32-bit slices are 0x1000. 17.3.0 is still <latest> in Google's maven-metadata, so there is no newer bundled build pending.
Suggest skipping non arm64-v8a / x86_64 ABIs in validate_archive_libraries (abi_for_library is already there) while still requiring at least one 64-bit library to be found. zipalign -P 16 on the whole APK is correct and ABI agnostic, keep that.
4. The PT_GNU_RELRO end-alignment rule is not a 16 KB requirement
relro_end=$((virtual_address + memory_size))
if (( relro_end % PAGE_SIZE != 0 )); thenGoogle's criterion is PT_LOAD p_align >= 16384. Bionic's _phdr_table_set_gnu_relro_prot page-rounds RELRO deliberately ("we're going to be over-protective here and put every page touched by the segment as read-only"), so a non-page-aligned RELRO end is not a load or runtime failure.
As written this rule rejects Google's own current prebuilts. Measured with NDK 28 llvm-readelf -W -l, arm64-v8a:
| library | PT_LOAD p_align |
RELRO end 16 KB aligned |
|---|---|---|
| camera-core 1.5.2 (what master already ships) | 0x4000 pass |
libsurface_util_jni fails |
| camera-core 1.6.1 | 0x4000 pass |
libsurface_util_jni fails |
| graphics-path 1.0.0 / 1.0.1 / 1.1.0 | 0x4000 pass |
fails |
| jna 5.18.1 (already on master) | 0x4000 pass |
pass |
| ldk-node / vss-client / paykit / bitkit-core | 0x4000 pass |
pass |
The cause is just lld padding RELRO to -z common-page-size (4096 by default), so any future AAR built with stock NDK defaults will block a release for an incompatibility that does not exist.
Neither library is actually at risk, which I checked in the program headers: for libsurface_util_jni.so the RW LOAD segment is exactly the RELRO region, so there is nothing writable to over-protect; for graphics-path the second RW segment (0x9ed8) sits in a different 16 KB page from the RELRO region (0x5b40).
Suggest dropping the relro_end % PAGE_SIZE assertion and the relro_count > 0 requirement, keeping PT_LOAD p_align >= 0x4000, zipalign -v -c -P 16 4, and the PAGE_ALIGNMENT_16K bundle check. If RELRO padding is wanted as project policy, apply it to the ndkBuild outputs only, not to every .so in the APK.
The question this raises
With the gate corrected to Google's actual criterion, every native library in master's dependency set already passes on arm64-v8a, including bundled ML Kit. That would make both app/src/main/cpp/ (48k lines) and the ML Kit swap unnecessary.
Which library and which criterion actually produced the page-size alert on the 16 KB emulator? If it was RELRO or 32-bit only, most of this PR can be dropped.
Product impact of the ML Kit swap
This is not a version bump, it is a change of distribution model, and the PR body item 1 ("Updates ... ML Kit barcode scanning to versions that support ... 16 KB memory pages") understates it.
barcode-scanning-17.3.0.aaris 9.9 MB and shipslibbarhopper_v3.sofor four ABIs plus three.tflitemodels. Decodes fully on device, no Play services.play-services-mlkit-barcode-scanning-18.3.1.aaris 519 KB and shipsclasses.jarand a manifest. No native code, no model. All detection is delegated to Play services at runtime.
On a device without Play services, areModulesAvailable fails, isScannerReady never becomes true, analyze() drops every frame, the gallery path never reaches processImage, and the new Retry button can never succeed. There is no fallback: com.google.zxing:core is a dependency but is only used for encoding in QrCodeImage.kt, and there is no GoogleApiAvailability handling anywhere in the repo. We distribute sideloadable APKs on GitHub releases, so this is not only a Play Store audience.
There is also an offline regression for Play Store users' first scan: the com.google.mlkit.vision.DEPENDENCIES manifest pre-download only applies to Play Store installs, so a sideloaded build has to fetch the module from Google over the network before the first scan works.
If the swap stays, it needs a zxing decode fallback for the unavailable case and explicit product sign-off. Either way the changelog fragment needs to say it: "Improves native library compatibility on Android devices using 16 KB memory pages" gives a user whose scanner stopped working nothing to go on.
Other findings
QrCodeAnalyzer.kt, spurious "Scan failed" toast after a successful scan. analyzer.close() in onDispose calls scanner.close(), which cancels in-flight ML Kit tasks. analyze() registers addOnCompleteListener with no executor and no closed guard, and a canceled Task reports isSuccessful=false with a null exception, so it.exception ?: AppError("Scan failed") fires on the main thread after dispose. Scan a valid QR in the Send sheet and the error toast lands on the Amount screen. QrImageScanOperation already gets this right with success/failure listeners plus closed/completed guards. Suggest a @Volatile private var closed, set it and isScannerReady = false in close(), return early from analyze() and from the listener, and skip it.isCanceled explicitly.
QrScanningScreen.kt / SendRecipientScreen.kt, raw exception text shown to users. toastQrScanError's non-model branch is toast(error), which renders error.message as the toast description. This replaces the localized other__qr_error_header / other__qr_error_text, so a camera scan failure now shows raw English ML Kit strings ("This detector is already closed!", "Scan failed"). Keep the localized copy in that branch and log the throwable via Logger.error(..., context = TAG).
No version lockstep between the vendored sources and the AARs. pickFirsts makes the locally built .so permanently shadow the AAR copy, but the camera = "1.6.1" / graphics-path = "1.1.0" pairing is recorded only in prose in third_party/androidx/README.md. graphics-path is a soft require and Compose already pulls it transitively at 1.0.1, so a routine compose-bom bump can resolve a newer AAR against a .so built from 1.1.0 sources. pathway.cpp does RegisterNatives with hardcoded descriptors, so a signature change there is an UnsatisfiedLinkError on API 28 to 33 with every build-time check still green. Suggest asserting in NativeReleaseConfigTest.kt that the versions in libs.versions.toml match the README, and/or strictly(...) pins so a transitive upgrade fails resolution instead. (Provenance itself is exact: the vendored files are byte-identical to upstream at the pinned SHAs.)
Nits
org.mockito.kotlin.any()is written inline fully qualified 11 times (9 inBarcodeModelInstallerTest.kt, 2 inQrImageScanOperationTest.kt) while five other symbols from the same package are imported. No other test file in the repo does this. Addimport org.mockito.kotlin.any.- Trailing comma after the last
modifier = ...argument at both newPrimaryButtoncalls (QrScanningScreen.kt:351,SendRecipientScreen.kt:455). Repo convention is no comma there: 242 modifier chains underui/end without one, 10 with. BarcodeModelUnavailableExceptionextendsExceptionrather thanAppError. Related,QrScanningScreenstill doesonError(Exception("No QR code found in the image"))whereSendRecipientScreencorrectly usesAppError(context.getString(R.string.other__qr_error_text)).
Conflicts with #1106
git merge-tree gives conflicts in three files: scripts/create-native-debug-symbols.sh, app/src/test/java/to/bitkit/build/NativeReleaseConfigTest.kt, gradle/libs.versions.toml. release-internal.yml and app/build.gradle.kts auto-merge cleanly.
The one to watch: #1106 adds libpaykit.so to required_libs, this PR splits the same line into dependency_required_libs plus application_required_libs. Taking this PR's side of the hunk silently drops libpaykit.so from the release symbol gate even though both artifacts ship it, and then NativeReleaseConfigTest fails either way, where the least-effort green fix cements the gap. Correct resolution:
dependency_symbols_dir=${NATIVE_SYMBOLS_DEPENDENCY_DIR:-app/build/intermediates/native-debug-symbol-artifacts}
dependency_required_libs="libbitkitcore.so libldk_node.so libpaykit.so libvss_rust_client_ffi.so"
application_required_libs="libimage_processing_util_jni.so libsurface_util_jni.so libandroidx.graphics.path.so"
required_libs="$dependency_required_libs $application_required_libs"Also, after both land, release-internal.yml builds an AAB that no step in that workflow inspects: release.yml passes both directories to the 16 KB check, the internal workflow passes only the APK dir and has no Setup bundletool step.
Verified as fine, no action needed
pickFirsts ordering is deterministic (AGP visits project native libs before external ones, with isReproducibleFileOrder()); APP_STL=c++_static with -nostdlib++ is sound and a missing libc++ symbol would be a link error, not a runtime one; the vendored trees are byte-identical to upstream at the pinned SHAs and the JNI signatures match camera-core 1.6.1 exactly; license packaging is adequate (no upstream NOTICE files exist at those commits, and the sources are unmodified); the libyuv subset resolves all referenced symbols; the ELF field parsing in check-16kb-compat.sh is correct for both 32- and 64-bit layouts; the bundletool pin and its SHA-256 are correct; strings.xml ordering and the values/-only placement match repo convention; NativeReleaseConfigTest asserting on generated assets is safe because preBuild carries the dependency; and ndkVersion matches the Justfile and both workflows.
Fixes #1103
This PR:
PT_LOADsegments, andGNU_RELROalignment across all packaged ABIs.QA Notes
Manual Tests
mainnetDebuginstallation on an Android 17 emulator using a true 16 KB page size → Confirm Android shows no page-size compatibility alert.Automated Checks
just compilejust testjust lintmasterAPKactionlint,shellcheck,bash -nfor the native validation and Bundletool provisioning scripts, andgit diff --check