Mac RCTDisplayLink fix plus bonus deprecated API cleanup (#854)#869
Merged
HeyImChris merged 2 commits intomicrosoft:0.63-stablefrom Oct 27, 2021
Merged
Mac RCTDisplayLink fix plus bonus deprecated API cleanup (#854)#869HeyImChris merged 2 commits intomicrosoft:0.63-stablefrom
HeyImChris merged 2 commits intomicrosoft:0.63-stablefrom
Conversation
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.
|
Saadnajmi
approved these changes
Oct 27, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
(then delete these lines)
Please select one of the following
Summary
Changelog
[CATEGORY] [TYPE] - Message
Test Plan
This was performed with the following command
git cherry-pick e877ebfe083ffaa252738477098322504492f4be