Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrent modification crash in the iOS gesture handler implementation. The crash occurred when reattachHandlersIfNeeded iterated over the handlers registry while the collection was being modified, causing a "NSDictionary was mutated while being enumerated" error. This method was introduced in PR #3964 to handle view recycling on the new architecture when parent views have display: none.
Changes:
- Create a defensive copy of the handlers collection before iterating in
reattachHandlersIfNeeded - Add a comment explaining the defensive copy to prevent concurrent modification
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create copy to avoid mutating collection while enumerating. | ||
| NSArray<NSNumber *> *handlerTags = [_registry.handlers.allKeys copy]; | ||
|
|
||
| // Re-bind handlers to their current native views. On Fabric, when a parent view has | ||
| // display:none and siblings change, the native UIView backing a component may be recycled | ||
| // and replaced. maybeBindHandler is a no-op if the view is nil or unchanged. This is only | ||
| // needed for handlers using the old api. | ||
| for (RNGestureHandler *handler in _registry.handlers.objectEnumerator) { | ||
| for (NSNumber *handlerTag in handlerTags) { | ||
| RNGestureHandler *handler = _registry.handlers[handlerTag]; |
There was a problem hiding this comment.
Copying handlers.allKeys avoids mutating during the subsequent for loop, but allKeys itself still has to enumerate the underlying mutable _handlers dictionary and can throw the same "mutated while being enumerated" exception if the registry is modified concurrently (e.g., dropAllHandlers/dropHandlerWithTag). To make this robust, access to the registry’s _handlers should be serialized (same queue) or protected with a lock/@synchronized when taking the snapshot and when mutating it elsewhere.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NSDictionary *handlers = _registry.handlers; | ||
|
|
||
| @synchronized(handlers) { | ||
| for (RNGestureHandler *handler in handlers.objectEnumerator) { | ||
| if (handler.viewTag == nil || [handler usesNativeOrVirtualDetector]) { | ||
| continue; | ||
| } | ||
|
|
||
| [self maybeBindHandler:handler.tag | ||
| toViewWithTag:handler.viewTag | ||
| withActionType:handler.actionType | ||
| withHostDetector:nil]; | ||
| [self maybeBindHandler:handler.tag | ||
| toViewWithTag:handler.viewTag | ||
| withActionType:handler.actionType | ||
| withHostDetector:nil]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Locking on handlers here won’t reliably prevent the "NSDictionary was mutated while being enumerated" crash unless all code paths that mutate the same underlying dictionary also synchronize on the exact same lock object. Currently, the registry still removes/clears entries without any lock, so concurrent mutation can still happen during this enumeration. A more robust fix is to iterate over a snapshot (e.g. copy the values under a registry-owned lock) or move the synchronization/snapshotting into RNGestureHandlerRegistry so callers don’t need to lock its internal storage.
There was a problem hiding this comment.
As said below, creating handler is only part which is called form other thread.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react-native-gesture-handler/apple/RNGestureHandlerRegistry.m
Outdated
Show resolved
Hide resolved
packages/react-native-gesture-handler/apple/RNGestureHandlerRegistry.m
Outdated
Show resolved
Hide resolved
…`RNGestureHandlerRegistry` to reduce lock contention.
Description
In
reattachHandlersIfNeededmethod we are iterating over handlers contained in registry. The following crash was reported:This PR adds
@synchronizedonregistry._handlersso attempting to modify this value should be safe.Note
For now we have no consistent reproduction. I'm also not sure where this concurrent modification takes place. It seems to be indirect, as previous keyframe in provided stack trace was
flushOperations.Test plan
Tested on the example code from #3964