Skip to content

Commit

Permalink
Log a soft exception from accessibility event exceptions (facebook#37002
Browse files Browse the repository at this point in the history
)

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
  • Loading branch information
Abbondanzo authored and facebook-github-bot committed Sep 19, 2023
1 parent 06d40f4 commit 5323221
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -945,7 +945,7 @@ private ThemedReactContext getReactContextForView(int reactTag) {
public void sendAccessibilityEvent(int tag, int eventType) {
View view = mTagsToViews.get(tag);
if (view == null) {
throw new JSApplicationIllegalArgumentException("Could not find view with tag " + tag);
throw new RetryableMountingLayerException("Could not find view with tag " + tag);
}
view.sendAccessibilityEvent(eventType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,18 @@ private SendAccessibilityEvent(int tag, int eventType) {

@Override
public void execute() {
mNativeViewHierarchyManager.sendAccessibilityEvent(mTag, mEventType);
try {
mNativeViewHierarchyManager.sendAccessibilityEvent(mTag, mEventType);
} catch (RetryableMountingLayerException e) {
// Accessibility events are similar to commands in that they're imperative
// calls from JS, disconnected from the commit lifecycle, and therefore
// inherently unpredictable and dangerous. If we encounter a "retryable"
// error, that is, a known category of errors that this is likely to hit
// due to race conditions (like the view disappearing after the event is
// queued and before it executes), we log a soft exception and continue along.
// Other categories of errors will still cause a hard crash.
ReactSoftExceptionLogger.logSoftException(TAG, e);
}
}
}

Expand Down

0 comments on commit 5323221

Please sign in to comment.