fix(ios): run endSigningSession DSMManager calls on the main thread - #8
Merged
Conversation
Expo AsyncFunction handlers are dispatched on AsyncFunctionQueue (a non-main queue). DSMManager APIs (clearAllWebCookies, logout) must run on the main thread, so endSigningSession could touch them off-main and intermittently hang or crash. Hop to main before calling clearWebCookiesAsync. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hop landed in endSigningSession, below the stateQueue.sync that cancels an in-flight signing promise, so the re-entrant pass ran that block a second time. Harmless when the slot is already empty, but a presentCaptiveSigning arriving in between would be cancelled by the re-entry. clearWebCookiesAsync is the only method reaching DSMManager.clearAllWebCookies() and WKWebsiteDataStore directly, so guarding it covers performLogin, endSigningSession and reset from one place, and no caller has to know the threading contract. Its completion is already dispatched on main, so the DSMManager.logout() every caller runs from it stays safe. reset() loses its own hop for the same reason: it sat below the same cancellation block and had the same re-entrant double-execution. Drop the pendingResolved flag in endSigningSession while here. It was assigned, never read, and carried a line existing only to silence the resulting warning.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Takes the main-thread fix from #2 so it can ship in 2.0.0 without waiting on the modal-dismissal work in that PR, which is a separate design question. First commit is @virajpsimformsolutions' original, cherry-picked unchanged. The two after it are the review follow-up.
Merge with rebase, not squash. Squashing collapses the contributor's commit into mine and drops the attribution.
The bug
Expo dispatches a synchronous
AsyncFunctionbody on a serial background queue, so JS-originated calls arrive off-main.DSMManagerandWKWebsiteDataStoreare main-thread only.endSigningSessionreachedclearAllWebCookies()andlogout()directly from that background queue on every call, including the oneuseDocuSignSigning'sreset()makes between signing flows.performLoginalready hopped to main, which is why this only bit the teardown paths.The follow-up
The hop landed inside
endSigningSession, below thestateQueue.syncthat cancels an in-flight signing promise. Re-entering the function from main therefore ran that cancellation block a second time. Empty slot makes it harmless, but apresentCaptiveSigningarriving between the two passes gets cancelled by the redundant one.Moved the guard into
clearWebCookiesAsync. It is the only method reachingDSMManager.clearAllWebCookies()andWKWebsiteDataStoredirectly, so one guard coversperformLogin,endSigningSessionandreset, and no caller has to know the threading contract. Its completion is already wrapped inDispatchQueue.main.async, so theDSMManager.logout()each caller runs from it stays on main.reset()loses its own hop for the same reason. It sat below the same cancellation block and had the same re-entrant double-execution, so this closes that race as well.Also drops a
pendingResolvedvar inendSigningSessionthat was assigned, never read, and carried a line existing only to silence the resulting warning.Verification
swiftc -parseclean,npm run build,npm run lint, 18/18 Jest.A threading review traced every
stateQueuecall site for a cycle with main and found none, confirmedlogout()andremoveObserverstill land on main through the completion dispatch, and confirmedcompletion()is invoked exactly once on every path includingreset()'sneedsTeardownearly return.Stating the limit plainly, same as #7: CI compiles no Swift and runs no iOS tests, so nothing here catches a type error in this file. Verified by parsing and by reading. A device run before tagging 2.0.0 is the honest bar.
Not included from #2
The force-dismiss commit stays there. It dismisses from the key window's root view controller while both present paths use
topmostViewController(), so it tears down the host app's own modal whenever signing was started from a sheet. Fixing that needs a reference to the controller actually presented, which touches the present path too. Tracked on #2.