@W-24345703: Avoid screen-lock feature persistence ANR by brandonpage · Pull Request #3058 · forcedotcom/SalesforceMobileSDK-Android

brandonpage · 2026-10-08T22:50:10Z

Summary

persist the screen-lock feature flag on Dispatchers.IO before publishing a newly authenticated account as current
separate Android account persistence from current-user publication so the login completion path awaits the persistence barrier
cover ordering and registration behavior with focused authentication tests

Verification

./gradlew :libs:SalesforceSDK:assembleDebugAndroidTest
git diff --check
attempted ./gradlew :libs:SalesforceSDK:connectedDebugAndroidTest; unable to run because no Android device or emulator is connected

Scope

limited to the screen-lock feature marker; other feature-marker timing is unchanged.

brandonpage · 2026-10-08T22:51:59Z

+	 * @return auth bundle with encrypted values.
+	 */
+	@RestrictTo(RestrictTo.Scope.LIBRARY)
+	public Bundle createAccount(UserAccount userAccount, boolean makeCurrentUser) {


I don't like having to add this, but I did not see any way around it (without a major refactor).

brandonpage · 2026-10-08T23:25:13Z

Fixed the reported test failures in c29d6dd.

Root causes:

LoginViewModelMockTest mocks omitted the newly injected persistScreenLockFeature argument.
Direct screen-lock persistence tests relied on a global singleton rather than the mocked manager.

Verification passed on emulator-5554:
./gradlew :libs:SalesforceSDK:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=com.salesforce.androidsdk.auth.AuthenticationUtilitiesTest,com.salesforce.androidsdk.auth.LoginViewModelMockTest

Result: 67 tests, 0 failures.

wmathurin · 2026-10-08T23:43:05Z

        )
    } else if (internalScreenLockManager?.enabled == true) {
-        SalesforceSDKManager.getInstance().unregisterUsedAppFeature(FEATURE_SCREEN_LOCK, account)
        internalScreenLockManager.cleanUp(account)


This still runs on Main (inside withContext(Dispatchers.Main) { onAuthFlowFinished { ... } }). ScreenLockManager.cleanUp(account) reads userAccountManager.authenticatedUsers, which is getAccountsByType plus buildUserAccount (several getUserData binder calls and decryption) for every account. So in a multi-user app where this user's policy has no screen-lock timeout but screen lock is enabled globally, we still do AccountManager IPC on the main thread, which is the same ANR class this PR targets and contradicts the new doc's rule. Could the cleanUp (or at least the account scan) move off Main, keeping only lock() / UI work on Main?

wmathurin · 2026-10-08T23:43:06Z


-        // Init user logging
+    if (!tokenMigration) {
+        withContext(IO) {


Minor: all callers (doCodeExchange -> withContext(IO), LoginActivity -> CoroutineScope(Default), NativeLoginManager -> withContext(Default)) already run onAuthFlowComplete off Main, so this withContext(IO) is defensive rather than the actual fix. The fix is moving the marker write out of the Dispatchers.Main block below. Fine to keep, but worth clarifying in the PR description.

wmathurin · 2026-10-08T23:43:06Z

+	 * @return auth bundle with encrypted values.
+	 */
+	@RestrictTo(RestrictTo.Scope.LIBRARY)
+	public Bundle createAccount(UserAccount userAccount, boolean makeCurrentUser) {


There's already a private UserAccountManager.persistAccount(...) extension in AuthenticationUtilities.kt that does exactly this (persist without changing the current user). It's used by the token-migration path. Could we reuse it (made internal if needed) instead of adding a new public overload on a public class? @RestrictTo is only a lint hint. If the overload is kept for mockability in tests, that's understandable, but then persistAccount's KDoc ("createAccount cannot be used because it unconditionally calls storeCurrentUserInfo") becomes stale, and we'd have two copies of the same persistence logic.

I completely forgot about that, great idea.

wmathurin · 2026-10-08T23:43:06Z

+        userAccountManager.storeCurrentUserInfo(account.userId, account.orgId)
        userAccountManager.sendUserSwitchIntent(userSwitchType, null)
+    } else {
+        withContext(IO) {


Question: if persistScreenLockFeature throws after createAccount(account, false) (non-migration branch above), we end up with an account that exists in AccountManager (visible in authenticatedUsers) but is not current and gets no switch intent. doCodeExchange routes this to onAuthFlowError via runCatching, but the LoginActivity user-agent path (CoroutineScope(Default).launch) has no handler. That part predates this PR. Is the orphaned non-current account acceptable here, or should we clean it up?

wmathurin · 2026-10-08T23:43:06Z


+/** Persists the screen-lock feature before the completed account becomes observable to the app. */
+@VisibleForTesting
+internal fun persistScreenLockFeature(


Nit: the screenLockTimeout > 0 / screenLockManager.enabled decision is now duplicated between this and handleScreenLockPolicy, so the registered marker and the stored policy could drift apart if one condition changes. Consider computing it once, or at least add a comment in handleScreenLockPolicy pointing here.

wmathurin · 2026-10-08T23:43:06Z

    }

+    @Test
+    fun testOnAuthFlowComplete_persistsScreenLockFeatureBeforeUserSwitchAndSuccess() = runTest {


This covers the non-migration ordering. Could we also add a migration-path test (tokenMigration = true) asserting that persistScreenLockFeature is invoked and that createAccount(any(), any()) / storeCurrentUserInfo are not? Once the cleanUp path is moved off Main, a test for that would be good too.

wmathurin · 2026-10-08T23:43:06Z

@@ -367,7 +388,7 @@ class AuthenticationUtilitiesTest {
        verify(exactly = 0) { onAuthFlowSuccess.invoke(any()) }
        verify(exactly = 0) { addAccount.invoke(any()) }
        verify(exactly = 0) { mockUserAccountManager.createAccount(any()) }


This line now duplicates the one above it. It was meant to replace the switchToUser check. Probably createAccount(any(), any()) and/or storeCurrentUserInfo(any(), any()).

wmathurin · 2026-10-08T23:43:06Z

+
+Before the SDK publishes the newly authenticated account as the current user, the screen-lock feature write must complete successfully. In particular, the SDK must not hand the app a REST client, send the user-switch notification, invoke the authentication-success callback, or launch the completed login flow while that write is pending.
+
+Feature writes must be awaited and must not be fire-and-forget. Work that calls `AccountManager` must not run on the main thread when it can block on binder IPC.


This rule is currently contradicted by handleScreenLockPolicy -> ScreenLockManager.cleanUp (see comment there), which still reads AccountManager on Main. Either fix that path or scope the wording. Also, please add this doc to the index table in docs/auth/README.md.

JohnsonEricAtSalesforce

I found no confirmed runtime regression in the account and screen-lock marker persistence barrier. I approve this change with the following optional follow-ups.

Requested Changes

None.

Open Questions

Would you add focused tests for the new shouldLock == true completion branch and for a marker-write exception? The current tests cover successful ordering but do not exercise the lock launch or verify that a failed marker write prevents current-user publication. The existing proceed test's name/comment also describes the old policy ordering.
Would you remove the now unused mockSdkManager bindings at lines 971 and 990 of AuthenticationUtilitiesTest.kt, while keeping the setup calls? The null-identity test name also still says unregistersAndCleansUp, although that test now checks cleanup only.

This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.

JohnsonEricAtSalesforce · 2026-10-14T22:46:22Z

-
-            // Screen lock required by mobile policy
-            handleScreenLockPolicy(userIdentity, account)
+            if (shouldLock) {


Optional test suggestion: the new shouldLock branch is not exercised by the completion tests because their shared mock returns false. A true-result case with a deferred completion hook could verify that lock() runs when proceed is invoked, after startMainActivity(), on Main. The existing proceed test's name/comment could then reflect the new policy ordering.

This comment was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.

brandonpage · 2026-10-15T00:33:29Z

Addressed the requested test follow-ups in 05d18df.

Added coverage that a screen-lock feature-marker write failure does not publish the current user, send a user-switch intent, or invoke auth success.
Added coverage that the shouldLock completion path starts the main activity before launching screen lock.
Removed unused screen-lock test bindings and renamed the stale cleanup-only test.

Verified on Pixel 8 Pro - 17:
./gradlew :libs:SalesforceSDK:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=com.salesforce.androidsdk.auth.AuthenticationUtilitiesTest,com.salesforce.androidsdk.auth.LoginViewModelMockTest,com.salesforce.androidsdk.security.ScreenLockManagerTest

Result: 77 tests, 0 failures.

@W-24345703: Avoid screen-lock feature persistence ANR

7086b8e

brandonpage changed the base branch from master to dev October 8, 2026 22:50

brandonpage commented Oct 8, 2026

View reviewed changes

@W-24345703: Fix screen-lock persistence tests

c29d6dd

wmathurin reviewed Oct 8, 2026

View reviewed changes

@W-24345703: Address screen-lock persistence review

090483b

brandonpage requested review from JohnsonEricAtSalesforce and wmathurin October 10, 2026 00:34

JohnsonEricAtSalesforce approved these changes Oct 14, 2026

View reviewed changes

@W-24345703: Add screen-lock completion tests

05d18df

brandonpage merged commit 1c697d7 into forcedotcom:dev Oct 15, 2026
7 of 9 checks passed


		Before the SDK publishes the newly authenticated account as the current user, the screen-lock feature write must complete successfully. In particular, the SDK must not hand the app a REST client, send the user-switch notification, invoke the authentication-success callback, or launch the completed login flow while that write is pending.

		Feature writes must be awaited and must not be fire-and-forget. Work that calls `AccountManager` must not run on the main thread when it can block on binder IPC.

Conversation

brandonpage commented Oct 8, 2026

Summary

Verification

Scope

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpage commented Oct 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohnsonEricAtSalesforce left a comment

Choose a reason for hiding this comment

Requested Changes

Open Questions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpage commented Oct 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants