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(Android): move calls to start transition to UI thread to prevent possible crash in prod #2532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion android/src/main/java/com/swmansion/rnscreens/Screen.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import android.annotation.SuppressLint
import android.content.pm.ActivityInfo
import android.graphics.Paint
import android.os.Parcelable
import android.util.Log
import android.util.SparseArray
import android.view.View
import android.view.ViewGroup
Expand All @@ -16,6 +17,7 @@ import androidx.fragment.app.Fragment
import androidx.swiperefreshlayout.widget.SwipeRefreshLayout
import com.facebook.react.bridge.GuardedRunnable
import com.facebook.react.bridge.ReactContext
import com.facebook.react.modules.core.ReactChoreographer
import com.facebook.react.uimanager.PixelUtil
import com.facebook.react.uimanager.ReactClippingViewGroup
import com.facebook.react.uimanager.UIManagerHelper
Expand Down Expand Up @@ -59,6 +61,8 @@ class Screen(
var screenOrientation: Int? = null
private set
var isStatusBarAnimated: Boolean? = null

@Volatile
var isBeingRemoved = false

// Props for controlling modal presentation
Expand Down Expand Up @@ -374,9 +378,32 @@ class Screen(
var nativeBackButtonDismissalEnabled: Boolean = true

fun startRemovalTransition() {
Log.w(TAG, "$this startRemovalTransition")
// isBeingRemoved is marked as volatile to ensure memory ordering.
// Synchronization is not required, because this method is called either from commit hook
// running on JS thread (before mounting) or from mounting code running on UI thread.
//
// We need to run this on UI thread due to assertions in RN code (otherwise we could crash
// the app in production env).
//
// Important thing is timing here: when we start transition from commit hook we rely on the fact
// that the mount items are not scheduled on UI yet, thus we rely on the fact that the task scheduled
// here will be executed before the mount items as the queues are serial.
//
// TODO: Verify whether this method should be called from view manager at all (or maybe only on Paper)
if (!isBeingRemoved) {
isBeingRemoved = true
startTransitionRecursive(this)
if (!reactContext.isOnUiQueueThread) {
Log.w(TAG, "$this startRemovalTransition schedule")
ReactChoreographer.getInstance().postFrameCallback(ReactChoreographer.CallbackType.DISPATCH_UI) {
Log.w(TAG, "$this startRemovalTransition exec")
startTransitionRecursive(this)
}
} else {
Log.w(TAG, "$this startRemovalTransition sync")
startTransitionRecursive(this)
}
// startTransitionRecursive(this)
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.swmansion.rnscreens

import android.util.Log
import android.view.View
import com.facebook.react.bridge.ReactApplicationContext
import com.facebook.react.module.annotations.ReactModule
Expand Down Expand Up @@ -46,6 +47,7 @@ class ScreenStackViewManager :
}

private fun prepareOutTransition(screen: Screen?) {
Log.w("StackViewManager", "$screen startRemovalTransition")
screen?.startRemovalTransition()
}

Expand Down
8 changes: 4 additions & 4 deletions apps/App.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import React from 'react';
import { enableFreeze } from 'react-native-screens';
import Example from './Example';
// import * as Test from './src/tests';
// import Example from './Example';
import * as Test from './src/tests';

enableFreeze(true);

export default function App() {
return <Example />;
// return <Test.Test42 />;
// return <Example />;
return <Test.Test2282 />;
}
5 changes: 3 additions & 2 deletions apps/src/tests/Test2282.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@ function Home({ navigation }: any) {
);
}

function ListScreen() {
function ListScreen({ navigation }: any) {
return (
<View
style={{ flex: 1, backgroundColor: 'slateblue' }}
removeClippedSubviews>
<ParentFlatlist />
<View removeClippedSubviews>
<View style={{ backgroundColor: 'pink', width: '100%', height: 50 }} />
<Button title='Go back' onPress={() => navigation.goBack()} />
</View>
<ParentFlatlist horizontal />
</View>
Expand Down Expand Up @@ -140,7 +141,7 @@ export default function App(): React.JSX.Element {
<NavigationContainer>
<Stack.Navigator screenOptions={{ animation: 'slide_from_right' }}>
<Stack.Screen name="Home" component={Home} />
<Stack.Screen name="List" component={ListScreenSimplified}/>
<Stack.Screen name="List" component={ListScreen}/>
</Stack.Navigator>
</NavigationContainer>
);
Expand Down
Loading