Skip to content
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

Log a soft exception from accessibility event exceptions #37002

Closed
wants to merge 3 commits into from

Conversation

Abbondanzo
Copy link
Contributor

@Abbondanzo Abbondanzo commented Apr 20, 2023

Summary:

This PR attempts to solve this issue I created last year, where accessibility events can fire after a view has been unmounted, causing the entire application to crash. This newly added try/catch block is modeled and copies verbiage exactly from the Fabric implementation of accessibility events here and here.

Changelog:

[ANDROID] [FIXED] - Fixed an issue where calling Accessibility.setAccessibilityFocus on an unmounted component would crash

Test Plan:

  1. I reimplemented the crash demo from the Snack showcased in this issue. I created the branch here, although I may archive or delete this repo in the future so the link is not permanent
  2. I disabled the new Fabric architecture. This crash only occurs in the old architecture
  3. I pressed the button that would set accessibility focus on an unmounted component, and it logs a soft exception as seen in the screenshot below. I've also attached a screenshot from an identical soft exception that already gets logged by Fabric
  4. In both cases, the app continues to function as expected
Example SoftException from this change Example SoftException from Fabric (no changes to this code)
image image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 20, 2023
@analysis-bot
Copy link

analysis-bot commented Apr 21, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,333,699 -303
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,573,226 -23
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 1ac3dab
Branch: main

@Abbondanzo
Copy link
Contributor Author

Wanted to bump this one for review since it's still causing issues for a number of our users

@consuelo-sanna
Copy link

Hi @cortinico, @Titozzz,
I tag you here because we were discussing this topic, I hope you don't mind

As explained in the summary by Abbondanzo this PR would solve/improve the issue on Android that crashes if does not find the tag. you also mentioned that findNodeHandle is deprecated/is going to be deprecated - based on that, do you think this PR can be merged in?

I have not found much documentation about deprecating findNodeHandle, would you also be so kind to point me in the right direction for that?
Thank you

@cortinico
Copy link
Contributor

Hey @consuelo-sanna
You mentioned there was a previous discussion around this topic which came to your attention. Could you share it here for future reference?

@consuelo-sanna
Copy link

sure @cortinico, it was this one issue 22807

Then I've also seen that recently this one was also opened up on the same matter issue 37015

@cortinico
Copy link
Contributor

When we met in person, you mentioned that someone shared that this is the "intended behavior". Could you link that conversation?

@consuelo-sanna
Copy link

yes, I was referring to this comment by titozzz

I definitely see titozzz's point of view, but being a feature used for accessibilities reasons I think it would make more sense to handle it more gracefully than crashing, which would also align with what happens on iOS

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Abbondanzo Abbondanzo closed this Sep 18, 2023
@Abbondanzo Abbondanzo reopened this Sep 18, 2023
@Abbondanzo
Copy link
Contributor Author

Syncing my fork reopened this PR, was going to start work on something else from main (converting some of those tests to Kotlin). Want me to close this PR since it's been imported on your end?

@github-actions
Copy link

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against 8cb582f

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 19, 2023
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in 5323221.

ShevO27 pushed a commit to ShevO27/react-native that referenced this pull request Sep 26, 2023
)

Summary:
This PR attempts to solve [this issue](facebook#33021) I created last year, where accessibility events can fire after a view has been unmounted, causing the entire application to crash. This newly added try/catch block is modeled and copies verbiage exactly from the Fabric implementation of accessibility events [here](https://github.com/facebook/react-native/blob/2d9c81780c2bb3c2c2dc8875147261c9a1843eb4/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEventMountItem.java#L30-L43) and [here](https://github.com/facebook/react-native/blob/2d9c81780c2bb3c2c2dc8875147261c9a1843eb4/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/SendAccessibilityEventMountItem.java#L30-L43).

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[ANDROID] [FIXED] - Fixed an issue where calling `Accessibility.setAccessibilityFocus` on an unmounted component would crash

Pull Request resolved: facebook#37002

Test Plan:
1. I reimplemented the crash demo from the Snack showcased in [this issue](facebook#33021). I created the branch [here](https://github.com/Abbondanzo/react-native/pull/1/files), although I may archive or delete this repo in the future so the link is not permanent
1. I disabled the new Fabric architecture. This crash only occurs in the old architecture
1. I pressed the button that would set accessibility focus on an unmounted component, and it logs a soft exception as seen in the screenshot below. I've also attached a screenshot from an identical soft exception that already gets logged by Fabric
1. In both cases, the app continues to function as expected

|Example SoftException from this change|Example SoftException from Fabric (no changes to this code)|
|-|-|
|![image](https://user-images.githubusercontent.com/10366495/233423632-9eaa7c06-90a0-4e5c-ad65-5cc98ea52dc6.png)|![image](https://user-images.githubusercontent.com/10366495/233423233-f8f0bdb4-7f90-44f8-92aa-27992f60ccf3.png)|

Reviewed By: cipolleschi

Differential Revision: D49270491

Pulled By: cortinico

fbshipit-source-id: a489f160be44d78afb751527f70aa88e8a463ee0
@zfrankdesign
Copy link

Has this been released yet? I can't find the RN version needed for this fix. Any help appreciated.

@Abbondanzo
Copy link
Contributor Author

Has this been released yet? I can't find the RN version needed for this fix. Any help appreciated.

5323221
According to the commit it was included in, it should be in 0.73.0 and up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants