fix(LayoutAnimations): deadlock in ReanimatedCommitHook surface initialization#9903
Draft
bartlomiejbloniarz wants to merge 1 commit into
Draft
fix(LayoutAnimations): deadlock in ReanimatedCommitHook surface initialization#9903bartlomiejbloniarz wants to merge 1 commit into
bartlomiejbloniarz wants to merge 1 commit into
Conversation
…alization maybeInitializeLayoutAnimations enumerated the ShadowTreeRegistry from inside a commit hook. Commits usually execute inside ShadowTreeRegistry::visit, which already holds the registry's shared_mutex in shared mode, so the enumerate was a recursive shared acquisition. libc++ writers block new readers while draining existing ones, so a concurrent startSurface/stopSurface (a registry writer) on another thread turns that recursion into a permanent deadlock - reported as production ANRs in #8579. The registry is now read exactly once, at hook construction (module init, never a commit stack), to pick up surfaces that existed before Reanimated loaded. After that, the first commit of each new surface registers the mounting override delegate directly through the committing ShadowTree - no registry access on commit stacks at all. currentMaxSurfaceId_ becomes a seenSurfaces_ set, and a mount hook erases stopped surfaces from it so surface id reuse cannot leave a new surface unregistered. Fixes #8579
c7685da to
5d9d63f
Compare
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
Fixes #8579.
maybeInitializeLayoutAnimationsenumerated theShadowTreeRegistryfrom inside a commit hook. Commits usually execute insideShadowTreeRegistry::visit, which already holds the registry'sshared_mutexin shared mode, so the enumerate was a recursive shared acquisition. libc++ writers block new readers while draining existing ones, so a concurrentstartSurface/stopSurface(a registry writer) on another thread turns that recursion into a permanent deadlock — the three-thread ANR in the issue (JS inenumerate, pool thread instartSurface, main inconstraintLayout).The registry is now read exactly once, at hook construction (module init — never a commit stack), to pick up surfaces that existed before Reanimated loaded (lazy init, brownfield). After that, the first commit of each new surface registers the mounting override delegate directly through the committing
ShadowTree— no registry access on commit stacks at all.currentMaxSurfaceId_becomes aseenSurfaces_set, and a mount hook erases stopped surfaces from it. That last part is not hypothetical: a JS reload reuses the same surface id, so without the erase the recreated surface would never get the override delegate.Test plan
fabric-example, iOS sim + Android emulator: