Skip to content

Commit

Permalink
fix: reportFullyDisplayed data race
Browse files Browse the repository at this point in the history
Fix a data race when calling reportFullyDisplayed from a background
thread by synchronizing the call to the main queue.
  • Loading branch information
philipphofmann committed Apr 30, 2024
1 parent 265f000 commit 345a565
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 5 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

- Ignore SentryFramesTracker thread sanitizer data races (#3922)
- Handle no releaseName in WatchDogTerminationLogic (#3919)
- Fix data race when calling reportFullyDisplayed from a background thread (#3926)


## 8.25.0

Expand Down
8 changes: 7 additions & 1 deletion Sources/Sentry/SentryTimeToDisplayTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#if SENTRY_HAS_UIKIT

# import "SentryDependencyContainer.h"
# import "SentryDispatchQueueWrapper.h"
# import "SentryFramesTracker.h"
# import "SentryLog.h"
# import "SentryMeasurementValue.h"
Expand All @@ -26,6 +27,7 @@

@property (nonatomic, weak) SentrySpan *initialDisplaySpan;
@property (nonatomic, weak) SentrySpan *fullDisplaySpan;
@property (nonatomic, strong, readonly) SentryDispatchQueueWrapper *dispatchQueueWrapper;

@end

Expand All @@ -38,10 +40,12 @@ @implementation SentryTimeToDisplayTracker {

- (instancetype)initForController:(UIViewController *)controller
waitForFullDisplay:(BOOL)waitForFullDisplay
dispatchQueueWrapper:(SentryDispatchQueueWrapper *)dispatchQueueWrapper
{
if (self = [super init]) {
_controllerName = [SwiftDescriptor getObjectClassName:controller];
_waitForFullDisplay = waitForFullDisplay;
_dispatchQueueWrapper = dispatchQueueWrapper;
_initialDisplayReported = NO;
_fullyDisplayedReported = NO;
}
Expand Down Expand Up @@ -119,7 +123,9 @@ - (void)reportInitialDisplay

- (void)reportFullyDisplayed
{
_fullyDisplayedReported = YES;
// All other accesses to _fullyDisplayedReported run on the main thread.
// To avoid using locks, we execute this on the main queue instead.
[_dispatchQueueWrapper dispatchOnMainQueue:^{ self->_fullyDisplayedReported = YES; }];
}

- (void)framesTrackerHasNewFrame:(NSDate *)newFrameDate
Expand Down
6 changes: 5 additions & 1 deletion Sources/Sentry/SentryUIViewControllerPerformanceTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#if SENTRY_HAS_UIKIT

# import "SentryDependencyContainer.h"
# import "SentryHub.h"
# import "SentryLog.h"
# import "SentryOptions.h"
Expand All @@ -23,6 +24,7 @@

@property (nonatomic, strong) SentryPerformanceTracker *tracker;
@property (nullable, nonatomic, weak) SentryTimeToDisplayTracker *currentTTDTracker;
@property (nonatomic, strong, readonly) SentryDispatchQueueWrapper *dispatchQueueWrapper;

@end

Expand All @@ -46,6 +48,7 @@ - (instancetype)init
inAppExcludes:options.inAppExcludes];

_enableWaitForFullDisplay = NO;
_dispatchQueueWrapper = SentryDependencyContainer.sharedInstance.dispatchQueueWrapper;
}
return self;
}
Expand Down Expand Up @@ -142,7 +145,8 @@ - (void)createTimeToDisplay:(UIViewController *)controller

SentryTimeToDisplayTracker *ttdTracker =
[[SentryTimeToDisplayTracker alloc] initForController:controller
waitForFullDisplay:self.enableWaitForFullDisplay];
waitForFullDisplay:self.enableWaitForFullDisplay
dispatchQueueWrapper:_dispatchQueueWrapper];

objc_setAssociatedObject(controller, &SENTRY_UI_PERFORMANCE_TRACKER_TTD_TRACKER, ttdTracker,
OBJC_ASSOCIATION_ASSIGN);
Expand Down
4 changes: 3 additions & 1 deletion Sources/Sentry/include/SentryTimeToDisplayTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

@class SentrySpan;
@class SentryTracer;
@class SentryDispatchQueueWrapper;
@class UIViewController;

NS_ASSUME_NONNULL_BEGIN
Expand All @@ -25,7 +26,8 @@ SENTRY_NO_INIT
@property (nonatomic, readonly) BOOL waitForFullDisplay;

- (instancetype)initForController:(UIViewController *)controller
waitForFullDisplay:(BOOL)waitForFullDisplay;
waitForFullDisplay:(BOOL)waitForFullDisplay
dispatchQueueWrapper:(SentryDispatchQueueWrapper *)dispatchQueueWrapper;

- (void)startForTracer:(SentryTracer *)tracer;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {
}

func getSut(for controller: UIViewController, waitForFullDisplay: Bool) -> SentryTimeToDisplayTracker {
return SentryTimeToDisplayTracker(for: controller, waitForFullDisplay: waitForFullDisplay)
return SentryTimeToDisplayTracker(for: controller, waitForFullDisplay: waitForFullDisplay, dispatchQueueWrapper: SentryDispatchQueueWrapper())
}

func getTracer() throws -> SentryTracer {
Expand Down Expand Up @@ -282,6 +282,18 @@ class SentryTimeToDisplayTrackerTest: XCTestCase {
assertMeasurement(tracer: tracer, name: "time_to_full_display", duration: 1_000)
}

func testReportFullyDisplayed_GetsDispatchedOnMainQueue() {
let dispatchQueueWrapper = TestSentryDispatchQueueWrapper()

let sut = SentryTimeToDisplayTracker(for: UIViewController(), waitForFullDisplay: true, dispatchQueueWrapper: dispatchQueueWrapper)

let invocationsBefore = dispatchQueueWrapper.blockOnMainInvocations.count
sut.reportFullyDisplayed()

let expectedInvocations = invocationsBefore + 1
expect(dispatchQueueWrapper.blockOnMainInvocations.count).to(equal(expectedInvocations), description: "reportFullyDisplayed should be dispatched on the main queue. ")
}

func testNotWaitingForFullyDisplayed_AfterTracerTimesOut() throws {
fixture.dateProvider.setDate(date: Date(timeIntervalSince1970: 9))

Expand Down
2 changes: 1 addition & 1 deletion Tests/SentryTests/SentryHubTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,7 @@ class SentryHubTests: XCTestCase {
class TestTimeToDisplayTracker: SentryTimeToDisplayTracker {

init() {
super.init(for: UIViewController(), waitForFullDisplay: false)
super.init(for: UIViewController(), waitForFullDisplay: false, dispatchQueueWrapper: SentryDispatchQueueWrapper())
}

var registerFullDisplayCalled = false
Expand Down

0 comments on commit 345a565

Please sign in to comment.