From 4abd722b54e2afed059ea54c362919318ea55e95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 6 Nov 2026 21:29:40 +0100 Subject: [PATCH] Android: Cache one provider client per authority, add an Android todo doc A query on a second authority closed the cached client while another thread could still be using it. Now each authority keeps its own client, dropped only when its provider dies. docs/android-todo.md lists the review findings left for later (image picker decoding on the main thread, the VR exit in onDestroy, the achievements host override receiver), and android-device-testing.md describes the LeakCanary/StrictMode setup. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + android/src/org/ppsspp/ppsspp/ContentUri.java | 37 ++++++------- docs/android-device-testing.md | 13 +++++ docs/android-todo.md | 53 +++++++++++++++++++ 4 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 docs/android-todo.md diff --git a/AGENTS.md b/AGENTS.md index 283b689660..970436ee14 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,6 +13,7 @@ for it: |---|---| | [docs/building.md](docs/building.md) | Build commands for every target (VS/MSBuild, CMake, UWP, legacy Android NDK, libretro), unit tests, pspautotests | | [docs/android-device-testing.md](docs/android-device-testing.md) | Running a build on a real Android device or a Quest over adb: installing the build you think you are, forcing activity recreates, measuring retained activities, VR, running the C++ unit tests on a device (`Tools/android_unittest.py`), starting the app with options (`Tools/android_run.py`), and the storage tests that exercise content URIs in the app (`Tools/android_storage_test.py`) | +| [docs/android-todo.md](docs/android-todo.md) | Known Android Java issues found in reviews and not fixed yet - check before reviewing or changing that code | | [docs/debugging.md](docs/debugging.md) | Driving the WebSocket debugger and PPSSPPHeadless from a script, measuring a commercial game with headless, comparing binaries' speed (`Tools/headless_bench.py`), finding a game's hot JIT blocks (`--jit-profile`), breakpoint reliability per CPU backend, debugging a game that works on hardware, finding and hashing a game function (like its own `memcpy`) for `ReplaceTables` when hardware backends miss CPU writes to VRAM | | [docs/DebuggerThreading.md](docs/DebuggerThreading.md) | `Core_RunOnCPUThread` / `g_frameMutex` / shutdown-lock rules - required reading before touching debugger code | | [docs/HLEModules.md](docs/HLEModules.md) | Adding an HLE module or function, and the seven build files a new source file goes in | diff --git a/android/src/org/ppsspp/ppsspp/ContentUri.java b/android/src/org/ppsspp/ppsspp/ContentUri.java index 077a565706..0fecccb146 100644 --- a/android/src/org/ppsspp/ppsspp/ContentUri.java +++ b/android/src/org/ppsspp/ppsspp/ContentUri.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Date; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Locale; @@ -76,9 +77,10 @@ public class ContentUri { // releases it again, and when our reference count drops to zero, each of those is another binder call, // to the system. Holding one keeps the count up. Unstable, so the provider dying doesn't take our process // with it: queries that then fail drop it, and the next one acquires a new one. + // One per authority, and only dropped when its provider dies, so a client is never closed while another + // thread is still using it. There are rarely more than one or two. private static final Object clientLock = new Object(); - private static ContentProviderClient client; - private static String clientAuthority; + private static final HashMap clients = new HashMap<>(); private static ContentProviderClient getClient(Context context, Uri uri) { final String authority = uri.getAuthority(); @@ -86,28 +88,31 @@ public class ContentUri { return null; } synchronized (clientLock) { - if (client != null && authority.equals(clientAuthority)) { - return client; + ContentProviderClient client = clients.get(authority); + if (client == null) { + // The application's resolver, so the client doesn't keep the activity alive. + client = context.getApplicationContext().getContentResolver().acquireUnstableContentProviderClient(authority); + if (client != null) { + clients.put(authority, client); + } } - dropClientLocked(); - // The application's resolver, so the client doesn't keep the activity alive. - client = context.getApplicationContext().getContentResolver().acquireUnstableContentProviderClient(authority); - clientAuthority = authority; return client; } } - private static void dropClientLocked() { - if (client == null) { - return; + // Drops the client if it's still the one cached for its authority (another thread may have replaced it already). + private static void dropClient(String authority, ContentProviderClient client) { + synchronized (clientLock) { + if (clients.get(authority) != client) { + return; + } + clients.remove(authority); } if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { client.close(); } else { client.release(); } - client = null; - clientAuthority = null; } private static Cursor query(Context context, Uri uri, String[] projection) throws Exception { @@ -117,11 +122,7 @@ public class ContentUri { return c.query(uri, projection, null, null, null); } catch (android.os.DeadObjectException e) { Log.w(TAG, "Storage provider died, acquiring it again"); - synchronized (clientLock) { - if (client == c) { - dropClientLocked(); - } - } + dropClient(uri.getAuthority(), c); } } return context.getContentResolver().query(uri, projection, null, null, null); diff --git a/docs/android-device-testing.md b/docs/android-device-testing.md index a89ef785b3..0ae0d2775b 100644 --- a/docs/android-device-testing.md +++ b/docs/android-device-testing.md @@ -70,6 +70,19 @@ stable across repeated trims. After: the same 13 recreates settled back to 1 and A JNI global reference is a GC root, so an activity pinned by one shows up here. This is a usable check for JNI ownership bugs, not only Java ones. +### LeakCanary and StrictMode + +Debug builds set a StrictMode VM policy that logs leaked closables, unregistered receivers and leaked +activities under the tag `StrictMode`. Building with `-PleakCanary` also adds LeakCanary to the debug +build: it logs `Watching instance of ...` for each destroyed activity, and `Found N objects retained` +(then dumps and analyses the heap) only if one isn't collected. No such line after a run of recreates +means nothing was retained. It adds a second launcher activity to the package, so start the app with +`adb shell am start -n org.ppsspp.ppsspp/.PpssppActivity` rather than `monkey`, which can pick +LeakCanary's. + +On a device where `persist.log.tag` is `S`, enable the tags first: +`adb shell setprop log.tag.LeakCanary V`, and the same for `StrictMode`. + ## logcat on a noisy device Filter to the process, since the tag alone is not enough: diff --git a/docs/android-todo.md b/docs/android-todo.md new file mode 100644 index 0000000000..4e2f6bcd28 --- /dev/null +++ b/docs/android-todo.md @@ -0,0 +1,53 @@ +# Android: known issues to fix later + +Found in reviews of the Java code, not yet fixed. Line numbers drift; search for the names. + +## Image picker decodes on the main thread + +`ImageResultProxyActivity.copyAndDownscaleToCache` runs in the activity result callback, on the main +thread. It opens the picked image's stream three times (bounds, orientation, decode) and decodes it, +then writes the downscaled copy to the cache. With a cloud-backed provider (Google Photos fetching the +original) or a very large image, that can take long enough to ANR. + +Fix: do the copy on a background thread, then call `returnWithResult` back on the UI thread. While +there: it doesn't fall back to `getClipData()` when `getData()` is null, though the comment near the top +says some pickers do that (`DocumentResultProxyActivity` handles it). + +## VR: `System.exit(0)` in `onDestroy` + +`PpssppActivity.onDestroy` ends with `if (isVRDevice() && !restarting) System.exit(0);`, a workaround for +OpenXR not surviving a restart. Only `graphics_restart` sets `restarting`, so: + +- The `recreate` command, and any config change that recreates the activity, kill the process + mid-recreate. +- After a finish and quick relaunch, the old activity's late `onDestroy` (which `checkCurrent` guards + against for the native shutdown just above it) kills the process the new activity runs in. + +Probably the exit should also be skipped when `isChangingConfigurations()` is true or the activity isn't +current. Needs testing on a Quest, which is why it wasn't changed blind. + +## Achievements host override receiver is open to any app + +`AchievementsHostOverrideReceiver` (PR #21760) is exported without a permission, so that a companion +app can point RetroAchievements at a local server. Any installed app can send it, though, and since +loopback is shared between apps, the "loopback only" check doesn't help: an app listening on +`127.0.0.1` can set itself as the host and receive the user's RA login and token. The override persists +across launches. + +`android:permission="android.permission.DUMP"` would restrict it to adb, but locks out the companion +app it was made for. Options: a signature permission (only if the companion is ours), or keep it open +but only apply an override after the user confirms it in PPSSPP. Also, `onReceive` calls +`NativeApp.setAchievementsHostOverride` on the main thread, which writes `g_Config` fields other +threads read. + +## Smaller ones + +- **Sanitised names read as conflicts:** `ContentUri` treats a name read back that differs from the + one asked for as "name taken" (the provider made `name (1)` instead) in `createDocument`, copy and + rename. `ExternalStorageProvider` also replaces FAT-invalid characters, which gives a false + `ALREADY_EXISTS`. Only report a conflict when the result looks like `name (N)`. +- **Denied permissions get no answer:** a denied location or microphone permission sends nothing to + native, so the game's request just stays silent. Storage and local network do report denial. +- **Picker result after an activity recreate:** if the activity was destroyed and recreated while a + picker proxy was open, check that the result still arrives through `onNewIntent`. (It should be + queued for the recreated activity, but nobody has tested this with "Don't keep activities".)