[Perf] Fix a race condition - #16153
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 |
![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 race condition crash by changing the remoteConfigFlags property from nonatomic to atomic and updates the CHANGELOG accordingly. The review feedback identifies an improvement opportunity to explicitly mark the remoteConfigFlags property as nullable to ensure correct static analysis and Swift interoperability, as it is lazily initialized and currently falls under a non-null assumption block.
4 participants
Fix #16144
AI analysis:
Crash Analysis
The crash occurs in [FPRConfigurations sdkDisabledVersions] at line 271 of
FPRConfigurations.m
(or alternatively at line 224 in sdkEnabled). Both of these lines correspond to the exact same check:
The crash is a segmentation fault (SIGSEGV) inside objc_retain, which indicates that the app is attempting to retain an invalid or garbage pointer.
The Root Cause: Data Race on remoteConfigFlags
This is a race condition caused by unsafe concurrent access to the remoteConfigFlags property:
Non-atomic Property: In FPRConfigurations+Private.h, the property is declared as nonatomic:
nonatomic properties are not thread-safe. If one thread reads the property while another is writing to it, the behavior is undefined, often resulting in a pointer that points to garbage or a deallocated object.
Concurrent Access Patterns:
Write: The property is initialized lazily in setupRemoteConfigFlags, which is called inside update (line 119). This runs asynchronously on self.updateQueue (a background serial queue).
Read: Methods like sdkEnabled and sdkDisabledVersions read self.remoteConfigFlags directly from whatever thread they are called on.
In this specific crash, Thread 0 (the main thread) was calling sdkEnabled during early app startup (triggered by StartupAnalytics.Trace.init), while the background updateQueue was likely trying to initialize remoteConfigFlags at the same time. The main thread read a partially written or non-synchronized pointer, leading to the crash in objc_retain.
Proposed Solution
The most straightforward and safe fix is to make the remoteConfigFlags property atomic. This ensures that the getter and setter for the pointer are synchronized, preventing the read/write race.