Skip to content

Fix thread safety of RNGestureHandlerModule methods#1173

Merged
jgonet merged 1 commit intomasterfrom
@kuba/fix-detachHandler-thread-safety
Aug 24, 2020
Merged

Fix thread safety of RNGestureHandlerModule methods#1173
jgonet merged 1 commit intomasterfrom
@kuba/fix-detachHandler-thread-safety

Conversation

@jgonet
Copy link
Copy Markdown
Member

@jgonet jgonet 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
Copy Markdown
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.

👍

@jgonet jgonet changed the title Fix thread safety of detachHandler() Fix thread safety of RNGestureHandlerModule methods Aug 24, 2020
@jgonet jgonet merged commit 8e1174b into master Aug 24, 2020
@jgonet jgonet 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