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 <noreply@anthropic.com>

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 |

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<String, ContentProviderClient> 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);

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:

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".)