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 |
There was a problem hiding this comment.
Code Review
This pull request transitions the screen trace tracking mechanism from NSMapTable to a thread-safe NSMutableDictionary protected by an NSLock, utilizing a new FPRScreenTraceHolder class. Review feedback identifies a critical risk of an NSInvalidArgumentException if a nil view controller is processed and notes a potential memory leak because NSMutableDictionary lacks the automatic cleanup behavior of weak-key NSMapTable. Additionally, a unit test was identified as having decreased rigor regarding memory management verification.
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses race condition crashes in FPRScreenTraceTracker by replacing the NSMapTable used for tracking active screen traces with a thread-safe NSMutableDictionary protected by an NSLock. It introduces a FPRScreenTraceHolder to manage weak references to view controllers and strong references to traces, and includes a cleanup mechanism for stale entries. Unit tests were updated to accommodate these changes, and a new stress test was added to verify thread safety. A review comment suggests improving a busy-wait loop in the tests to prevent potential infinite loops.
0ded3bb to
bf99bdd
Compare
15b3ae7 to
03d2a3d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses race condition crashes in FPRScreenTraceTracker by replacing NSMapTable with a thread-safe NSMutableDictionary protected by an NSLock. It also introduces an FPRScreenTraceHolder to manage trace associations and adds a cleanup mechanism for stale entries. I have provided feedback regarding the use of a safer polling mechanism in tests to avoid potential hangs, as well as suggestions to use NSRecursiveLock and optimize the locking sequence in the tracker for better atomicity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses race condition crashes in FPRScreenTraceTracker by replacing NSMapTable with a thread-safe NSMutableDictionary protected by an NSRecursiveLock. It introduces a FPRScreenTraceHolder class to manage weak references to view controllers and includes a cleanup mechanism for stale entries. Review feedback suggests simplifying the logic for handling pointer reuse during trace removal, using idiomatic array factory methods, and increasing the iteration count in the stress test to improve the reliability of concurrency checks.
Fix #14473
Based on the source changes in PR #16072, the fix addresses a crash related to
NSMapTableby replacing it with a more manual, thread-safe implementation usingNSMutableDictionaryandNSLock.Core Changes
The PR moves away from
NSMapTable(configured with weak keys and strong values) in favor of a custom "Holder" pattern to manageUIViewControllerreferences and their associated performance traces.FPRScreenTraceHolder: A new internal class that holds aweakreference to theUIViewControllerand astrongreference to theFIRTrace.activeScreenTracesproperty changed from anNSMapTableto a standardNSMutableDictionary<NSValue *, FPRScreenTraceHolder *>.activeScreenTracesLock(NSLock) to synchronize access to the dictionary, which was likely a contributor to the original crash.Technical Analysis
NSValuestores the raw pointer (non-retained), the code now verifiesif (holder && holder.viewController != viewController)before starting or stopping a trace. This prevents a new view controller from accidentally inheriting a trace from a deallocated object that happened to share the same memory address.weakproperty in theFPRScreenTraceHolder, the SDK avoids retain cycles while still allowing the dictionary to be cleaned up when a view controller is dismissed or deallocated.testScreenTraceTrackerStress) that spawns multiple threads to hammer the appear/disappear/deallocate cycle simultaneously, ensuring the new locking mechanism holds up under pressure.Observations
NSMapTableis designed to handle weak-to-strong mappings automatically, it is notoriously finicky regarding thread safety and when it actually "cleans up" nilled-out entries. This manual implementation is more verbose but significantly more predictable.stopScreenTraceForViewController, the entry is now explicitly removed from the dictionary after the trace is stopped, which is cleaner than relying on the internal "garbage collection" of a map table.