[dennytosp]

Fixes #3834

What

Requesting more than one permission at a time on Android — e.g. camera, microphone and location from the same useEffect — leaves every requestPermission() Promise hanging. Nitro eventually destroys them, and the app sees:

ERROR  [Error: Uncaught (in promise, id: 0) Error: java.lang.RuntimeException: Timeouted: JPromise was destroyed!]

The dialogs themselves work and hasPermission / status end up correct — only the Promises never settle.

Root cause

Two single-slot fields, one in React Native and one in Android itself. Routing results by request code alone does not fix it — the requests also have to actually run one at a time.

1. PermissionAwareActivity only remembers one PermissionListener.

ReactActivityDelegate.requestPermissions overwrites the field on every call:

public void requestPermissions(String[] permissions, int requestCode, @Nullable PermissionListener listener) {
    mPermissionListener = listener;
    getPlainActivity().requestPermissions(permissions, requestCode);
}

VisionCamera created a fresh PermissionListener per request, so listener A was replaced by B and B by C. Only the last one ever ran; the earlier continuations stayed suspended forever.

The same delegate also drops the listener as soon as one returns true, and buffers the result in a single mPermissionsCallback field while a dialog is up — so an overlapping request can lose a result there too.

2. Activity.requestPermissions(...) refuses concurrent requests.

AOSP Activity.java:

if (mHasCurrentPermissionsRequest) {
    Log.w(TAG, "Can request only one set of permissions at a time");
    // Dispatch the callback with empty arrays which means a cancellation.
    onRequestPermissionsResult(requestCode, new String[0], new int[0], deviceId);
    return;
}

The second request is cancelled with empty grant results before the user ever sees it. The old code mapped that empty array to "denied" — and since shouldShowRequestPermissionRationale(...) is false for a permission that was never asked, PermissionStateStore would then persist it as permanently denied.

Fix

A new PermissionRequestDispatcher (android/.../camera/extensions/PermissionRequestDispatcher.kt) owns all of the plumbing:

• one shared PermissionListener, with the per-request state kept here in a ConcurrentHashMap<Int, CancellableContinuation<IntArray>> keyed by request code, so no request can clobber another one's listener,
• a Mutex, so Android only ever sees one in-flight request and never hits the "only one set of permissions at a time" cancellation,
• the listener only returns true — which makes React Native drop the shared listener again — once nothing is in flight anymore,
• cancelling the coroutine removes its pending entry, and a requestPermissions(...) that throws resumes its caller with that error instead of leaking it.

ReactApplicationContext.requestPermission(...) keeps the exact same signature and still owns the PermissionStateStore bookkeeping, so HybridCameraFactory and HybridLocationManager are untouched — none of this leaks into surface-level code.

One small behavior change worth calling out: an empty (cancelled) grant result no longer records a permanently denied state, since nothing was actually asked. Happy to drop that if you'd rather keep the diff strictly to the hang.

Tests

New apps/simple-camera/__tests__/visioncamera.permissions.harness.ts, plus its row in the __tests__ README layout table:

• resolves camera and microphone requests that are started in parallel
• resolves every request when the same permission is requested multiple times at once

Both are shared tests with no platform guard — the behavior should hold everywhere, and iOS passes them today. On Android before this change they hang and fail on withTimeout(...). The harness grants permissions on install (permissions: true in rn-harness.config.mjs), so both requests are expected to resolve true, same as the existing expect(cameraPermissionStatus).toBe('authorized') in the hooks suite.

No JUnit tests / no new Gradle test dependencies, per the Harness-only convention.

Verification

• bun run lint-kotlin — clean, no reformatting
• biome check — clean on the new test file
• tsc --noEmit — no errors in the new test file
• ./gradlew :react-native-vision-camera:compileDebugKotlin — compiles
• No Android device on hand for a local Harness run, so the on-device pass is up to CI.

[vercel]

@dennytosp is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

[mrousavy]

Thanks for attempting to fix this! A few nit picks before we can merge

[dennytosp]

Thanks, all four fixed:

• Listener always returns false now — true could drop the listener that the next queued request had just registered.
• Skipped the mainHandler idea, agreed it only narrows the window.
• Rolled back setHasRequestedPermission on a cancelled request. Good catch, that was reporting DENIED for a permission the user was never asked.
• singleOrNull() in, throws on null.

ktlint and compileDebugKotlin green locally.

[dennytosp]

@mrousavy thanks for the review — all four are answered. Three were already handled in 1a9671e, pushed a couple of hours after your pass, so the CHANGES_REQUESTED on this PR is against ede0d0e5 rather than the current head. 8bba6475 adds the one thing that was still missing, which was a proper justification rather than a code change.

1. The listener race — you were right, and it is worse than "an assumption"

