Skip to content

Commit e877ebf

Browse files
authored
Mac RCTDisplayLink fix plus bonus deprecated API cleanup (#854)
Defensive changes to Mac RCTDisplayLink Real-world data suggests crashes with this stack: CVDisplayLink::start() -[RCTDisplayLink updateJSDisplayLinkState] (RCTDisplayLink.m:157) -[RCTDisplayLink registerModuleForFrameUpdates:withModuleData:]_block_invoke (RCTDisplayLink.m:67) -[RCTTiming timerDidFire] (RCTTiming.mm:324) -[_RCTTimingProxy timerDidFire] (RCTTiming.mm:93) Some symbols are missing in this stack, presumably due to compiler optimizations. -updateJSDisplayLinkState is calling CVDisplayLinkStart as a result of a call to "_jsDisplayLink.paused = NO". -registerModuleForFrameUpdates block is presumably getting called via pauseCallback, likely via [RCTTiming startTimers], presumably owned by RCTCxxBridge. The most likely immediate explanation for the crash is that we are calling CVDisplayLinkStart with a zombie _displayLink pointer. However there is a lot of indirection here as well as thread-hopping, and unfortunately no clearly incorrect code that would explain such a zombie pointer. Some defensive changes: -explicitly remove the association to pauseCallback when underlying display link object is invalidated. -remove a prior attempt at additional check in updateJSDisplayLinkState itself as it is not relevant. -make sure we explicitly set _displayLink to NULL when we release it, such that there is one less failure point.
1 parent 8ed0e0e commit e877ebf

File tree

2 files changed

+11
-3
lines changed

2 files changed

+11
-3
lines changed

React/Base/RCTDisplayLink.m

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,15 @@ - (void)dealloc
9696

9797
- (void)invalidate
9898
{
99+
// [TODO: GH#858
100+
// ensure observer callbacks do not hold a reference to weak self via pauseCallback
101+
for (RCTModuleData *moduleData in _frameUpdateObservers) {
102+
id<RCTFrameUpdateObserver> observer = (id<RCTFrameUpdateObserver>)moduleData.instance;
103+
[observer setPauseCallback:nil];
104+
}
105+
[_frameUpdateObservers removeAllObjects]; // just to be explicit
106+
// TODO: GH#858]
107+
99108
[_jsDisplayLink invalidate];
100109
}
101110

@@ -146,9 +155,7 @@ - (void)updateJSDisplayLinkState
146155
BOOL pauseDisplayLink = YES;
147156
for (RCTModuleData *moduleData in _frameUpdateObservers) {
148157
id<RCTFrameUpdateObserver> observer = (id<RCTFrameUpdateObserver>)moduleData.instance;
149-
BOOL validObserverProtocol = [observer conformsToProtocol:@protocol(RCTFrameUpdateObserver)]; // [TODO: GH#774- add protocol check before accessing data
150-
RCTAssert(validObserverProtocol, @"Observer does not conform to necessary protocol (RCTFrameUpdateObserver)");
151-
if (validObserverProtocol && !observer.paused) { // TODO: GH#774- add protocol check before accessing data]
158+
if (!observer.paused) {
152159
pauseDisplayLink = NO;
153160
break;
154161
}

React/Base/macOS/RCTPlatformDisplayLink.m

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ - (void)dealloc
6565
if (_displayLink != NULL) {
6666
CVDisplayLinkStop(_displayLink);
6767
CVDisplayLinkRelease(_displayLink);
68+
_displayLink = NULL;
6869
}
6970
}
7071

0 commit comments

Comments
 (0)