Repository navigation
Fix PushNotificationIOS leaking the previous listener when re-adding the same event type - #59758
Open
seyedmostafahasani wants to merge 3 commits into
Open
seyedmostafahasani wants to merge 3 commits into
seyedmostafahasani wants to merge 3 commits into
Conversation
…ding the same event type addEventListener stored its subscription in `_notifHandlers` keyed by event type without checking for an existing one. Calling it twice for the same type overwrote the first subscription in the map, so removeEventListener could only remove the second one. The first listener stayed registered on the emitter, kept its handler alive, and kept firing. Remove any previous subscription for the type before registering the new one.
Removes a stray `g` accidentally left after `);` in cancelLocalNotifications by the previous commit.
…ent type Covers removal, the double-add case that previously leaked the first subscription, and that only the latest handler is invoked.
This branch has not been deployed
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.
Summary:
PushNotificationIOS.addEventListenerstores its subscription in_notifHandlers, keyed by event type, without checking whether one already exists. Calling it twice for the same type overwrites the first subscription in the map, soremoveEventListenercan only remove the second one. The first listener stays registered on the emitter, keeps its handler (and everything the handler closes over) alive, and keeps firing.This change removes any previous subscription for the type before registering the new one, so at most one listener per type is ever registered and
removeEventListeneralways cleans it up. The behavior change is that a secondaddEventListenerfor the same type now replaces the earlier handler instead of leaving it active and un-removable.Changelog:
[IOS] [FIXED] -
PushNotificationIOS.addEventListenerno longer leaks the previous listener when called again for the same event typeTest Plan:
Added
Libraries/PushNotificationIOS/__tests__/PushNotificationIOS-test.js, covering removal, adding the same type twice, and that only the latest handler is invoked.yarn jest packages/react-native/Libraries/PushNotificationIOS: 3 passedyarn eslint --max-warnings 0andyarn prettier --checkon the changed files: cleanNot run:
yarn flow-checkand the native/iOS builds.