I went and read React Native's side rather than reasoning about it. ReactActivityDelegate.java (RN 0.85.3, the version this repo resolves):

43:   private @Nullable PermissionListener mPermissionListener;
246:  mPermissionListener = listener;        // unconditional overwrite, no map
254:  if (mPermissionListener != null
255:      && mPermissionListener.onRequestPermissionsResult(...)) {
257:    mPermissionListener = null;          // `true` nulls the single slot

One global, non-volatile slot — no keying by request code anywhere. So true does not mean "drop my listener", it means "drop the listener".

It is also reachable rather than theoretical, because registration and callback are on different threads. Promise.kt:130 is private val defaultScope = CoroutineScope(Dispatchers.Default), and HybridCameraFactory.kt:51-61 calls Promise.async { context.requestPermission(...) } with no scope override — so requestPermission runs off the main thread, while the listener is always invoked on the main thread (ReactActivity.onRequestPermissionsResult, or deferred into mPermissionsCallback and replayed from onResume() at :199-206, which is the real-world path since the permission dialog pauses the host activity). An unsynchronized read-modify-write of a plain field across two threads is a genuine data race, not just an ordering question.

The same reading is what proves the original bug, for the record: pre-PR the code created a new listener per request and returned true only on a request-code match, so request B overwrote A's listener at :246, A's result hit B's listener, missed the code, returned false, and A's suspendCoroutine never resumed.

What is in the PR: one long-lived shared listener, per-request state in pendingRequests: ConcurrentHashMap<Int, CancellableContinuation<IntArray>>, and return@PermissionListener false always. That removes the mPermissionListener = null write entirely, so there is nothing left to race. It is leak-free too — PermissionRequestDispatcher is an object and the lambda captures only that singleton, never the Activity, so the delegate retains one static reference.

What 8bba6475 adds is the argument I should have led with. React Native's own permissions module already does exactly this:

// com/facebook/react/modules/permissions/PermissionsModule.kt
27: public class PermissionsModule(reactContext: ReactApplicationContext?) :
28:     NativePermissionsAndroidSpec(reactContext), PermissionListener {
30:   private val callbacks: SparseArray<Callback> = SparseArray<Callback>()
93:   activity.requestPermissions(arrayOf(permission), requestCode, this)
163:  callbacks.size() == 0

One long-lived listener, per-request state keyed by request code, and true only once its own bucket has emptied — never unconditionally. So this dispatcher is the RN-sanctioned shape, and return false is just the conservative form of callbacks.size() == 0.

2. "Maybe we can find a better solution"

Agreed on rejecting the mainHandler resume — resuming on a specific handler is a timing assumption, which is the flakiness you were worried about.

There is a structural version of it that I think is better, and I would rather you decide than push it: don't resume on main, hand the request to Android on main.

withContext(Dispatchers.Main) {
  suspendCancellableCoroutine { continuation -> ... }
}

The reasoning is that registration is currently the odd one out. The callback is always on main; the delegate's mPermissionListener is read and nulled on main; Android's own mHasCurrentPermissionsRequest is cleared on main in dispatchRequestPermissionsResult (Activity.java:9892) but we write it from a background thread at :5908. Posting the registration to main (plain Dispatchers.Main, not .immediate, so it is always posted) makes registration and callback strictly ordered instead of racing on another core — and then return pendingRequests.isEmpty() becomes provably safe and this can match PermissionsModule.kt:163 exactly. withContext returns to Default afterwards, so the SharedPreferences writes and the shouldShowRequestPermissionRationale binder call stay off the main thread.

The honest caveat: RN's own module calls activity.requestPermissions from the JS thread, i.e. off-main, so I cannot cite "Android requires the main thread here" as a documented rule. This is a determinism improvement and a reasonable convention, not a provable bug fix, and I did not want to land it under the banner of one. return false is already sufficient and strictly safer off-main than isEmpty() is. Say the word and I will add it.

3. shouldShowRequestPermissionRationale — yes it was a bug, and a pre-existing one

The cancellation state is Android's documented contract, not something this PR creates. Activity.java:5857-5862 does not throw when a request is already in flight:

if (mHasCurrentPermissionsRequest) {
  Log.w(TAG, "Can request only one set of permissions at a time");
  onRequestPermissionsResult(requestCode, new String[0], new int[0], deviceId);
  return;
}

and :5952-5955 says to treat empty arrays as a cancellation.

The old code turned that cancellation into a permanent false DENIED. Pre-PR it set setHasRequestedPermission(..., true) up front (:52), collapsed the empty array with grantResults.firstOrNull() ?: PERMISSION_DENIED (:57), and then wrote setPermissionPermanentlyDenied off the rationale call (:63-64). getPermissionStatus then sees DENIED + hasRequested + permanentlyDenied and returns DENIED, persisted in SharedPreferences until the permission is actually granted — for a dialog the user never saw. That is the state you sensed, and it is 100% pre-existing.

So: false, in practice. But the reason it does not matter is that getPermissionStatus only consults the rationale flag after hasRequestedPermission is true, precisely because the flag is meaningless before the user has seen a dialog. The current code detects the cancellation with singleOrNull(), rolls setHasRequestedPermission back to false, and throws — so the status falls back to NOT_DETERMINED instead of being poisoned. And the Mutex removes the main cause of the cancellation in the first place.

Being precise about one thing, since you asked a factual question: I could not find Google documenting the never-requested return value of shouldShowRequestPermissionRationale. It goes Activity.java:5983 → PackageManager:7492 → ApplicationPackageManager:983 → PermissionManager:1145, which is a binder call into PermissionManagerService and not in the SDK sources. I am confident it is false — it is the basis of every first-request-vs-permanently-denied check, and this repo's own getPermissionStatus assumes it — but I would rather say "in practice false" than claim a citation I do not have. The verdict holds either way: whatever it returns, the PR makes that state unreachable.

One adjacent thing I noticed and deliberately left out. ReactApplicationContext+permissions.kt:49 still sets setHasRequestedPermission(..., true) before acquiring the mutex, so with serialization a second caller can now sit behind the first dialog with the marker already set, and a cameraPermissionStatus read during that window reports DENIED for a permission not yet presented. It is strictly better than before — pre-PR the same bad state was permanent rather than transient — and closing it means threading an onRequested: () -> Unit into the dispatcher so the marker is set at the actual requestPermissions call. AGENTS.md:12-14 asks for single atomically revertable changes, so I would rather do that separately than widen this one. Happy either way.

4. singleOrNull() — done

ReactApplicationContext+permissions.kt:51-60:

val grantResult = grantResults.singleOrNull()
if (grantResult == null) {
  PermissionStateStore.setHasRequestedPermission(this, permission, false)
  throw Error("Permission request for \"$permission\" was cancelled by Android! (got ${grantResults.size} results)")
}

On "throw here? or reject?" — in this codebase throw is reject: Promise.kt:146-153 is scope.launch { try { promise.resolve(run()) } catch (e: Throwable) { promise.reject(e) } }, so a thrown Error becomes a JS rejection. throw Error(...) also matches the surrounding style (throw Error("No Activity!") at :44). The message carries grantResults.size, which covers your multiple-results concern as well as the empty one.

On a deterministic test, and on CI

You asked for a Harness test not affected by flaky races, and I want to be straight that I cannot give you one in the current setup. There is no src/test/ or src/androidTest/ anywhere in the repo, and no testImplementation, junit, robolectric, mockito or kotlinx-coroutines-test in any of the eight Gradle files. Testing the dispatcher against a controlled TestDispatcher — the only way to make the interleaving deterministic — means standing that stack up from zero. I would rather say so than promise it.

What the PR does add is apps/simple-camera/__tests__/visioncamera.permissions.harness.ts: Promise.all over a camera + microphone request, and three concurrent camera requests, each wrapped in the existing withTimeout(..., 10_000, ...). That is not flaky-sensitive, because the failure mode before the fix is a promise that never settles — it fails at 10s with a name rather than timing out the job. Permissions are pre-granted by the runner (rn-harness.config.mjs:120 → adb shell pm grant), but the test still exercises the bottleneck: ReactApplicationContext+permissions.kt:43-50 has no checkSelfPermission short-circuit, so an already-granted request still goes through activity.requestPermissions and RN's single-listener slot. The denial path and the real-dialog cancellation are not covered, and Harness 1.4.0-rc.1 exposes no permission API to revoke per-test.

CI, for what it is worth: there is no real signal on this PR and it is not the code. The only check in the rollup is Vercel — FAILURE — "Authorization required to deploy.", the fork-authorization gate. The two Actions runs on head 024e5110 both have 0 jobs and 0 check-runs and their logs 404, so no step ever executed: Lint Kotlin needs maintainer approval for fork PRs (action_required on essentially every fork PR in that workflow's history), Harness Android Emulator is disabled_manually with its last run in April, and Harness AWS Device uses OIDC role-to-assume with id-token: write, which GitHub does not grant to fork pull_request runs. If you approve workflows on this PR there would at least be a ktlint result.

I did run ktlint locally in the meantime, with the repo's own invocation rather than eyeballing it:

$ bun run lint-kotlin     # ktlint --editorconfig=./config/.editorconfig --format
Kotlin Format done!
$ git diff --exit-code    # unchanged

No reformatting, so lint-kotlin.yml's diff assertion would pass.