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

[UIManager] Only handle accessibility notifications from the correct RCTAccessibilityManager #3279

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@ide
Collaborator

ide commented Oct 8, 2015

When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues.

I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See #2001 for the kinds of crashes we were seeing.

Test Plan: Loaded a complex app. Crashes on reload seem to have gone away but this was non-deterministic so there may still be other issues.

[UIManager] Only handle accessibility notifications from the correct …
…RCTAccessibilityManager

When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues.

I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager.

Test Plan: Loaded a complex app. Crashes on reload seem to have gone away but this was non-deterministic so there may still be other issues.
@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Oct 8, 2015

Contributor

Fix makes sense. Test failures seem to be unrelated.

@facebook-github-bot shipit

Contributor

nicklockwood commented Oct 8, 2015

Fix makes sense. Test failures seem to be unrelated.

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

facebook-github-bot commented Oct 8, 2015

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/520080238154371/int_phab to review.

@jsierles

This comment has been minimized.

Show comment
Hide comment
@jsierles

jsierles Oct 8, 2015

Contributor

We see this crash in the Playground app a lot as well - couldn't figure it out. Nice one!

Contributor

jsierles commented Oct 8, 2015

We see this crash in the Playground app a lot as well - couldn't figure it out. Nice one!

@ide ide closed this in cb8b656 Oct 8, 2015

liaohuqiu added a commit to liaohuqiu/react-native that referenced this pull request Oct 9, 2015

Only handle accessibility notifications from the correct RCTAccessibi…
…lityManager

Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues.

I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See #2001 for the kinds of crashes we were seeing.
Closes facebook#3279

Reviewed By: @​svcscm

Differential Revision: D2521652

Pulled By: @nicklockwood

fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1

@ide ide deleted the expo:text-queue-fix branch Oct 9, 2015

MattFoley added a commit to skillz/react-native that referenced this pull request Nov 9, 2015

Only handle accessibility notifications from the correct RCTAccessibi…
…lityManager

Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues.

I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See #2001 for the kinds of crashes we were seeing.
Closes facebook#3279

Reviewed By: @​svcscm

Differential Revision: D2521652

Pulled By: @nicklockwood

fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1

Crash-- added a commit to Crash--/react-native that referenced this pull request Dec 24, 2015

Only handle accessibility notifications from the correct RCTAccessibi…
…lityManager

Summary: When you reload and create a new bridge, one of the things that happens during setup is that the RCTAccessibilityManager fires a notification. The old bridge would receive this notification from the new bridge's RCTAccessibilityManager, which we don't want, especially because the two are running on different shadow queues.

I believe this led to a gnarly crash in NSConcreteTextStorage because RCTMeasure in RCTShadowText.m was getting called for the old RCTText (getting destroyed) from a notification fired from the new shadow queue. The fix is for the UIManager to handle notifications only from its bridge's RCTAccessibilityManager. See #2001 for the kinds of crashes we were seeing.
Closes facebook#3279

Reviewed By: @​svcscm

Differential Revision: D2521652

Pulled By: @nicklockwood

fb-gh-sync-id: a4ffe3ef8304667727e573e2a2e8b716e1d2f3e1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment