Skip to content

Replace @synchronized(self) in GIDGoogleUser with a private lock - #652

Merged
w-goog merged 8 commits into
mainfrom
fix/googleuser-auth-state-lock
Oct 20, 2026
Merged

w-goog merged 8 commits into
mainfrom
fix/googleuser-auth-state-lock

Conversation

@w-goog

@w-goog w-goog commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor
  • Change @synchronized(self) to synchronize on a private lock, to prevent other callers from battling for the user object's lock. The private lock remains recursive, because AppAuth calls didChangeState / KVO observers may need to re-enter.
  • Update the refresh path to use said lock.
  • Update emmSupport and encodeWithCoder to use said lock.
  • Known follow-up: Other readers grab authState without the lock. Tracked in GIDGoogleUser.authState is sometimes read without a lock #648.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces @synchronized(self) with a recursive lock (_authStateLock) in GIDGoogleUser to serialize auth state updates and reads, avoiding potential deadlocks during KVO notifications. Feedback on the changes identifies a potential crash in refreshTokensIfNeededWithCompletion: if lastTokenAdditionalParameters is nil, as passing nil to addEntriesFromDictionary: raises an exception. A guard or fallback dictionary is recommended to prevent this crash.

Comment thread GoogleSignIn/Sources/GIDGoogleUser.m
@w-goog
w-goog force-pushed the fix/googleuser-auth-state-lock branch from 61e2916 to a6d3717 Compare October 15, 2026 22:39
@w-goog
w-goog added this pull request to stack #654 October 15, 2026 22:48
Comment thread GoogleSignIn/Sources/GIDGoogleUser.m Outdated
Comment thread GoogleSignIn/Sources/GIDGoogleUser.m Outdated
@w-goog
w-goog force-pushed the fix/googleuser-auth-state-lock branch from a6d3717 to 988f058 Compare October 15, 2026 23:11
@w-goog
w-goog requested a review from brnnmrls October 16, 2026 19:29
Comment thread GoogleSignIn/Sources/GIDGoogleUser.m Outdated
Comment thread GoogleSignIn/Sources/GIDGoogleUser.m Outdated
Comment thread GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m
@w-goog
w-goog force-pushed the fix/googleuser-auth-state-lock branch from 988f058 to 0e17431 Compare October 19, 2026 20:04
@w-goog w-goog changed the title Replace @synchronized in GIDGoogleUser with an auth state lock Replace @synchronized(self) in GIDGoogleUser with a private lock Oct 19, 2026
@w-goog
w-goog requested a review from brnnmrls October 19, 2026 20:19
Comment thread GoogleSignIn/Sources/GIDGoogleUser.m Outdated
Comment thread GoogleSignIn/Tests/Unit/GIDGoogleUserTest.m Outdated
Base automatically changed from fix/googleuser-snapshot-scopes-config-profile to main October 20, 2026 16:06
@w-goog
w-goog force-pushed the fix/googleuser-auth-state-lock branch from 0cfb17d to 75884b5 Compare October 20, 2026 16:06
@w-goog
w-goog merged commit a49a465 into main Oct 20, 2026
22 of 29 checks passed
@w-goog
w-goog deleted the fix/googleuser-auth-state-lock branch October 20, 2026 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants