Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+94 −9
Open

Fix #14273: Prevent race condition crash in FPRTraceBackgroundActivityTracker #15382

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f17995d
fix: prevent race condition crash in FPRTraceBackgroundActivityTracker
JesusRojass Oct 3, 2025
a0b811f
Merge remote-tracking branch 'origin/main' into JesusRojass/#14273
JesusRojass Oct 6, 2025
cfcf2b6
run Style.sh
JesusRojass Oct 6, 2025
0810b73
Removed Development tests
JesusRojass Oct 6, 2025
5f1bc14
added Gemini Suggestion
JesusRojass Oct 6, 2025
bc03222
Corrections based on comments regarding dead lock and nitpick
JesusRojass Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions FirebasePerformance/Sources/AppActivity/FPRTraceBackgroundActivityTracker.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ @interface FPRTraceBackgroundActivityTracker ()

@property(nonatomic, readwrite) FPRTraceState traceBackgroundState;

- (void)registerNotificationObservers;

@end

@implementation FPRTraceBackgroundActivityTracker
Expand All @@ -35,21 +37,29 @@ - (instancetype)init {
} else {
_traceBackgroundState = FPRTraceStateForegroundOnly;
}
dispatch_async(dispatch_get_main_queue(), ^{
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(applicationDidBecomeActive:)
name:UIApplicationDidBecomeActiveNotification
object:[UIApplication sharedApplication]];

[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(applicationDidEnterBackground:)
name:UIApplicationDidEnterBackgroundNotification
object:[UIApplication sharedApplication]];
});
if ([NSThread isMainThread]) {
[self registerNotificationObservers];
} else {
dispatch_sync(dispatch_get_main_queue(), ^{
Copy link
Contributor

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.

Copy link
Contributor

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 ??

Copy link
Contributor Author

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

[self registerNotificationObservers];
});
}
}
return self;
}

- (void)registerNotificationObservers {
[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(applicationDidBecomeActive:)
name:UIApplicationDidBecomeActiveNotification
object:[UIApplication sharedApplication]];

[[NSNotificationCenter defaultCenter] addObserver:self
selector:@selector(applicationDidEnterBackground:)
name:UIApplicationDidEnterBackgroundNotification
object:[UIApplication sharedApplication]];
}

- (void)dealloc {
// Remove all the notification observers registered.
[[NSNotificationCenter defaultCenter] removeObserver:self];
Expand Down
78 changes: 78 additions & 0 deletions FirebasePerformance/Tests/Unit/FPRTraceBackgroundActivityTrackerTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,82 @@ - (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. */
- (void)testRapidCreationAndDeallocation_noRaceCondition {
for (int i = 0; i < 100; i++) {
@autoreleasepool {
FPRTraceBackgroundActivityTracker *tracker = [[FPRTraceBackgroundActivityTracker alloc] init];
XCTAssertNotNil(tracker);

[[NSNotificationCenter defaultCenter]
postNotificationName:UIApplicationDidBecomeActiveNotification
object:[UIApplication sharedApplication]];
}
}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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");
}];
}

@end