-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix #14273: Prevent race condition crash in FPRTraceBackgroundActivityTracker #15382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
f17995d
a0b811f
cfcf2b6
0810b73
5f1bc14
bc03222
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,4 +61,213 @@ - (void)testBackgroundTracking { | |
}]; | ||
} | ||
|
||
/** Tests that synchronous observer registration works correctly and observers are immediately | ||
* available. */ | ||
- (void)testObservers_synchronousRegistrationAddsObserver { | ||
NSNotificationCenter *notificationCenter = [NSNotificationCenter defaultCenter]; | ||
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
XCTAssertNotNil(tracker); | ||
|
||
[notificationCenter postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
XCTAssertEqual(tracker.traceBackgroundState, FPRTraceStateForegroundOnly); | ||
|
||
tracker = nil; | ||
XCTAssertNil(tracker); | ||
XCTAssertNoThrow([notificationCenter postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]); | ||
XCTAssertNoThrow([notificationCenter | ||
postNotificationName:UIApplicationDidEnterBackgroundNotification | ||
object:[UIApplication sharedApplication]]); | ||
} | ||
|
||
/** Tests rapid creation and deallocation to verify race condition fix. */ | ||
JesusRojass marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
- (void)testRapidCreationAndDeallocation_noRaceCondition { | ||
// This test simulates the real crash scenario by forcing async dispatch timing | ||
XCTestExpectation *expectation = | ||
[self expectationWithDescription:@"All async operations complete"]; | ||
|
||
__block int completedOperations = 0; | ||
const int totalOperations = 50; | ||
|
||
for (int i = 0; i < totalOperations; i++) { | ||
@autoreleasepool { | ||
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
XCTAssertNotNil(tracker); | ||
|
||
// Force multiple runloop cycles to increase chance of race condition | ||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
// This would crash with old async registration if tracker is deallocated | ||
[[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
|
||
// Increment counter and fulfill expectation when done | ||
completedOperations++; | ||
if (completedOperations == totalOperations) { | ||
[expectation fulfill]; | ||
} | ||
}); | ||
|
||
// Tracker deallocates here immediately due to @autoreleasepool | ||
} | ||
} | ||
|
||
// Wait for all async operations to complete | ||
[self waitForExpectationsWithTimeout:10.0 | ||
handler:^(NSError *error) { | ||
XCTAssertNil( | ||
error, @"Operations timed out - potential deadlock or crash"); | ||
}]; | ||
|
||
// Additional safety check - post more notifications after everything is done | ||
XCTAssertNoThrow([[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]); | ||
XCTAssertNoThrow([[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidEnterBackgroundNotification | ||
object:[UIApplication sharedApplication]]); | ||
} | ||
|
||
/** Tests that observers are registered immediately after init on main thread. */ | ||
- (void)testObservers_immediateRegistrationOnMainThread { | ||
XCTAssertTrue([NSThread isMainThread]); | ||
|
||
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
|
||
[[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
|
||
XCTAssertEqual(tracker.traceBackgroundState, FPRTraceStateForegroundOnly); | ||
} | ||
Comment on lines
+106
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
/** Tests observer registration when created from background thread. */ | ||
- (void)testObservers_registrationFromBackgroundThread { | ||
XCTestExpectation *expectation = [self expectationWithDescription:@"Background thread creation"]; | ||
|
||
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ | ||
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
XCTAssertNotNil(tracker); | ||
|
||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
[[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
|
||
XCTAssertEqual(tracker.traceBackgroundState, FPRTraceStateForegroundOnly); | ||
[expectation fulfill]; | ||
}); | ||
}); | ||
|
||
[self waitForExpectationsWithTimeout:5.0 | ||
handler:^(NSError *error) { | ||
XCTAssertNil(error, @"Test timed out"); | ||
}]; | ||
} | ||
|
||
/** Tests the exact crash scenario with async dispatch timing that would crash with old | ||
* implementation. */ | ||
- (void)testAsyncDispatch_wouldCrashWithOldImplementation { | ||
// This test simulates what the OLD code would do and should crash with async registration | ||
// With the NEW synchronous code, this should pass safely | ||
|
||
XCTestExpectation *expectation = [self expectationWithDescription:@"Async crash test complete"]; | ||
|
||
__block int remainingOperations = 200; | ||
|
||
for (int i = 0; i < 200; i++) { | ||
@autoreleasepool { | ||
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init]; | ||
XCTAssertNotNil(tracker); | ||
|
||
// Simulate the old problematic pattern | ||
__weak typeof(tracker) weakTracker = tracker; | ||
|
||
// This mimics what the OLD async registration would do | ||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
// In old code: tracker might be deallocated here → CRASH | ||
// In new code: observers already registered synchronously → SAFE | ||
|
||
if (weakTracker) { | ||
[[NSNotificationCenter defaultCenter] | ||
postNotificationName:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
} | ||
|
||
remainingOperations--; | ||
if (remainingOperations == 0) { | ||
[expectation fulfill]; | ||
} | ||
}); | ||
|
||
// Immediately deallocate tracker - this creates the race condition window | ||
} | ||
|
||
// Force runloop processing to increase race condition likelihood | ||
[[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.001]]; | ||
} | ||
|
||
[self waitForExpectationsWithTimeout:5.0 | ||
handler:^(NSError *error) { | ||
XCTAssertNil(error, @"Async crash test failed"); | ||
}]; | ||
} | ||
|
||
@end | ||
|
||
/** | ||
* CRASH REPRODUCTION TEST - Only use this to verify the original bug exists | ||
* This simulates the original async registration pattern that would cause crashes | ||
*/ | ||
|
||
@interface CrashReproductionTracker : NSObject | ||
@property(nonatomic, readwrite) int traceBackgroundState; | ||
@end | ||
|
||
@implementation CrashReproductionTracker | ||
|
||
- (instancetype)init { | ||
self = [super init]; | ||
if (self) { | ||
_traceBackgroundState = 0; | ||
|
||
// This is the ORIGINAL problematic code that would crash | ||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
[[NSNotificationCenter defaultCenter] addObserver:self | ||
selector:@selector(handleNotification:) | ||
name:UIApplicationDidBecomeActiveNotification | ||
object:[UIApplication sharedApplication]]; | ||
}); | ||
} | ||
return self; | ||
} | ||
|
||
- (void)handleNotification:(NSNotification *)notification { | ||
_traceBackgroundState = 1; | ||
} | ||
|
||
- (void)dealloc { | ||
[[NSNotificationCenter defaultCenter] removeObserver:self]; | ||
} | ||
|
||
/** | ||
* CRASH REPRODUCTION TEST - Only use this to verify the original bug exists | ||
* This simulates the original async registration pattern that would cause crashes | ||
* WARNING: This test is commented out because it WILL crash with the original async pattern | ||
*/ | ||
|
||
- (void)testCrashReproduction_originalAsyncBug { | ||
JesusRojass marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
// This test WILL crash with the original async pattern | ||
for (int i = 0; i < 100; i++) { | ||
@autoreleasepool { | ||
CrashReproductionTracker *tracker = [[CrashReproductionTracker alloc] init]; | ||
// tracker deallocates here, but async block is still queued → CRASH | ||
} | ||
|
||
// Process run loop to execute queued async blocks | ||
[[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.01]]; | ||
} | ||
} | ||
|
||
@end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@visumickey @eBlender
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