Merge pull request #22967 from hrydgard/android-java-review

Android: Java review fixes, dependency updates, leak instrumentation
This commit is contained in:
Henrik Rydgård authored and GitHub committed 2026-11-06 22:43:43 +01:00
commit cbaec7fac2
13 files changed
+216 -46

No files matched your search

+1
View File
@@ -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 |
+8 -5
View File
@@ -58,17 +58,20 @@ 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")
// 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 {
protoc {
artifact = "com.google.protobuf:protoc:3.25.3"
artifact = "com.google.protobuf:protoc:4.36.2"
}
generateProtoTasks {
+81 -27
View File
@@ -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;
@@ -72,13 +73,61 @@ 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
// 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 +135,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 +169,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);
@@ -165,6 +213,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 +256,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,11 +360,14 @@ 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 (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);
@@ -371,7 +425,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 && isRenamedCopy(name, copyName)) {
Log.w(TAG, "contentUriCopyFile: '" + name + "' is taken, removing the renamed copy " + copy);
DocumentsContract.deleteDocument(resolver, copy);
return STORAGE_ERROR_ALREADY_EXISTS;
@@ -419,11 +474,13 @@ 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 && 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);
@@ -495,16 +552,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)
@@ -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 {
@@ -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;
}
@@ -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) {
@@ -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()));
@@ -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);
@@ -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();
}
+2 -2
View File
@@ -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
}
+13
View File
@@ -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:
+47
View File
@@ -0,0 +1,47 @@
# 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.
## 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
- **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".)
+1 -1
View File
@@ -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