Fix #14273: Prevent race condition crash in FPRTraceBackgroundActivityTracker #15382
Conversation
Replace async notification observer registration with synchronous registration to eliminate timing-dependent crashes when objects are deallocated quickly. Fixes firebase#14273
|
@visumickey Created PR 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 effectively addresses a critical race condition in FPRTraceBackgroundActivityTracker by switching from asynchronous to synchronous notification observer registration. The solution is robust, handling initialization from both main and background threads correctly to prevent deadlocks. The accompanying tests are comprehensive, not only verifying the fix under race conditions but also including a clear reproduction case for the original bug, which is excellent for future reference. The code is clean and the changes are well-explained. I have one minor suggestion in the tests to improve clarity.
FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
Excellent work on this pull request. You've correctly identified and fixed a critical race condition in FPRTraceBackgroundActivityTracker by replacing the asynchronous observer registration with a synchronous approach. The use of if ([NSThread isMainThread]) followed by dispatch_sync is a robust pattern that ensures thread safety and prevents the deallocation issue. The new unit tests are thorough and provide great coverage for the fix, including a specific test to reproduce and verify the absence of the race condition. I have one minor suggestion regarding code style for better maintainability.
FirebasePerformance/Sources/AppActivity/FPRTraceBackgroundActivityTracker.m
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a race condition crash in FPRTraceBackgroundActivityTracker by replacing asynchronous notification observer registration with a synchronous approach. The implementation correctly handles object initialization from both main and background threads, and the new unit tests are comprehensive, verifying the fix under various conditions, including rapid allocation/deallocation cycles. I have one minor suggestion to consolidate a redundant test case, but overall this is an excellent fix.
| - (void)testObservers_immediateRegistrationOnMainThread { | ||
| XCTAssertTrue([NSThread isMainThread]); | ||
|
|
||
| FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
|
|
||
| [[NSNotificationCenter defaultCenter] | ||
| postNotificationName:UIApplicationDidBecomeActiveNotification | ||
| object:[UIApplication sharedApplication]]; | ||
|
|
||
| XCTAssertEqual(tracker.traceBackgroundState, FPRTraceStateForegroundOnly); | ||
| } |
There was a problem hiding this comment.
This test case, testObservers_immediateRegistrationOnMainThread, appears to be redundant. Its logic is already covered in the first half of the testObservers_synchronousRegistrationAddsObserver test (lines 66-73). To improve the conciseness of the test suite and reduce future maintenance, consider removing this test.
There was a problem hiding this comment.
NIT:We usually don't want to duplicate code if possible, if possible try to make it a single tests, so we don't have to maintain extra tests.
|
@visumickey you can review from here, I added some of the Gemini suggestions (though idk if they where any good) lmk if anything! |
| if ([NSThread isMainThread]) { | ||
| [self registerNotificationObservers]; | ||
| } else { | ||
| dispatch_sync(dispatch_get_main_queue(), ^{ |
There was a problem hiding this comment.
I'm worried about a potential deadlock situation here. If we are already in the main thread and doing a dispatch_sync, that is going to cause a deadlock. I think we should protect against that.
There was a problem hiding this comment.
x2, my understating of this issue is not on the threads but if the observers are still alive when the app transition from states, I was proposing something like
If (observers != nil){
//don't do anything
}
esle {
//Create them
}
WDYT @visumickey ??
There was a problem hiding this comment.
I have made a new commit addressing this block of code
The approach is to capture strong self during async observer registration so we can avoid messaging deallocated tracker instances when observers are registered asynchronously on the main queue
Just so self lives long enough to prevent another EXC_BAD_ACCESS crash
lmk what you think
FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
Outdated
Show resolved
Hide resolved
Capture strong self during async observer registration
|
@visumickey Made new changes and added commit, I also re tested everything and it seems fine 
|
Replace async notification observer registration with synchronous registration
to eliminate timing-dependent crashes when objects are deallocated quickly.
Fixes #14273 (Link)
Discussion
The FPRTraceBackgroundActivityTracker class had a race condition during initialization where notification observers were registered asynchronously using dispatch_async(dispatch_get_main_queue()). This created a timing window where the tracker object could be deallocated before the async block executed, causing crashes when the block tried to call addObserver: with a deallocated self reference.
The crash occurred when:
My solution involves to replace the asynchronous observer registration with synchronous registration to eliminate the race condition entirely
Testing
API Changes