Skip to content

Commit

Permalink
Fix threading issue in RNGestureHandlerStateChangeEvent (#1171)
Browse files Browse the repository at this point in the history
## Description

This commit fixes a threading issue connected with `enabled` property of gesture handlers. Changing this property in JS called `updateGestureHandler` in the RNGH Java module which in turn called `setEnabled`. `setEnabled` cancels handler by using `cancel()` method if it was in an active state previously.
This method was mistakenly called directly from the native modules thread - state transition methods are intended to be called from the UI thread.

This made GH orchestrator call `handler.dispatchStateChange()` on the wrong thread. This caused event listeners to receive the event on a non-UI thread (`NodeManager.onEventDispatch()` from Reanimated) via `EventDispatcher`.

Reanimated handles non-UI events in `onEventDispatch` (e.g. `onLayout` event) by adding them to the internal queue and posting frame callback if it wasn't posted previously (`onAnimationFrame()` wasn't called). Then any queued event is handled on UI thread in the next frame. 
Problem is, `EventDispatcher` first calls `onEventDispatch()` of any registered listeners and then runs `maybePostFrameCallbackFromNonUI()` which tries to post frame callback dispatching and disposing events from JS thread. 

So there was a possibility that we:
1. queue event in `NodeManager.onEventDispatch` in native modules thread
2. handle and **dispose** event in `EventDispatcher`, setting extra data to `null`
3. take the event from the queue and try handling it in the `NodeManager.onAnimationFrame`, raising exception.

The solution to that problem is to always run `cancel()` (and any other `stateChange` method) on UI thread.


- Fixes react-navigation/react-navigation/issues/6403
- Fixes satya164/react-native-tab-view/issues/976
- Fixes software-mansion/react-native-reanimated/issues/704

## Test plan

Huge thanks to the @midoushitongtong who provided small enough code example which reproduced this issue.

```jsx
import * as React from 'react';
import { View, StyleSheet, Dimensions, Text } from 'react-native';
import { TabView, SceneMap } from 'react-native-tab-view';

const FirstRoute = () => (
  <View style={[styles.scene, { backgroundColor: '#ff4081' }]}>
    <Text>aaaaa</Text>
    <Text>aaaaa</Text>
    <Text>aaaaa</Text>
    <Text>aaaaa</Text>
    <Text>aaaaa</Text>
  </View>
);

const initialLayout = { width: Dimensions.get('window').width };

const InnerTab = () => {
  const [index, setIndex] = React.useState(0);
  const [routes] = React.useState([{ key: 'first', title: 'First' }]);

  const renderScene = SceneMap({
    first: FirstRoute,
  });

  return (
    <TabView
      lazy
      navigationState={{ index, routes }}
      renderScene={renderScene}
      onIndexChange={setIndex}
      initialLayout={initialLayout}
    />
  );
};

export default () => {
  const [index, setIndex] = React.useState(0);
  const [routes] = React.useState([
    { key: 'a', title: 'a' },
    { key: 'b', title: 'b' },
    { key: 'c', title: 'c' },
  ]);

  const renderScene = SceneMap({
    a: InnerTab,
    b: InnerTab,
    c: InnerTab,
  });

  return (
    <TabView
      lazy
      navigationState={{ index, routes }}
      renderScene={renderScene}
      onIndexChange={setIndex}
      initialLayout={initialLayout}
    />
  );
};

const styles = StyleSheet.create({
  scene: {
    flex: 1,
  },
});
```


When swiping rapidly in the first or last tab app crashed. After running `cancel()` on UI thread it stopped crashing. Also made sure that the Example app still works correctly.
  • Loading branch information
jakub-gonet authored Aug 21, 2020
1 parent a2f2186 commit f8df91b
Showing 1 changed file with 9 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import android.view.MotionEvent;
import android.view.View;

import com.facebook.react.bridge.UiThreadUtil;

import java.util.Arrays;

public class GestureHandler<T extends GestureHandler> {
Expand Down Expand Up @@ -104,7 +106,12 @@ public T setEnabled(boolean enabled) {
if (mView != null) {
// If view is set then handler is in "active" state. In that case we want to "cancel" handler
// when it changes enabled state so that it gets cleared from the orchestrator
cancel();
UiThreadUtil.runOnUiThread(new Runnable() {
@Override
public void run() {
cancel();
}
});
}
mEnabled = enabled;
return (T) this;
Expand Down Expand Up @@ -326,6 +333,7 @@ public final void handle(MotionEvent origEvent) {
}

private void moveToState(int newState) {
UiThreadUtil.assertOnUiThread();
if (mState == newState) {
return;
}
Expand Down

0 comments on commit f8df91b

Please sign in to comment.