[RC] Fix data race in RCNConfigExperiment (#16303)#16326
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 the 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 counterproductive. 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 |
paulb777
changed the title
![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 addresses a data race condition in RCNConfigExperiment by introducing @synchronized blocks to synchronize access to internal properties (experimentPayloads, experimentMetadata, and activeExperimentPayloads) and their corresponding database update methods. It also adds a concurrent access unit test. The review feedback highlights several critical thread-safety issues: first, a race condition exists in updateExperimentsWithHandler: because updateActiveExperimentsInDB is called outside the synchronized block, meaning it could persist modified payloads instead of the ones that were actually activated. Second, the custom getters for mutable properties return the internal mutable collections directly, which is not thread-safe for external callers; they should return mutable copies instead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by adding @synchronized blocks to synchronize access to experiment payloads, metadata, and active payloads, and introduces a concurrent access unit test. The review feedback highlights that OCMock is not thread-safe and should not be used concurrently in the new test. Additionally, the custom setters should defensively handle nil inputs to prevent silent failures, and the custom getters should return immutable copies to avoid misleading callers and reduce unnecessary memory allocations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by changing its experiment payload and metadata properties to immutable types and adding thread-safe getters, setters, and synchronization blocks around direct instance variable accesses. It also introduces a concurrent access unit test. The review feedback highlights two important improvements: first, _experimentController is uninitialized (nil) in the new concurrent test, which should be resolved by instantiating it locally; second, the raw _experimentPayloads instance variable is passed to the experiment controller in latestStartTimeWithExistingLastStartTime:, which should be replaced with the thread-safe self.experimentPayloads property to prevent potential thread-safety issues.
…StartTimeWithExistingLastStartTime
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by converting experiment payloads and metadata properties to immutable types and wrapping their accessors and modification methods in @synchronized blocks. A concurrent access unit test was also introduced to verify thread safety. The review feedback recommends avoiding DISPATCH_TIME_FOREVER in the new unit test to prevent potential indefinite hangs in CI/CD pipelines, and optimizing latestStartTimeWithExistingLastStartTime: by directly accessing the backing instance variable _experimentPayloads to avoid a redundant reentrant lock and array copy.
ncooke3
left a comment
ncooke3
left a comment
There was a problem hiding this comment.
Is the @synchronized(self) prone to deadlock? See bot's review below.
bot review
### Critical Concerns / Blocking Issues
* Holding instance-level locks (`@synchronized(self)`) during database interactions and calls to external controllers introduces severe risks of lock contention, priority inversion, and deadlocks. State locks should only cover the mutation/access of the in-memory variables.
### Concurrency & Memory
Status: Block
* `FirebaseRemoteConfig/Sources/RCNConfigExperiment.m` / Lines 164-185: Holding `@synchronized(self)` across an entire loop that performs JSON serialization and makes `_DBManager` insertion calls is too broad. This blocks any thread attempting to read experiment state while these expensive operations occur. Prepare the `JSONPayload` objects and update the internal array inside a short lock, then execute the DB manager calls outside of the lock.
* `FirebaseRemoteConfig/Sources/RCNConfigExperiment.m` / Lines 215-236: The `updateExperimentStartTime` method calls `[_DBManager insertExperimentTableWithKey:...]` while holding the `self` lock. If the DB manager shares a queue that calls back into this class, it will deadlock. Copy the `_experimentMetadata`, perform the JSON serialization, and invoke the DB manager outside the critical section.
* `FirebaseRemoteConfig/Sources/RCNConfigExperiment.m` / Lines 241-249: `updateActiveExperimentsInDBWithPayloads:` delegates to `_DBManager` inside the lock. Extract the DB deletion and insertion logic outside of `@synchronized(self)`.
* `FirebaseRemoteConfig/Sources/RCNConfigExperiment.m` / Lines 255-259: `latestStartTimeWithExistingLastStartTime:` invokes an external object (`self.experimentController`) while holding the lock. Calling external components from within a lock is a known deadlock risk. Copy `_experimentPayloads` locally within the lock, then pass the copy to the controller outside the lock.
* `FirebaseRemoteConfig/Sources/RCNConfigExperiment.m` / Lines 219-220: `updateExperimentStartTime` calls `latestStartTimeWithExistingLastStartTime:`. Both methods synchronize on `self`. While Objective-C `@synchronized` blocks are re-entrant and will not self-deadlock, this indicates a flawed locking hierarchy and adds unnecessary overhead. Refactor to use a private, unlocked helper method for internal calls where the lock is already acquired.
…during DB/external controller calls
| @property(nonatomic, copy) NSArray<NSData *> *experimentPayloads; ///< Experiment payloads. | ||
| @property(nonatomic, copy) | ||
| NSDictionary<NSString *, id> *experimentMetadata; ///< Experiment metadata | ||
| @property(nonatomic, copy) |
There was a problem hiding this comment.
Are these properties ever actually accessed as properties? The methods below access the instance variables directly, and the synchronized properties aren't referenced in areas where the critical section can ever only be the property access itself. Since these are private properties I think it would be better to get rid of them and just use the instance variable consistently throughout or make the property get/set methods nonlocking and use those instead of directly accessing instance variables.
There was a problem hiding this comment.
They're used as extension properties for unit testing. See irebaseRemoteConfig/Tests/Unit/RCNConfigExperimentTest.m below.
Good catch! Every |
…n updateExperimentStartTime
There was a problem hiding this comment.
review feedback (drop-down)
### Critical Concerns / Blocking Issues
* Database delete and insert operations have been moved outside of critical sections, creating a data race where concurrent operations can interleave and corrupt the persistent database state.
* Disjoint locks across `updateExperimentsWithHandler:` and its helper methods lead to time-of-check to time-of-use (TOCTOU) inconsistencies, resulting in mismatched state being passed to the external experiment controller.
### Concurrency & Thread Safety
Status: Block
* [RCNConfigExperiment.m/Lines 199-204] `updateExperimentsWithResponse:` executes `deleteExperimentTableForKey:` and subsequent `insertExperimentTableWithKey:` calls sequentially without synchronization. If called concurrently, one thread's insertions could be overwritten or duplicated by another thread. These operations must be atomic (e.g., serialized via a dedicated database dispatch queue).
* [RCNConfigExperiment.m/Lines 286-291] The database delete and insert sequence in `updateActiveExperimentsInDBWithPayloads:` is completely unprotected. This creates an identical interleaving vulnerability. Ensure database atomicity.
* [RCNConfigExperiment.m/Lines 211-222] `updateExperimentsWithHandler:` retrieves `lastStartTime`, calls `updateExperimentStartTime`, and captures `payloadsCopy` in separate, disjoint locks. If another thread mutates `_experimentPayloads` or `_experimentMetadata` between these locks, the state provided to the `experimentController` will be fundamentally inconsistent. Capture a single consistent snapshot of all required state at the beginning of the method.
### Test Coverage & Platform Support
Status: Comment
* [RCNConfigExperimentTest.m/Lines 39-41] The property redeclarations in the test class extension omit the lightweight generics (e.g., `<NSData *>`) present in the main source file's implementation. Add the generics to match the source file to maintain strict type consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by converting internal collections to immutable types, implementing thread-safe getters and setters using @synchronized, and offloading database operations to a new serial dispatch queue (experimentDBQueue). Additionally, a concurrent access unit test has been added to verify these thread-safety improvements. Feedback was provided regarding a potential issue where a nil experimentController could prevent the completion handler from being executed, which could cause callers to hang indefinitely.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by converting internal properties to immutable types with thread-safe accessors, introducing a serial dispatch queue for database operations, and synchronizing access to state variables. Feedback suggests copying the payloads parameter in updateActiveExperimentsInDBWithPayloads: to prevent potential crashes from nil arguments or concurrent mutations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by converting internal state properties to immutable types, implementing thread-safe getters and setters using @synchronized, and introducing a serial dispatch queue (experimentDBQueue) for database operations. Additionally, a concurrent access unit test has been added to verify thread safety. The review feedback suggests moving a nil check for self.experimentController to the beginning of updateExperimentsWithHandler: to prevent logical side effects, correcting a mismatched method signature for updateExperimentStartTime in the test file, and improving error logging when parsing activated experiment payloads.
…nd test signature
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by converting properties to immutable types, adding synchronized accessors, and introducing a serial dispatch queue for database operations. The review feedback points out a potential race condition where database loads could overwrite newer in-memory state, and suggests validating JSON payloads before serialization to prevent crashes. Additionally, for the concurrent unit test, it is recommended to expose and flush the serial database queue to prevent background thread leaks and test flakiness.
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by introducing thread-safe property accessors, synchronized blocks, and a serial dispatch queue for database operations. The review feedback identifies two critical concurrency issues: first, a potential loss of metadata and active payloads if a concurrent fetch completes before the database load, which can be resolved by tracking updates to individual components with separate flags; second, a race condition in updateExperimentsWithHandler: where an inconsistent payload snapshot could cause new experiments to be silently skipped, which can be fixed by passing a consistent payload snapshot to updateExperimentStartTime.
| @implementation RCNConfigExperiment { | ||
| NSMutableArray<NSData *> *_experimentPayloads; | ||
| NSMutableDictionary<NSString *, id> *_experimentMetadata; | ||
| NSMutableArray<NSData *> *_activeExperimentPayloads; | ||
| BOOL _databaseLoadCompleted; | ||
| } |
There was a problem hiding this comment.
Issue: Potential Loss of Metadata and Active Payloads on Concurrent Fetch
If updateExperimentsWithResponse: is called before the database load completes, it sets _databaseLoadCompleted = YES. When the database load completion block eventually runs, the entire block inside @synchronized(strongSelf) is skipped because !strongSelf->_databaseLoadCompleted evaluates to false.
This results in:
_experimentMetadataremaining empty (losing the storedlast_experiment_start_time)._activeExperimentPayloadsremaining empty (losing the active experiments).
Additionally, if updateExperimentsWithHandler: runs before the database load completes, it will update the metadata and active payloads. When the database load subsequently completes, it will overwrite these newly updated values with the stale ones from the database because _databaseLoadCompleted is still NO.
Recommendation
Instead of a single _databaseLoadCompleted flag, track whether each component (payloads, metadata, activePayloads) has been updated by an external/user action before the database load completes. This ensures that we only skip loading a specific component from the database if it has already been updated with newer data, while still loading the other components safely.
Let's add three new boolean flags to track this.
| @implementation RCNConfigExperiment { | |
| NSMutableArray<NSData *> *_experimentPayloads; | |
| NSMutableDictionary<NSString *, id> *_experimentMetadata; | |
| NSMutableArray<NSData *> *_activeExperimentPayloads; | |
| BOOL _databaseLoadCompleted; | |
| } | |
| @implementation RCNConfigExperiment { | |
| NSMutableArray<NSData *> *_experimentPayloads; | |
| NSMutableDictionary<NSString *, id> *_experimentMetadata; | |
| NSMutableArray<NSData *> *_activeExperimentPayloads; | |
| BOOL _databaseLoadCompleted; | |
| BOOL _payloadsUpdated; | |
| BOOL _metadataUpdated; | |
| BOOL _activePayloadsUpdated; | |
| } |
| @synchronized(strongSelf) { | ||
| if (!strongSelf->_databaseLoadCompleted) { | ||
| if (parsedPayloads) { | ||
| [strongSelf->_experimentPayloads removeAllObjects]; | ||
| [strongSelf->_experimentPayloads addObjectsFromArray:parsedPayloads]; | ||
| } | ||
| if (parsedMetadata) { | ||
| strongSelf->_experimentMetadata = parsedMetadata; | ||
| } | ||
| if (parsedActivePayloads) { | ||
| [strongSelf->_activeExperimentPayloads removeAllObjects]; | ||
| [strongSelf->_activeExperimentPayloads addObjectsFromArray:parsedActivePayloads]; | ||
| } | ||
| strongSelf->_databaseLoadCompleted = YES; | ||
| } | ||
| } |
There was a problem hiding this comment.
Update the database load completion block to selectively load each component only if it has not been updated by a concurrent operation yet.
@synchronized(strongSelf) {
if (!strongSelf->_payloadsUpdated && parsedPayloads) {
[strongSelf->_experimentPayloads removeAllObjects];
[strongSelf->_experimentPayloads addObjectsFromArray:parsedPayloads];
}
if (!strongSelf->_metadataUpdated && parsedMetadata) {
strongSelf->_experimentMetadata = parsedMetadata;
}
if (!strongSelf->_activePayloadsUpdated && parsedActivePayloads) {
[strongSelf->_activeExperimentPayloads removeAllObjects];
[strongSelf->_activeExperimentPayloads addObjectsFromArray:parsedActivePayloads];
}
strongSelf->_databaseLoadCompleted = YES;
}| @synchronized(self) { | ||
| _databaseLoadCompleted = YES; | ||
| // cache fetched experiment payloads. | ||
| [_experimentPayloads removeAllObjects]; | ||
| [_experimentPayloads addObjectsFromArray:serializedPayloads]; | ||
| } |
There was a problem hiding this comment.
Set the _payloadsUpdated flag to YES when the payloads are updated concurrently.
| @synchronized(self) { | |
| _databaseLoadCompleted = YES; | |
| // cache fetched experiment payloads. | |
| [_experimentPayloads removeAllObjects]; | |
| [_experimentPayloads addObjectsFromArray:serializedPayloads]; | |
| } | |
| @synchronized(self) { | |
| _payloadsUpdated = YES; | |
| _databaseLoadCompleted = YES; | |
| // cache fetched experiment payloads. | |
| [_experimentPayloads removeAllObjects]; | |
| [_experimentPayloads addObjectsFromArray:serializedPayloads]; | |
| } |
| @synchronized(self) { | ||
| // Check if the calculated start time is newer or equal to what is currently stored, | ||
| // avoiding out-of-order overwrites from logical races during concurrent operations. | ||
| NSTimeInterval currentStoredTime = | ||
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| if (latestStartTime >= currentStoredTime) { | ||
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | ||
| metadataCopy = [_experimentMetadata copy]; | ||
| shouldUpdate = YES; | ||
| } | ||
| } |
There was a problem hiding this comment.
Set the _metadataUpdated flag to YES when the metadata is updated concurrently.
| @synchronized(self) { | |
| // Check if the calculated start time is newer or equal to what is currently stored, | |
| // avoiding out-of-order overwrites from logical races during concurrent operations. | |
| NSTimeInterval currentStoredTime = | |
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | |
| if (latestStartTime >= currentStoredTime) { | |
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | |
| metadataCopy = [_experimentMetadata copy]; | |
| shouldUpdate = YES; | |
| } | |
| } | |
| @synchronized(self) { | |
| _metadataUpdated = YES; | |
| // Check if the calculated start time is newer or equal to what is currently stored, | |
| // avoiding out-of-order overwrites from logical races during concurrent operations. | |
| NSTimeInterval currentStoredTime = | |
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | |
| if (latestStartTime >= currentStoredTime) { | |
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | |
| metadataCopy = [_experimentMetadata copy]; | |
| shouldUpdate = YES; | |
| } | |
| } |
| @synchronized(self) { | ||
| /// Put current fetched experiment payloads into activated experiment DB. | ||
| [_activeExperimentPayloads removeAllObjects]; | ||
| [_activeExperimentPayloads addObjectsFromArray:payloadsCopy]; | ||
| } |
There was a problem hiding this comment.
Set the _activePayloadsUpdated flag to YES when the active payloads are updated concurrently.
| @synchronized(self) { | |
| /// Put current fetched experiment payloads into activated experiment DB. | |
| [_activeExperimentPayloads removeAllObjects]; | |
| [_activeExperimentPayloads addObjectsFromArray:payloadsCopy]; | |
| } | |
| @synchronized(self) { | |
| _activePayloadsUpdated = YES; | |
| /// Put current fetched experiment payloads into activated experiment DB. | |
| [_activeExperimentPayloads removeAllObjects]; | |
| [_activeExperimentPayloads addObjectsFromArray:payloadsCopy]; | |
| } |
| } | ||
|
|
||
| // Update the last experiment start time with the latest payload. | ||
| [self updateExperimentStartTime]; |
There was a problem hiding this comment.
Issue: Race Condition / Inconsistent Payload Snapshot in Experiment Activation
In updateExperimentsWithHandler:, a consistent snapshot of the payloads is captured at the beginning (payloadsCopy). However, [self updateExperimentStartTime] is then called, which internally takes a new snapshot of _experimentPayloads via latestStartTimeWithExistingLastStartTime:.
If updateExperimentsWithResponse: runs concurrently between these two steps, updateExperimentStartTime will calculate the new start time based on the newer payloads, updating _experimentMetadata[kExperimentMetadataKeyLastStartTime] to that newer timestamp.
But updateExperimentsWithHandler: will proceed to call updateExperimentsWithServiceOrigin:... using the older payloadsCopy.
When the newer payloads are eventually activated later, FIRExperimentController will see that the stored lastStartTime is already equal to or greater than their start time, causing those new experiments to be silently skipped and never set to Firebase Analytics.
Recommendation
Refactor updateExperimentStartTime to accept the payloads array as a parameter (e.g., updateExperimentStartTimeWithPayloads:), and pass the consistent payloadsCopy snapshot to it.
| [self updateExperimentStartTime]; | |
| [self updateExperimentStartTimeWithPayloads:payloadsCopy]; |
| - (void)updateExperimentStartTime { | ||
| NSTimeInterval existingLastStartTime = | ||
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| NSTimeInterval existingLastStartTime; | ||
| @synchronized(self) { | ||
| existingLastStartTime = [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| } | ||
|
|
||
| NSTimeInterval latestStartTime = | ||
| [self latestStartTimeWithExistingLastStartTime:existingLastStartTime]; | ||
|
|
||
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | ||
| NSDictionary *metadataCopy = nil; | ||
| BOOL shouldUpdate = NO; | ||
| @synchronized(self) { | ||
| // Check if the calculated start time is newer or equal to what is currently stored, | ||
| // avoiding out-of-order overwrites from logical races during concurrent operations. | ||
| NSTimeInterval currentStoredTime = | ||
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| if (latestStartTime >= currentStoredTime) { | ||
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | ||
| metadataCopy = [_experimentMetadata copy]; | ||
| shouldUpdate = YES; | ||
| } | ||
| } | ||
|
|
||
| if (![NSJSONSerialization isValidJSONObject:_experimentMetadata]) { | ||
| if (!shouldUpdate) { | ||
| return; | ||
| } | ||
|
|
||
| if (![NSJSONSerialization isValidJSONObject:metadataCopy]) { | ||
| FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028", | ||
| @"Invalid fetched experiment metadata to be serialized."); | ||
| return; | ||
| } | ||
| NSError *error; | ||
| NSData *serializedExperimentMetadata = | ||
| [NSJSONSerialization dataWithJSONObject:_experimentMetadata | ||
| [NSJSONSerialization dataWithJSONObject:metadataCopy | ||
| options:NSJSONWritingPrettyPrinted | ||
| error:&error]; | ||
| [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata | ||
| value:serializedExperimentMetadata | ||
| completionHandler:nil]; | ||
| if (serializedExperimentMetadata) { | ||
| __weak __typeof__(self) weakSelf = self; | ||
| dispatch_async(self.experimentDBQueue, ^{ | ||
| __typeof__(self) strongSelf = weakSelf; | ||
| if (!strongSelf) return; | ||
| [strongSelf->_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata | ||
| value:serializedExperimentMetadata | ||
| completionHandler:nil]; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor updateExperimentStartTime to support passing a consistent snapshot of payloads, while keeping the parameterless version as a wrapper for backward compatibility and tests.
- (void)updateExperimentStartTime {
[self updateExperimentStartTimeWithPayloads:self.experimentPayloads];
}
- (void)updateExperimentStartTimeWithPayloads:(NSArray<NSData *> *)payloads {
NSTimeInterval existingLastStartTime;
@synchronized(self) {
existingLastStartTime = [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue];
}
NSTimeInterval latestStartTime = [self.experimentController
latestExperimentStartTimestampBetweenTimestamp:existingLastStartTime
andPayloads:payloads];
NSDictionary *metadataCopy = nil;
BOOL shouldUpdate = NO;
@synchronized(self) {
_metadataUpdated = YES;
// Check if the calculated start time is newer or equal to what is currently stored,
// avoiding out-of-order overwrites from logical races during concurrent operations.
NSTimeInterval currentStoredTime =
[_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue];
if (latestStartTime >= currentStoredTime) {
_experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime);
metadataCopy = [_experimentMetadata copy];
shouldUpdate = YES;
}
}
if (!shouldUpdate) {
return;
}
if (![NSJSONSerialization isValidJSONObject:metadataCopy]) {
FIRLogError(kFIRLoggerRemoteConfig, @
There was a problem hiding this comment.
Code Review
This pull request addresses a data race condition in RCNConfigExperiment by introducing thread-safe property accessors, using @synchronized blocks for state synchronization, and offloading database operations to a serial dispatch queue. Additionally, a concurrent access unit test has been added to verify thread safety. The review feedback identifies a remaining race condition in updateExperimentsWithHandler: where updateExperimentStartTime queries the live _experimentPayloads instead of using the consistently captured payloadsCopy snapshot, and suggests refactoring the method to accept the payloads array directly to prevent inconsistent metadata updates.
| } | ||
|
|
||
| // Update the last experiment start time with the latest payload. | ||
| [self updateExperimentStartTime]; |
There was a problem hiding this comment.
There is a potential race condition/inconsistency here. payloadsCopy is captured under a lock to ensure consistency during processing. However, calling [self updateExperimentStartTime] queries the current _experimentPayloads again (which might have been updated to a newer set of payloads by a concurrent call to updateExperimentsWithResponse:).
If _experimentPayloads was updated concurrently, updateExperimentStartTime will calculate and persist a newer last_experiment_start_time (e.g., 200) to the database. But updateExperimentsWithHandler: will proceed to call updateExperimentsWithServiceOrigin:... using the older payloadsCopy (e.g., with start time 100).
This results in an inconsistent state where the database metadata says the last start time is 200, but the active experiments are actually the older ones. On subsequent runs, the newer experiments (with start time 200) will be skipped because their start time is not strictly greater than the stored last_experiment_start_time (which is already 200), causing them to never be activated in Firebase Analytics.
To fix this, we should pass the captured payloadsCopy to a helper method updateExperimentStartTimeWithPayloads:.
| [self updateExperimentStartTime]; | |
| [self updateExperimentStartTimeWithPayloads:payloadsCopy]; |
| - (void)updateExperimentStartTime { | ||
| NSTimeInterval existingLastStartTime = | ||
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| NSTimeInterval existingLastStartTime; | ||
| @synchronized(self) { | ||
| existingLastStartTime = [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| } | ||
|
|
||
| NSTimeInterval latestStartTime = | ||
| [self latestStartTimeWithExistingLastStartTime:existingLastStartTime]; | ||
|
|
||
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | ||
| NSDictionary *metadataCopy = nil; | ||
| BOOL shouldUpdate = NO; | ||
| @synchronized(self) { | ||
| // Check if the calculated start time is newer or equal to what is currently stored, | ||
| // avoiding out-of-order overwrites from logical races during concurrent operations. | ||
| NSTimeInterval currentStoredTime = | ||
| [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue]; | ||
| if (latestStartTime >= currentStoredTime) { | ||
| _experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime); | ||
| metadataCopy = [_experimentMetadata copy]; | ||
| shouldUpdate = YES; | ||
| } | ||
| } | ||
|
|
||
| if (![NSJSONSerialization isValidJSONObject:_experimentMetadata]) { | ||
| if (!shouldUpdate) { | ||
| return; | ||
| } | ||
|
|
||
| if (![NSJSONSerialization isValidJSONObject:metadataCopy]) { | ||
| FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028", | ||
| @"Invalid fetched experiment metadata to be serialized."); | ||
| return; | ||
| } | ||
| NSError *error; | ||
| NSData *serializedExperimentMetadata = | ||
| [NSJSONSerialization dataWithJSONObject:_experimentMetadata | ||
| [NSJSONSerialization dataWithJSONObject:metadataCopy | ||
| options:NSJSONWritingPrettyPrinted | ||
| error:&error]; | ||
| [_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata | ||
| value:serializedExperimentMetadata | ||
| completionHandler:nil]; | ||
| if (serializedExperimentMetadata) { | ||
| __weak __typeof__(self) weakSelf = self; | ||
| dispatch_async(self.experimentDBQueue, ^{ | ||
| __typeof__(self) strongSelf = weakSelf; | ||
| if (!strongSelf) return; | ||
| [strongSelf->_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata | ||
| value:serializedExperimentMetadata | ||
| completionHandler:nil]; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor updateExperimentStartTime to use a helper method updateExperimentStartTimeWithPayloads: that accepts the payloads array. This allows updateExperimentsWithHandler: to pass its consistently captured snapshot of payloads, preventing race conditions and inconsistent metadata updates.
- (void)updateExperimentStartTime {
[self updateExperimentStartTimeWithPayloads:self.experimentPayloads];
}
- (void)updateExperimentStartTimeWithPayloads:(NSArray<NSData *> *)payloads {
NSTimeInterval existingLastStartTime;
@synchronized(self) {
existingLastStartTime = [_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue];
}
NSTimeInterval latestStartTime = [self.experimentController
latestExperimentStartTimestampBetweenTimestamp:existingLastStartTime
andPayloads:payloads];
NSDictionary *metadataCopy = nil;
BOOL shouldUpdate = NO;
@synchronized(self) {
// Check if the calculated start time is newer or equal to what is currently stored,
// avoiding out-of-order overwrites from logical races during concurrent operations.
NSTimeInterval currentStoredTime =
[_experimentMetadata[kExperimentMetadataKeyLastStartTime] doubleValue];
if (latestStartTime >= currentStoredTime) {
_experimentMetadata[kExperimentMetadataKeyLastStartTime] = @(latestStartTime);
metadataCopy = [_experimentMetadata copy];
shouldUpdate = YES;
}
}
if (!shouldUpdate) {
return;
}
if (![NSJSONSerialization isValidJSONObject:metadataCopy]) {
FIRLogError(kFIRLoggerRemoteConfig, @"I-RCN000028",
@"Invalid fetched experiment metadata to be serialized.");
return;
}
NSError *error;
NSData *serializedExperimentMetadata =
[NSJSONSerialization dataWithJSONObject:metadataCopy
options:NSJSONWritingPrettyPrinted
error:&error];
if (serializedExperimentMetadata) {
__weak __typeof__(self) weakSelf = self;
dispatch_async(self.experimentDBQueue, ^{
__typeof__(self) strongSelf = weakSelf;
if (!strongSelf) return;
[strongSelf->_DBManager insertExperimentTableWithKey:@RCNExperimentTableKeyMetadata
value:serializedExperimentMetadata
completionHandler:nil];
});
}
}4 participants
Description
Fixes a data race condition in
RCNConfigExperimentivars (_experimentPayloads,_experimentMetadata,_activeExperimentPayloads) whenactivate()is called shortly after initialization, concurrently racing with the asynchronous experiment DB load completion block.This update refactors and narrows the synchronization locks (
@synchronized(self)) to cover only quick in-memory variable accesses and mutations. Expensive operations (such as JSON serialization, database operations, and calls to external controller methods) have been moved completely outside critical sections to prevent deadlocks and lock contention.Changes
@synchronized(self)locks insideRCNConfigExperiment.m.updateExperimentsWithResponse:, performed JSON payload serialization and_DBManageroperations outside the lock.updateExperimentsWithHandler:, resolved nested locking by retrieving state parameters inside short independent locks, callingupdateExperimentStartTimeand the externalexperimentControlleroutside the lock.updateExperimentStartTime, performed JSON metadata serialization and DB writes outside the lock.updateActiveExperimentsInDBWithPayloads:, performed_DBManagerdelete/insert calls outside the lock.latestStartTimeWithExistingLastStartTime:, copied payloads under a short lock and queried the external controller outside the lock.NSArrayandNSDictionaryto prevent unintended caller modifications, returning immutable copies from the getters to resolve races.nilvalues in setters defensively by fallback-initializing backing mutable ivars to empty collections.testConcurrentAccessinRCNConfigExperimentTest.mwhich verifies correctness under high concurrent stress. Introduced a custom thread-safe test double (RCNConfigDBManagerFake) which overrides database setup and operation methods, entirely bypassing OCMock for concurrent calls to avoid OCMock internal race conditions. Added a 10-second timeout to prevent test hangs.Fixes #16303