From 85ceff529f5b04d2e105b82536eed3043de2a849 Mon Sep 17 00:00:00 2001 From: Saad Najmi Date: Wed, 16 Nov 2022 06:27:35 -0800 Subject: [PATCH] Port RCTDisplayLink fix from React Native macOS (#35359) Summary: Cherry pick https://github.com/microsoft/react-native-macos/commit/e877ebfe083ffaa252738477098322504492f4be from React Native macOS into React Native Core. We've had this bug fix in our fork for a while now, so it's probably safe. === Original change notes === First, misc straightforward deprecated API updates. Second, defensive changes to potentially address RCTDisplayLink crash. 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. ## Changelog [Internal] [Changed] - RCTDisplayLink fix plus bonus deprecated API cleanup Pull Request resolved: https://github.com/facebook/react-native/pull/35359 Test Plan: CI should pass. Reviewed By: cipolleschi Differential Revision: D41335438 Pulled By: christophpurrer fbshipit-source-id: 5e97d28eab7dd8e5c81d0a5386df6631e16405a2 --- React/Base/RCTDisplayLink.m | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/React/Base/RCTDisplayLink.m b/React/Base/RCTDisplayLink.m index 6c42e3241c8..4aed812ed7e 100644 --- a/React/Base/RCTDisplayLink.m +++ b/React/Base/RCTDisplayLink.m @@ -95,6 +95,13 @@ - (void)invalidate { + // ensure observer callbacks do not hold a reference to weak self via pauseCallback + for (RCTModuleData *moduleData in _frameUpdateObservers) { + id observer = (id)moduleData.instance; + [observer setPauseCallback:nil]; + } + [_frameUpdateObservers removeAllObjects]; // just to be explicit + [_jsDisplayLink invalidate]; }