Skip to content

Commit

Permalink
Fix thread safety of RNGestureHandlerModule methods (#1173)
Browse files Browse the repository at this point in the history
## Description

Fixes #1169.

Another instance of changing the handler state outside the UI thread. In this case, calling `handler.cancel()` which calls `mOrchestrator.onHandlerStateChange()` interfered with `scheduleFinishedHandlersCleanup()` method which calls `handler.reset()` and sets `mOrchestrator` to `null`. 

This may happen when threads interleave in the way described below:

1. `RNGestureHandlerModule.dropGestureHandler` is called (directly or in `attachGestureHandler()`; it's called on native modules thread)
2. RNGH dispatches first `onStateChange` event on UI thread
3. after dispatching event `mHandlingChangeSemaphore` is zero and call to `scheduleFinishedHandlersCleanup()` causes `cleanupFinishedHandlers()` to reset handlers and clear `mOrchestrator`
4. Native modules thread continues with execution and tries to call `moveToState` which crashes the app

This may show when `RNGestureHandlerModule.attachGestureHandler()` or `RNGestureHandlerModule.dropGestureHandler()` is rapidly called from JS (e.g. when frequently unmounting or updating handler component).

After checking the call hierarchy of `GestureHandler.moveToState()` it seems like this is the last occurrence of a native-module-calling-UI-only-method problem.

## Example

Fast clicking on the TapGestureHandler crashes with error "Expected to run on UI thread", introduced in #1171. After applying this patch, the error disappears.

```jsx
import React, { useState } from 'react';
import { View } from 'react-native';
import { TapGestureHandler } from 'react-native-gesture-handler';
export default () => {
  const [i, setI] = useState(0);
  const updateState = () => setI(i + 1);
  return (
    <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
      <TapGestureHandler onHandlerStateChange={updateState} key={i}>
        <View style={{ height: 100, width: 100, backgroundColor: 'pink' }} />
      </TapGestureHandler>
    </View>
  );
};
```
  • Loading branch information
jakub-gonet authored Aug 24, 2020
1 parent f8df91b commit 8e1174b
Showing 1 changed file with 8 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import android.util.SparseArray;
import android.view.View;

import com.facebook.react.bridge.UiThreadUtil;
import com.swmansion.gesturehandler.GestureHandler;
import com.swmansion.gesturehandler.GestureHandlerRegistry;

Expand Down Expand Up @@ -50,7 +51,7 @@ private synchronized void registerHandlerForViewWithTag(int viewTag, GestureHand
}
}

private synchronized void detachHandler(GestureHandler handler) {
private synchronized void detachHandler(final GestureHandler handler) {
Integer attachedToView = mAttachedTo.get(handler.getTag());
if (attachedToView != null) {
mAttachedTo.remove(handler.getTag());
Expand All @@ -66,7 +67,12 @@ private synchronized void detachHandler(GestureHandler handler) {
// Handler is in "prepared" state which means it is registered in the orchestrator and can
// receive touch events. This means that before we remove it from the registry we need to
// "cancel" it so that orchestrator does no longer keep a reference to it.
handler.cancel();
UiThreadUtil.runOnUiThread(new Runnable() {
@Override
public void run() {
handler.cancel();
}
});
}
}

Expand Down

0 comments on commit 8e1174b

Please sign in to comment.