Skip to content

Commit

Permalink
Fix ConcurrentModificationException. (#2750)
Browse files Browse the repository at this point in the history
## Description

A while ago we decided to remove limit of gesture handlers on android (#2672). We missed one thing - `awaitngHandlers` can be modified while iterating over its items (inside [this loop](https://github.com/software-mansion/react-native-gesture-handler/blob/2e5df1c9a8595929b9879dc1246a7fd78de0549a/android/src/main/java/com/swmansion/gesturehandler/core/GestureHandlerOrchestrator.kt#L105)). This resulted in `ConcurrentModificationException`. 

This PR changes logic so that now we iterate over copy of `awaitingHandlers`, avoiding modification of collection that we iterate through.

Even though it looks like it changes a lot, the only difference beside adding copy is changing 

```jsx
for(otherHandler in currentlyAwaitingHandlers){
    if (shouldHandlerWaitForOther(otherHandler, handler)) {
        ...
    }
}
```

into

```jsx
for(otherHandler in currentlyAwaitingHandlers){
    if (!shouldHandlerWaitForOther(otherHandler, handler)) {
        continue
    }
    ...
}
```

to remove one level of nested `ifs`.

## Test plan

<details>
<summary> Modified code from #2739 </summary>

```jsx
import React from 'react';
import { ScrollView, StyleSheet, Text, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Animated from 'react-native-reanimated';

const Showcase = () => {
  const [lastActions, setLastActions] = React.useState([]);

  const addAction = (action) => {
    setLastActions((actions) => {
      const date = new Date();
      const time = date.toLocaleTimeString();

      if (actions.length >= 3) {
        actions.pop();
      }
      return [action + ': ' + time, ...actions];
    });
  };

  const longPressGesture = Gesture.LongPress()
    .onStart(() => {
      addAction('long press');
    })
    .runOnJS(true);

  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      addAction('double tap');
    })
    .runOnJS(true);

  const gesture = Gesture.Race(doubleTapGesture, longPressGesture);

  return (
    <View
      style={{
        flex: 1,
        backgroundColor: 'white',
        marginHorizontal: 20,
        marginVertical: 60,
      }}>
      <ScrollView contentContainerStyle={{ gap: 20 }}>
        <GestureDetector gesture={gesture}>
          <Animated.View style={styles.outer}>
            <Blue onAddAction={addAction} />
          </Animated.View>
        </GestureDetector>
        <View>
          {lastActions.map((action, index) => (
            <Text key={index}>
              {index} - {action}
            </Text>
          ))}
        </View>
        <View style={{ gap: 20 }}>
          <Text>
            <Bold>DoubleTap</Bold>: on all rectangles - should print "double
            tap" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>LongPress</Bold>: on all rectangles - should print "long
            press" - no tap gesture should be recognized ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on red - should do nothing ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on blue - should print "Blue: tap" ✅
          </Text>
          <Text>
            <Bold>Tap</Bold>: on orange - should print "Orange: tap" ⛔️ -
            Crashes on Android
          </Text>
        </View>
      </ScrollView>
    </View>
  );
};

export default Showcase;

const Blue = ({ onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Blue: tap');
    })
    .runOnJS(true);
  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      onAddAction('double tap');
    })
    .runOnJS(true);

  const composedGesture = Gesture.Exclusive(doubleTapGesture, tapGesture);

  return (
    <GestureDetector gesture={composedGesture}>
      <Animated.View style={styles.blue}>
        <Orange onAddAction={onAddAction} />
      </Animated.View>
    </GestureDetector>
  );
};

const Orange = ({ onAddAction }) => {
  const tapGesture = Gesture.Tap()
    .onStart(() => {
      onAddAction('Orange: tap');
    })
    .runOnJS(true);

  const doubleTapGesture = Gesture.Tap()
    .numberOfTaps(2)
    .onStart(() => {
      onAddAction('double tap');
    })
    .runOnJS(true);

  return (
    <GestureDetector gesture={Gesture.Exclusive(doubleTapGesture, tapGesture)}>
      <Animated.View style={styles.orange}></Animated.View>
    </GestureDetector>
  );
};

const Bold = ({ children }) => {
  return <Text style={{ fontWeight: 'bold' }}>{children}</Text>;
};

const styles = StyleSheet.create({
  outer: {
    padding: 20,
    backgroundColor: 'red',
    width: 300,
    height: 200,
  },
  blue: {
    width: '80%',
    height: '80%',
    backgroundColor: 'blue',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  orange: {
    margin: 5,
    width: '50%',
    height: '50%',
    backgroundColor: 'orange',
  },
});

```

</details>

<details>
<summary> Smaller example </summary>

```jsx
import React from 'react';
import { StyleSheet, View } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';

export default function App() {
  const t3 = Gesture.Tap()
    .numberOfTaps(3)
    .onEnd(() => console.log('t3'));
  const t1_1 = Gesture.Tap().onEnd(() => console.log('t1_1'));
  const t1_2 = Gesture.Tap().onEnd(() => console.log('t1_2'));

  const g = Gesture.Exclusive(t3, t1_1, t1_2);

  return (
    <GestureDetector gesture={g}>
      <View style={styles.box} />
    </GestureDetector>
  );
}

const styles = StyleSheet.create({
  box: {
    width: 250,
    height: 250,
    backgroundColor: 'crimson',
  },
});

```
</details>
  • Loading branch information
m-bert authored Feb 9, 2024
1 parent 1704323 commit 1f4e47b
Showing 1 changed file with 25 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,33 @@ class GestureHandlerOrchestrator(
fun onHandlerStateChange(handler: GestureHandler<*>, newState: Int, prevState: Int) {
handlingChangeSemaphore += 1
if (isFinished(newState)) {
// We have to loop through copy in order to avoid modifying collection
// while iterating over its elements
val currentlyAwaitingHandlers = awaitingHandlers.toList()

// if there were handlers awaiting completion of this handler, we can trigger active state
for (otherHandler in awaitingHandlers) {
if (shouldHandlerWaitForOther(otherHandler, handler)) {
if (newState == GestureHandler.STATE_END) {
// gesture has ended, we need to kill the awaiting handler
otherHandler.cancel()
if (otherHandler.state == GestureHandler.STATE_END) {
// Handle edge case, where discrete gestures end immediately after activation thus
// their state is set to END and when the gesture they are waiting for activates they
// should be cancelled, however `cancel` was never sent as gestures were already in the END state.
// Send synthetic BEGAN -> CANCELLED to properly handle JS logic
otherHandler.dispatchStateChange(
GestureHandler.STATE_CANCELLED,
GestureHandler.STATE_BEGAN
)
}
otherHandler.isAwaiting = false
} else {
// gesture has failed recognition, we may try activating
tryActivate(otherHandler)
for (otherHandler in currentlyAwaitingHandlers) {
if (!shouldHandlerWaitForOther(otherHandler, handler)) {
continue
}

if (newState == GestureHandler.STATE_END) {
// gesture has ended, we need to kill the awaiting handler
otherHandler.cancel()
if (otherHandler.state == GestureHandler.STATE_END) {
// Handle edge case, where discrete gestures end immediately after activation thus
// their state is set to END and when the gesture they are waiting for activates they
// should be cancelled, however `cancel` was never sent as gestures were already in the END state.
// Send synthetic BEGAN -> CANCELLED to properly handle JS logic
otherHandler.dispatchStateChange(
GestureHandler.STATE_CANCELLED,
GestureHandler.STATE_BEGAN
)
}
otherHandler.isAwaiting = false
} else {
// gesture has failed recognition, we may try activating
tryActivate(otherHandler)
}
}
cleanupAwaitingHandlers()
Expand Down

0 comments on commit 1f4e47b

Please sign in to comment.