Skip to content

Mac RCTDisplayLink fix plus bonus deprecated API cleanup #854

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.

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

Merged
merged 8 commits into from
Oct 15, 2021

Conversation

ntre
Copy link

@ntre ntre commented Oct 12, 2021

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

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.

Test Plan

N/A

ntre and others added 6 commits October 11, 2021 15:55
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.
@ntre ntre requested a review from alloy as a code owner October 12, 2021 23:34
@pull-bot
Copy link

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 6f35dab

@harrieshin
Copy link

this is great! @HeyImChris this change should really be in RNCore

Copy link

@HeyImChris HeyImChris left a comment

Choose a reason for hiding this comment

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

This is a great candidate to make the fix in FB's repo and we really don't want to introduce diffs if we can avoid it.

I think it's great to 1) keep this fix here 2) apply this fix to the 0.63-stable branch (what devmain uses) 3) apply this fix upstream to fb's fork 4) remove our macOS TODO tags since we'd be the same again.

I can help with any of these steps, but that'd probably be the most correct way to approach an awesome preventative fix like this!

@ghost ghost removed the Needs: Author Feedback label Oct 15, 2021
@ntre ntre merged commit e877ebf into microsoft:master Oct 15, 2021
HeyImChris pushed a commit to HeyImChris/react-native-macos that referenced this pull request Oct 27, 2021
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.
HeyImChris added a commit that referenced this pull request Oct 27, 2021
* 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.

* podfile update

Co-authored-by: Nick Trescases <[email protected]>
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Nov 16, 2022
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants