Repository navigation
fix(auth): release removed state and ID token listeners - #16962
Conversation
Store the observer handle returned by `NotificationCenter` in `listenerHandles`, instead of the listener closure. The remove methods and `deinit` look for the handle there, so they never found the closure. Previously, removed listeners, and the objects they captured, stayed in memory for the lifetime of `Auth`, and `deinit` left the observers registered with `NotificationCenter`. The Objective-C SDK stored the handle; this regressed in the Swift port in 11.0.0. Type `listenerHandles` as `[NSObjectProtocol]`, so the compiler rejects storing anything other than an observer handle. Fixes #16938
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves a memory leak in FirebaseAuth where removed auth state and ID token listeners, along with their captured objects, were retained in memory. The fix transitions listenerHandles from NSMutableArray to a Swift array of [NSObjectProtocol], simplifying the addition, removal, and deinitialization cleanup. Additionally, new unit tests are introduced to verify that listeners are correctly released. The review feedback suggests extending the platform check in deinit to include tvOS to prevent observer leaks on that platform as well.
`getToken(forcingRefresh:completion:)` adds the `didBecomeActive` and `didEnterBackground` observers on iOS and tvOS, but `deinit` only removed them on iOS. Remove them on tvOS as well, using the same platform condition.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves a memory leak in FirebaseAuth by migrating listenerHandles from NSMutableArray to a native Swift array of [NSObjectProtocol]. This allows proper cleanup of auth state and ID token listeners, ensuring they and their captured objects are released when removed or when the Auth instance is deallocated. Additionally, new unit tests have been added to verify this behavior. The review feedback suggests improving safety and consistency by using defer { objc_sync_exit(Auth.self) } immediately after entering synchronized blocks in addIDTokenDidChangeListener and removeIDTokenDidChangeListener.
Release the `Auth.self` lock with `defer` in `addIDTokenDidChangeListener(_:)` and `removeIDTokenDidChangeListener(_:)`, like `removeStateDidChangeListener(_:)`. In `addIDTokenDidChangeListener(_:)`, put the `defer` in a `do` block, so the lock is released right after the handle is added, not at the end of the method.
Summary
Store the observer handle returned by
NotificationCenterinlistenerHandles, instead of the listener closure. Removing an auth state orID token listener now releases it, and
deinitremoves the observers that arestill registered.
Fixes #16938.
Why
addIDTokenDidChangeListener(_:)added the listener closure tolistenerHandles, butremoveIDTokenDidChangeListener(_:),removeStateDidChangeListener(_:), anddeinitexpect the handles returned byNotificationCenter.addObserver(forName:object:queue:using:)there. A handlenever matched a closure, so:
listenerHandles. The listener, andeverything it captured, stayed in memory for the lifetime of
Auth, whichfor
Auth.auth()is the life of the app. Apps that add and remove listenersrepeatedly, for example one per screen or per subscription, accumulated them
without bound.
deinitpassed the closures toremoveObserver(_:), which did nothing, sothe observers of a deallocated
Authstayed registered withNotificationCenter, along with their listeners.addStateDidChangeListener(_:)is built onaddIDTokenDidChangeListener(_:),and the
authStateChangesandidTokenChangessequences and the Combinepublishers use these methods, so all of them were affected. The Objective-C SDK
stored the handle. This regressed in the Swift port (#11237), and has been
present since 11.0.0.
Changes
addIDTokenDidChangeListener(_:)adds the observer handle tolistenerHandles, like the Objective-C SDK did.listenerHandlesis now[NSObjectProtocol]instead ofNSMutableArray, sothe compiler rejects adding anything other than an observer handle, such as
the closure. The remove methods remove the handle by identity, and
deinitremoves each remaining observer from
NotificationCenter.deinitalso removes thedidBecomeActiveNotificationanddidEnterBackgroundNotificationobservers on tvOS, wheregetToken(forcingRefresh:completion:)adds them too. Before, it only removedthem on iOS (from review).
Notes for reviewers
NSMutableArray.remove(_:)matched withisEqual:. Thehandles are
__NSObserverinstances, which don't overrideisEqual:, somatching by identity (
===) finds the same handles.Auth.selflock, and now release it withdefer, likeremoveStateDidChangeListener(_:)(from review). InaddIDTokenDidChangeListener(_:), thedeferis in adoblock, so the lockisn't held while the initial call is dispatched.
deinitdoesn't take thelock, as before, since it only runs when nothing else holds the instance.
still called once, and is released after that. The Objective-C SDK behaved
the same way. Not changed here.
Testing
AuthTests, which check through a weak reference that an objectcaptured by a listener is deallocated:
testRemoveStateDidChangeListenerReleasesListenerandtestRemoveIDTokenDidChangeListenerReleasesListeneradd a listener, removeit, and let the initial call run.
testDeallocatingAuthRemovesListenersadds both kinds of listener to a newAuth, and lets it deallocate without removing them. It also checks thatthe
Authinstance is deallocated.main, all three fail. IntestDeallocatingAuthRemovesListeners, theAuthinstance is deallocated,but the captured object isn't, because
NotificationCenterstill holds theobserver.
xcodebuild -scheme AuthUnit -destination 'platform=iOS Simulator,name=iPhone 17 Pro,OS=26.5' test: 403 tests, 0 failures.xcodebuild -scheme AuthUnit -destination 'platform=tvOS Simulator,name=Apple TV 4K (3rd generation),OS=26.5' test: 296 tests, 0 failures.-test-iterations 200).Related
Noted under "Not included" in #16856.