fix(android): prevent one-shot Callback double-invoke crash (New Architecture)#896
Open
susonthapa wants to merge 1 commit into
Open
Conversation
…itecture) play() and prepare() each register two MediaPlayer listeners (completion + error / prepared + error) that wrap the *same* one-shot React Native Callback, and the early no-player / no-resource paths can also reach it. The listeners guarded themselves with independent per-listener `callbackWasCalled` booleans, which do not coordinate across the two listeners — so the same Callback could still be invoked twice (e.g. an error arriving right after completion, or a teardown race). On the legacy bridge the second invoke threw a catchable RuntimeException (swallowed by the surrounding try/catch). Under the New Architecture (`newArchEnabled=true`) the second invoke of a one-shot Callback is a `LOG(FATAL)` in `JavaTurboModule.cpp` -> `std::abort()` (SIGABRT) — not catchable, so the try/catch no longer protects anything and the process crashes. Replace the per-listener flags with a single shared `AtomicBoolean` latch per callback, routed through a local `fire()` gate (`compareAndSet`). First verdict wins; any later invoke is dropped and logged. This makes the exactly-once contract hold across both listeners and every early-return path, on both the old and new architectures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
On Android with the New Architecture (
newArchEnabled=true),play()andprepare()can crash the whole app with SIGABRT:Both methods register two
MediaPlayerlisteners that wrap the same one-shot React NativeCallback:play()→OnCompletionListener+OnErrorListenerprepare()→OnPreparedListener+OnErrorListener…and the early
null-player / no-resource paths can also invoke that sameCallback.Each listener guarded itself with its own
callbackWasCalledboolean. Those flags are per-listener, so they don't coordinate across the two listeners — when an error arrives right after completion (or during a teardown race), the sameCallbackis invoked a second time.On the legacy bridge that second invoke threw a catchable
RuntimeException, which the surroundingtry/catchswallowed — so it was invisible. Under the New Architecture, a second invoke of a one-shotCallbackis aLOG(FATAL)→std::abort(), which is not catchable. Thetry/catchno longer protects anything and the process crashes.This is the same class of crash documented across several RN libraries after the New-Arch migration (e.g. AppsFlyer plugin #601, react-native-image-picker #2390). Related: #152 ("Illegal callback invocation from native module. This callback type only permits a single invocation from native code.").
Fix
Replace the independent per-listener flags with one shared
AtomicBooleanlatch per callback, routed through a localfire()gate:Every invoke site in
play()/prepare()(early-return paths + both listeners) goes throughfire().compareAndSetmakes the first invoke win atomically and drops any later invoke (logged atLog.w), so the exactly-once contract now holds across both listeners and every early-return path — on both the old and new architectures. The now-uselesstry/catch (RuntimeException)blocks are removed.Behavior is unchanged on the happy path: the first verdict (success or error, with its original arguments) is delivered exactly as before.
Scope
android/src/main/java/com/zmxv/RNSound/Sound.ktonly —play()andprepare(). iOS and the JS layer are untouched. Other single-invoke synchronous callbacks (stop,pause,getCurrentTime, …) are not affected and were left alone.Testing
Verified in a production New-Arch app (RN 0.81.5): with the original code the app reliably aborts (SIGABRT,
CxxCallbackImpl.invoke -> Sound.play) on aplay()→release()teardown race around clip completion; with this change it no longer aborts and the completion callback fires exactly once. The fix has also been driven through an exhaustive event-ordering matrix ({completion},{error},{completion,error},{error,completion}, double/triple completion, early no-resource) asserting exactly-once + first-verdict-wins.