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

Fix thread safety of RNGestureHandlerModule methods #1173

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

jakub-gonet
Copy link
Member

@jakub-gonet jakub-gonet commented Aug 22, 2020

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.

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>
  );
};

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jakub-gonet jakub-gonet changed the title Fix thread safety of detachHandler() Fix thread safety of RNGestureHandlerModule methods Aug 24, 2020
@jakub-gonet jakub-gonet merged commit 8e1174b into master Aug 24, 2020
@jakub-gonet jakub-gonet deleted the @kuba/fix-detachHandler-thread-safety branch August 24, 2020 12:58
braincore pushed a commit to braincore/react-native-gesture-handler that referenced this pull request Mar 4, 2021
…on#1173)

## Description

Fixes software-mansion#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 software-mansion#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>
  );
};
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants