[ABT] Fix race crash - #16148
Conversation
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 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 counter productive. 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 |
|
Closing in favor of #16147 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a crash caused by a race condition on the mutable payloads array in FIRExperimentController. It achieves this by creating an immutable copy of the payloads array before passing it into an asynchronous dispatch block, ensuring thread safety. The CHANGELOG has been updated to reflect this fix. I have no feedback to provide.
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
2 participants
Fix #16145
Crash Analysis
The crash occurs due to a thread safety issue (race condition) when creating a copy of a mutable array on a background thread.
1. The Underlying Cause
The crash is a classic concurrency issue caused by
FIRExperimentController.m
asynchronously accessing a mutable array that is passed by the caller.
A. Asynchronous Execution in Firebase ABTesting
In FIRExperimentController.m, the updateExperimentsWithServiceOrigin:events:policy:lastStartTime:payloads:completionHandler: method dispatches the work to a background queue:
B. Mutation by the Caller (RCNConfigExperiment.m)
Even though the user's request states they are not using any A/B testing functionality, Firebase Remote Config internally uses Firebase ABTesting to process experiments (frc service origin).
In RCNConfigExperiment.m , the _experimentPayloads property is a mutable array (NSMutableArray<NSData >). During synchronization, Remote Config triggers an update:
C. Race Condition
updateExperimentsWithHandler: passes _experimentPayloads (NSMutableArray) to updateExperimentsWithServiceOrigin:.
FIRExperimentController captures a reference to this mutable array inside a dispatch_async block.
While the background thread starts executing, the main thread concurrently receives a new server payload and mutates the array (e.g., it clears all objects in updateExperimentsWithResponse: by calling [_experimentPayloads removeAllObjects]).
As the background thread reaches line 51 of FIRExperimentController.m (NSArray *payloadsCopy = [payloads copy]), it executes initWithArray:range:copyItems:. It reads the array's count and tries to obtain the objects. Because the array was cleared by the main thread, it tries to read a range that is out of the new bounds of the empty array, resulting in the crash: *** -[__NSArrayM getObjects:range:]: range {0, 1} extends beyond bounds for empty array
2. The Fix
To fix this crash, FIRExperimentController must create an immutable copy of the payloads array synchronously on the calling thread before passing it to the background queue.