From 98c127bbb365f97e243e8faf3c1a6ff230ca4f3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 6 Nov 2026 20:57:56 +0100 Subject: [PATCH 1/4] Android: Update Gradle to 9.9.0, AGP to 9.5.1, protobuf to 4.36.2 protoc was still on 3.25.3 while the runtime was on 4.x; both are now 4.36.2. appcompat stays at 1.7.1, since 1.8.0 needs minSdk 23. Co-Authored-By: Claude Opus 5.5 --- android/build.gradle.kts | 9 ++++----- build.gradle.kts | 4 ++-- gradle/wrapper/gradle-wrapper.properties | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/android/build.gradle.kts b/android/build.gradle.kts index 4ddb43ce00..656497a809 100644 --- a/android/build.gradle.kts +++ b/android/build.gradle.kts @@ -58,17 +58,16 @@ val gitVersionCode = commitsSinceTag dependencies { - // 1.6.1 is the newest version we can use that won't complain about minSdk version, - // and also doesn't collide kotlin versions with com.gladed.androidgitversion. - // Will replace with a different plugin soon. + // appcompat 1.8.0 and later need minSdk 23. implementation("androidx.appcompat:appcompat:1.7.1") implementation("androidx.documentfile:documentfile:1.1.0") - implementation("com.google.protobuf:protobuf-javalite:4.35.1") + // Keep the runtime and protoc versions in step. + implementation("com.google.protobuf:protobuf-javalite:4.36.2") } protobuf { protoc { - artifact = "com.google.protobuf:protoc:3.25.3" + artifact = "com.google.protobuf:protoc:4.36.2" } generateProtoTasks { diff --git a/build.gradle.kts b/build.gradle.kts index b1a0884fb3..b496fd5d5b 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -1,4 +1,4 @@ plugins { - id("com.android.application") version "9.5.0" apply false - id("com.google.protobuf") version "0.10.0" apply false + id("com.android.application") version "9.5.1" apply false + id("com.google.protobuf") version "0.10.1" apply false } diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index b2eeda23ea..7cdca83086 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,6 +1,6 @@ #Wed Oct 14 18:07:36 CEST 2026 distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-9.8.0-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-9.9.0-bin.zip zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists From 156b9b3ee64457101b379bac17f5be025a7c4500 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 6 Nov 2026 21:13:52 +0100 Subject: [PATCH 2/4] Android: Fixes from a review of the Java code, debug leak instrumentation - A recreate (font scale, resolution change) re-sent the launch intent's game to native, which booted it over whatever was running. - Exceptions in posted commands (bad parameters, setPrimaryClip) no longer crash the app; the request is answered as failed. - Close tombstone streams and the free-space fd on error paths. - Keep the CameraHelper across graphics restarts (it leaked a SurfaceTexture). - Don't treat an unreadable document name as a name conflict, which deleted the file just created. - Tree picker: convert to a document URI even if the persistable grant fails. - ShortcutActivity: don't open a second picker on recreate, and finish when the old file chooser is backed out of. - Skip null names/MIME types in listings, null device in the Joy-Con check. Debug builds now set a StrictMode VM policy (leaked closables, registrations, activities), and -PleakCanary adds LeakCanary to debug builds. Co-Authored-By: Claude Opus 5.5 --- android/build.gradle.kts | 4 +++ android/src/org/ppsspp/ppsspp/ContentUri.java | 22 +++++++----- .../ppsspp/DocumentResultProxyActivity.java | 18 +++++++--- .../org/ppsspp/ppsspp/InputDeviceState.java | 6 +++- .../src/org/ppsspp/ppsspp/PpssppActivity.java | 34 +++++++++++++++++-- .../org/ppsspp/ppsspp/ShortcutActivity.java | 5 +++ .../org/ppsspp/ppsspp/SimpleFileChooser.java | 2 ++ 7 files changed, 73 insertions(+), 18 deletions(-) diff --git a/android/build.gradle.kts b/android/build.gradle.kts index 656497a809..2f2bce04d0 100644 --- a/android/build.gradle.kts +++ b/android/build.gradle.kts @@ -63,6 +63,10 @@ dependencies { implementation("androidx.documentfile:documentfile:1.1.0") // Keep the runtime and protoc versions in step. implementation("com.google.protobuf:protobuf-javalite:4.36.2") + // Leak detection for debug builds, opt-in since its heap dumps freeze the app: ./gradlew -PleakCanary ... + if (project.hasProperty("leakCanary")) { + debugImplementation("com.squareup.leakcanary:leakcanary-android:2.14") + } } protobuf { diff --git a/android/src/org/ppsspp/ppsspp/ContentUri.java b/android/src/org/ppsspp/ppsspp/ContentUri.java index 0aeb7c36c7..077a565706 100644 --- a/android/src/org/ppsspp/ppsspp/ContentUri.java +++ b/android/src/org/ppsspp/ppsspp/ContentUri.java @@ -165,6 +165,9 @@ public class ContentUri { return null; } final String documentName = c.getString(0); + if (documentName == null) { + return null; + } if (prefixLowercase != null && !prefixLowercase.isEmpty()) { // Filter by prefix, case insensitive. if (!documentName.regionMatches(true, 0, prefixLowercase, 0, prefixLowercase.length())) { @@ -205,7 +208,7 @@ public class ContentUri { if (c != null) { while (c.moveToNext()) { final String mimeType = c.getString(2); - final boolean isDirectory = mimeType.equals(DocumentsContract.Document.MIME_TYPE_DIR); + final boolean isDirectory = DocumentsContract.Document.MIME_TYPE_DIR.equals(mimeType); if (isDirectory) { final String childDocumentId = c.getString(0); final Uri childUri = DocumentsContract.buildDocumentUriUsingTree(uri, childDocumentId); @@ -309,7 +312,9 @@ public class ContentUri { if (created == null) { return STORAGE_ERROR_UNKNOWN; } - if (!name.equals(documentName(activity, created))) { + // Null if the provider wouldn't say, which isn't a conflict. + final String createdName = documentName(activity, created); + if (createdName != null && !name.equals(createdName)) { Log.w(TAG, "createDocument: '" + name + "' is taken, removing the renamed copy " + created); DocumentsContract.deleteDocument(resolver, created); return STORAGE_ERROR_ALREADY_EXISTS; @@ -371,7 +376,8 @@ public class ContentUri { } // A taken name gets a renamed copy, see createDocument. final String name = documentName(activity, srcUri); - if (name != null && !name.equals(documentName(activity, copy))) { + final String copyName = documentName(activity, copy); + if (name != null && copyName != null && !name.equals(copyName)) { Log.w(TAG, "contentUriCopyFile: '" + name + "' is taken, removing the renamed copy " + copy); DocumentsContract.deleteDocument(resolver, copy); return STORAGE_ERROR_ALREADY_EXISTS; @@ -419,7 +425,8 @@ public class ContentUri { final String oldName = documentName(activity, uri); final Uri newUri = DocumentsContract.renameDocument(resolver, uri, newName); // A taken name gets "name (1)" instead, see createDocument. Put the old name back. - if (newUri != null && oldName != null && !newName.equals(documentName(activity, newUri))) { + final String renamedName = newUri != null ? documentName(activity, newUri) : null; + if (oldName != null && renamedName != null && !newName.equals(renamedName)) { Log.w(TAG, "contentUriRenameFileTo: '" + newName + "' is taken, renaming back to '" + oldName + "'"); DocumentsContract.renameDocument(resolver, newUri, oldName); return STORAGE_ERROR_ALREADY_EXISTS; @@ -495,16 +502,13 @@ public class ContentUri { @Keep @SuppressWarnings("unused") public static long contentUriGetFreeStorageSpaceSlow(Activity activity, Uri uri) { - try { - ParcelFileDescriptor pfd = activity.getContentResolver().openFileDescriptor(uri, "r"); + try (ParcelFileDescriptor pfd = activity.getContentResolver().openFileDescriptor(uri, "r")) { if (pfd == null) { Log.w(TAG, "Failed to get free storage space from URI: " + uri); return -1; } StructStatVfs stats = Os.fstatvfs(pfd.getFileDescriptor()); - long freeSpace = stats.f_bavail * stats.f_bsize; - pfd.close(); - return freeSpace; + return stats.f_bavail * stats.f_bsize; } catch (Exception e) { // FileNotFoundException | ErrnoException e // Log.getStackTraceString(e) diff --git a/android/src/org/ppsspp/ppsspp/DocumentResultProxyActivity.java b/android/src/org/ppsspp/ppsspp/DocumentResultProxyActivity.java index cee38a4691..7ac345ff13 100644 --- a/android/src/org/ppsspp/ppsspp/DocumentResultProxyActivity.java +++ b/android/src/org/ppsspp/ppsspp/DocumentResultProxyActivity.java @@ -52,19 +52,27 @@ public class DocumentResultProxyActivity extends AppCompatActivity { if (uri != null) { Log.i(TAG, "DocumentResultProxy: Selected URI: " + uri); + final boolean isTree = pickerIntent != null && Intent.ACTION_OPEN_DOCUMENT_TREE.equals(pickerIntent.getAction()); try { - if (pickerIntent != null && Intent.ACTION_OPEN_DOCUMENT_TREE.equals(pickerIntent.getAction())) { + if (isTree) { getContentResolver().takePersistableUriPermission(uri, Intent.FLAG_GRANT_READ_URI_PERMISSION | Intent.FLAG_GRANT_WRITE_URI_PERMISSION); + } else { + getContentResolver().takePersistableUriPermission(uri, Intent.FLAG_GRANT_READ_URI_PERMISSION); + } + } catch (Exception e) { + Log.w(TAG, "DocumentResultProxy: Exception taking the URI permission: " + e); + } + // Even without a persistable grant, native needs the document URI, not the bare tree URI. + if (isTree) { + try { DocumentFile documentFile = DocumentFile.fromTreeUri(this, uri); if (documentFile != null) { uri = documentFile.getUri(); Log.i(TAG, "DocumentResultProxy: DocumentFile URI: " + uri); } - } else { - getContentResolver().takePersistableUriPermission(uri, Intent.FLAG_GRANT_READ_URI_PERMISSION); + } catch (Exception e) { + Log.w(TAG, "DocumentResultProxy: Exception getting DocumentFile: " + e); } - } catch (Exception e) { - Log.w(TAG, "DocumentResultProxy: Exception getting permissions or DocumentFile: " + e); } resultPath = uri.toString(); } else { diff --git a/android/src/org/ppsspp/ppsspp/InputDeviceState.java b/android/src/org/ppsspp/ppsspp/InputDeviceState.java index 18085c622f..982117ee50 100644 --- a/android/src/org/ppsspp/ppsspp/InputDeviceState.java +++ b/android/src/org/ppsspp/ppsspp/InputDeviceState.java @@ -220,7 +220,11 @@ public class InputDeviceState { } private boolean isEventSentByNintendoSwitchLeftJoyCon(KeyEvent event) { - return event.getDevice().getName().contains("Nintendo Switch Left Joy-Con"); + InputDevice device = event.getDevice(); + if (device == null || device.getName() == null) { + return false; + } + return device.getName().contains("Nintendo Switch Left Joy-Con"); } private int remapNintendoSwitchLeftJoyConKeyCodeFromScanCode(int scanCode) { diff --git a/android/src/org/ppsspp/ppsspp/PpssppActivity.java b/android/src/org/ppsspp/ppsspp/PpssppActivity.java index 25e3e0f5b6..7229ceffba 100644 --- a/android/src/org/ppsspp/ppsspp/PpssppActivity.java +++ b/android/src/org/ppsspp/ppsspp/PpssppActivity.java @@ -35,6 +35,7 @@ import android.os.Environment; import android.os.Handler; import android.os.Looper; import android.os.PowerManager; +import android.os.StrictMode; import android.provider.MediaStore; import android.provider.Settings; import android.text.InputType; @@ -548,7 +549,9 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList mInfraredHelper = null; Log.i(TAG, "InfraredHelper exception: " + e); } - mCameraHelper = new CameraHelper(getApplicationContext()); + if (mCameraHelper == null) { + mCameraHelper = new CameraHelper(getApplicationContext()); + } } @NonNull @@ -725,6 +728,20 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); + if (BuildConfig.DEBUG) { + // Logs unclosed streams/cursors, unregistered receivers and leaked activities (tag StrictMode). + StrictMode.VmPolicy.Builder vmPolicy = new StrictMode.VmPolicy.Builder() + .detectLeakedClosableObjects() + .detectLeakedRegistrationObjects() + .detectLeakedSqlLiteObjects() + .detectActivityLeaks() + .penaltyLog(); + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { + vmPolicy.detectContentUriWithoutPermission(); + } + StrictMode.setVmPolicy(vmPolicy.build()); + } + if (m_hasNoNativeBinary) { // Nothing else gets set up, so everything else that Android calls has to check for this too // or we'll crash before the user gets to read the message. @@ -808,7 +825,8 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList // The render loop thread will be started once we get a surface. - if (!firstRun && shortcutParam != null && !shortcutParam.isEmpty()) { + // A recreate (savedInstanceState set) gets the same intent back, which was already acted on. + if (!firstRun && savedInstanceState == null && shortcutParam != null && !shortcutParam.isEmpty()) { // The native side is already up, so it didn't see this through NativeApp.init. Log.i(TAG, "Got shortcutParam in onCreate on secondary run: " + shortcutParam); NativeApp.sendMessageFromJava("shortcutParam", shortcutParam); @@ -2221,7 +2239,15 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList failRequestCommand(command, parameter); return; } - if (!processCommand(command, parameter)) { + boolean success; + try { + success = processCommand(command, parameter); + } catch (Exception e) { + // A bad parameter, or a system service throwing (setPrimaryClip does on some devices). + NativeApp.reportException(e, command); + success = false; + } + if (!success) { // Not logging the parameter, an inputbox one can contain a password. Log.e(TAG, "processCommand failed: cmd: '" + command + "'"); failRequestCommand(command, parameter); @@ -2345,6 +2371,8 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList // use tombstone... } catch (Exception e) { Log.e(TAG, e.toString()); + } finally { + trace.close(); } } else { Log.e(TAG, "Trace input stream was null, ignoring. Reason: " + exitReasonToString(aei.getReason())); diff --git a/android/src/org/ppsspp/ppsspp/ShortcutActivity.java b/android/src/org/ppsspp/ppsspp/ShortcutActivity.java index 932fcffff1..a762c37b99 100644 --- a/android/src/org/ppsspp/ppsspp/ShortcutActivity.java +++ b/android/src/org/ppsspp/ppsspp/ShortcutActivity.java @@ -37,6 +37,11 @@ public class ShortcutActivity extends Activity { // Ensure library is loaded before any native methods are called. PpssppActivity.CheckABIAndLoadLibrary(); + // Recreated while the picker was open: its result is still on the way. + if (savedInstanceState != null) { + return; + } + // Show file selector dialog here. If Android version is more than or equal to 11, // use the native document file browser instead of our SimpleFileChooser. boolean scoped = (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R); diff --git a/android/src/org/ppsspp/ppsspp/SimpleFileChooser.java b/android/src/org/ppsspp/ppsspp/SimpleFileChooser.java index 252d1adb1f..4fe28b4488 100644 --- a/android/src/org/ppsspp/ppsspp/SimpleFileChooser.java +++ b/android/src/org/ppsspp/ppsspp/SimpleFileChooser.java @@ -39,6 +39,8 @@ public class SimpleFileChooser { builder.setTitle(mCurrentPath.getPath()); // populate dialog with list of files and directories. builder.setItems(mFileList, onDialogItemClickedListener); + // Backing out shouldn't leave the (empty) activity behind. + builder.setOnCancelListener(dialog -> mActivity.finish()); builder.show(); } 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 3/4] 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".) From b35bfcc869616ccf4b3983ae9203f2490287ddbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Fri, 6 Nov 2026 21:37:24 +0100 Subject: [PATCH 4/4] Android: Only count a "name (N)" result as a name conflict The provider answers a taken name with a renamed copy, "name (1)" or "name (1).ext", but it also replaces characters FAT doesn't allow with '_', which isn't a conflict. Create, copy and rename now only report ALREADY_EXISTS (and undo) for the numbered form of the sanitised name. Also take the image picker's result from ClipData when there's no data URI, like the document picker does. Co-Authored-By: Claude Opus 5.5 --- android/src/org/ppsspp/ppsspp/ContentUri.java | 55 ++++++++++++++++++- .../ppsspp/ImageResultProxyActivity.java | 9 ++- docs/android-todo.md | 8 +-- 3 files changed, 60 insertions(+), 12 deletions(-) diff --git a/android/src/org/ppsspp/ppsspp/ContentUri.java b/android/src/org/ppsspp/ppsspp/ContentUri.java index 0fecccb146..855e65bc11 100644 --- a/android/src/org/ppsspp/ppsspp/ContentUri.java +++ b/android/src/org/ppsspp/ppsspp/ContentUri.java @@ -73,6 +73,53 @@ public class ContentUri { } } + // Whether the provider made a renamed copy because the name was taken. It answers that with "name (1)", or + // "name (1).ext" for a file (FileUtils.buildUniqueFile), and before that replaces the characters FAT doesn't + // allow with '_' (buildValidFatFilename). The replacement alone isn't a conflict, the name was free. + private static boolean isRenamedCopy(String requested, String actual) { + if (actual == null || actual.equals(requested)) { + return false; + } + final String sanitized = fatSanitized(requested); + // Directories, and files the provider didn't split an extension off, get the number at the end. + if (isNumbered(actual, sanitized, "")) { + return true; + } + final int dot = sanitized.lastIndexOf('.'); + return dot > 0 && isNumbered(actual, sanitized.substring(0, dot), sanitized.substring(dot)); + } + + // Whether name is "base (N)ext". + private static boolean isNumbered(String name, String base, String ext) { + final int digitsStart = base.length() + 2; + final int digitsEnd = name.length() - ext.length() - 1; + if (digitsEnd <= digitsStart || !name.startsWith(base + " (") || !name.endsWith(")" + ext)) { + return false; + } + for (int i = digitsStart; i < digitsEnd; i++) { + final char ch = name.charAt(i); + if (ch < '0' || ch > '9') { + return false; + } + } + return true; + } + + private static String fatSanitized(String name) { + final StringBuilder sb = new StringBuilder(name.length()); + for (int i = 0; i < name.length(); i++) { + final char ch = name.charAt(i); + sb.append((ch <= 0x1f || ch == 0x7f || "\"*/:<>?\\|".indexOf(ch) >= 0) ? '_' : ch); + } + return sb.toString(); + } + + private static void logIfNameChanged(String what, String requested, String actual) { + if (actual != null && !actual.equals(requested)) { + Log.w(TAG, what + ": the provider named '" + requested + "' '" + actual + "'"); + } + } + // Without a client of our own, every query and DocumentsContract call acquires the storage provider and // 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 @@ -315,11 +362,12 @@ public class ContentUri { } // Null if the provider wouldn't say, which isn't a conflict. final String createdName = documentName(activity, created); - if (createdName != null && !name.equals(createdName)) { + if (isRenamedCopy(name, createdName)) { Log.w(TAG, "createDocument: '" + name + "' is taken, removing the renamed copy " + created); DocumentsContract.deleteDocument(resolver, created); return STORAGE_ERROR_ALREADY_EXISTS; } + logIfNameChanged("createDocument", name, createdName); return STORAGE_ERROR_SUCCESS; } catch (Exception e) { Log.e(TAG, "createDocument exception: " + e); @@ -378,7 +426,7 @@ public class ContentUri { // A taken name gets a renamed copy, see createDocument. final String name = documentName(activity, srcUri); final String copyName = documentName(activity, copy); - if (name != null && copyName != null && !name.equals(copyName)) { + if (name != null && isRenamedCopy(name, copyName)) { Log.w(TAG, "contentUriCopyFile: '" + name + "' is taken, removing the renamed copy " + copy); DocumentsContract.deleteDocument(resolver, copy); return STORAGE_ERROR_ALREADY_EXISTS; @@ -427,11 +475,12 @@ public class ContentUri { final Uri newUri = DocumentsContract.renameDocument(resolver, uri, newName); // A taken name gets "name (1)" instead, see createDocument. Put the old name back. final String renamedName = newUri != null ? documentName(activity, newUri) : null; - if (oldName != null && renamedName != null && !newName.equals(renamedName)) { + if (oldName != null && isRenamedCopy(newName, renamedName)) { Log.w(TAG, "contentUriRenameFileTo: '" + newName + "' is taken, renaming back to '" + oldName + "'"); DocumentsContract.renameDocument(resolver, newUri, oldName); return STORAGE_ERROR_ALREADY_EXISTS; } + logIfNameChanged("contentUriRenameFileTo", newName, renamedName); return STORAGE_ERROR_SUCCESS; } catch (Exception e) { Log.e(TAG, "contentUriRenameFile exception: " + e); diff --git a/android/src/org/ppsspp/ppsspp/ImageResultProxyActivity.java b/android/src/org/ppsspp/ppsspp/ImageResultProxyActivity.java index 8024009fcf..70ad401859 100644 --- a/android/src/org/ppsspp/ppsspp/ImageResultProxyActivity.java +++ b/android/src/org/ppsspp/ppsspp/ImageResultProxyActivity.java @@ -24,7 +24,6 @@ public class ImageResultProxyActivity extends AppCompatActivity { private String copyAndDownscaleToCache(Uri selectedImage) { Log.i(TAG, "Selected image: " + selectedImage); if (selectedImage == null) { - // Some pickers only return a ClipData. Log.e(TAG, "No image URI in the result"); return null; } @@ -147,7 +146,13 @@ public class ImageResultProxyActivity extends AppCompatActivity { String localPath; if (result.getResultCode() == Activity.RESULT_OK && result.getData() != null) { - localPath = copyAndDownscaleToCache(result.getData().getData()); + Intent data = result.getData(); + Uri uri = data.getData(); + // Some pickers only return a ClipData. + if (uri == null && data.getClipData() != null && data.getClipData().getItemCount() > 0) { + uri = data.getClipData().getItemAt(0).getUri(); + } + localPath = copyAndDownscaleToCache(uri); } else { localPath = null; } diff --git a/docs/android-todo.md b/docs/android-todo.md index 4e2f6bcd28..662a2c2fa8 100644 --- a/docs/android-todo.md +++ b/docs/android-todo.md @@ -9,9 +9,7 @@ thread. It opens the picked image's stream three times (bounds, orientation, dec 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). +Fix: do the copy on a background thread, then call `returnWithResult` back on the UI thread. ## VR: `System.exit(0)` in `onDestroy` @@ -42,10 +40,6 @@ 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