Closed jmcginty closed 8 years ago
@jmcginty that definitely sounds like a difficult bug. I'm traveling this week, but may be able to find some time to help debug. Can you set up a private repo with the sources and add me as a collaborator?
Thanks @rozele, very much appreciated if you could look into it. I have created a public repository here: https://github.com/jmcginty/mobilepos I created a gif to show what steps to take to reproduce the bug.
I have been running in Visual Studio Debug\x86 mode . The crash happens on the last screen when the user clicks 'New Sale' which clears the cart and navigates back to the home screen. A second or so after arriving at the home screen, the application crashes.
I put a log inside the private void DropView(DependencyObject view)
method in C:\web\react-native\MobilePos\node_modules\react-native-windows\ReactWindows\ReactNative\UIManager\NativeViewHierarchyManager.cs
The log shows that the tag which cannot be found, has been Dropped a few calls previously.
Nice, thanks for the repro, taking a look now.
@jmcginty it looks like an issue with uses the remove layout animation. One of the views being removed recursively is also scheduled for removal by layout animation.
On the last view before the crash, there's an order summary view on the left. This view is assigned a layout animation when it is being removed. In a subsequent batched bridge operation, the parent container is dropped (and this order summary view is dropped recursively). Sometime after this operation, the layout animation completes and attempts to drop the order summary view (which was already dropped recursively) and causes the crash.
Does this work on Android? Conceptually, we are doing the exact same thing as Android in terms of managing layout animations. Namely, this here which calls DropView after an animation ends: https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/uimanager/NativeViewHierarchyManager.java#L420 (although it is possible that Android does not call OnAnimationEnd() if DropView() is called first)
We haven't been testing on Android, but I'll set it up and try.
@jmcginty any luck reproducing on Android?
Getting it running on android is taking longer than expected, I have some issues with Android studio which aren't anything to do with this project, but we are working on it.
@jmcginty I found that if you comment out this line of code: https://github.com/jmcginty/mobilepos/blob/4e45a893e27b5b7968b743351d56b6df46775410/js/screens/Sales.js#L21, the app no longer crashes.
You should be able to create a custom LayoutAnimation configuration that just prevents the "delete" animation, which will prevent the crash.
Turns out it's very easy to work around this so you can keep your easeInEaseOut behavior for your create
and update
operations, just replace:
componentWillUpdate() {
LayoutAnimation.easeInEaseOut();
}
with:
componentWillUpdate() {
LayoutAnimation.configureNext({
duration: 300,
create: {
type: LayoutAnimation.Types.easeInEaseOut,
property: LayoutAnimation.Properties.opacity,
},
update: {
type: LayoutAnimation.Types.easeInEaseOut,
}
});
}
It took longer than I thought, but I've got my android build up and running, and no crash.
Did you try the recommendation above on Windows?
I have updated to 0.37 of react-native and 0.37.1 of react-native-windows, and the latest version of our sources.
With the new version of react-native windows, there is a different error:
at System.Linq.Enumerable.Contains[TSource] (IEnumerable
1 source, TSource value, IEqualityComparer
1 comparer) at System.Linq.Enumerable.Contains[TSource](IEnumerable`1 source, TSource value) at ReactNative.UIManager.NativeViewHierarchyManager.ManageChildren(Int32 tag, Int32[] indexesToRemove, ViewAtIndex[] viewsToAdd, Int32[] tagsToDelete) at ReactNative.UIManager.UIViewOperationQueue.<>cDisplayClass28_0.b0() at ReactNative.UIManager.UIViewOperationQueue.<>cDisplayClass36_0. 0() at ReactNative.UIManager.UIViewOperationQueue.OnRendering(Object sender, Object e) at ReactNative.UIManager.UIViewOperationQueue.OnRenderingSafe(Object sender, Object e)b
I tried the workarounds, and the first suggestion - commenting out the animation works.
The second workaround does not help, and I get the same red box error.
There are two sources with the custom animation - PaymentEntry.js & Sales.js. Commenting both out hides the problem. I have updated the example code on git.
I have updated the sample repro again. It seems I have to comment out ALL animations in the application (there are three - Home.js, PaymentEntry.js, Sales.js. ) or it will still crash. Commenting out all animations is not a long term solution, this seems like a bug that will affect any non trivial application.
Thanks @jmcginty, taking a look
@jmcginty I'm really surprised that this is not a problem for Android, I've traced through the code multiple times and it appears that we're doing the exact same behavior (dropping a node from a parent, then dropping it from a layout animation).
@jmcginty can you try out the native code changes in #884 to see if this solves the crashing issue?
Hi @rozele, thanks for taking another look. It definitely does not crash on android or ios. I tried it again just now. Did you trace the android code? Is it being asked to drop views that have already been dropped? I am not sure how to debug into the android code.
I will try the change you have suggested, I think it will work.
No, haven't debugged. I need to re-read the docs on how to integrate react-native android source into an existing project, since android ships as a pre-built binary.
OK, I figured out how to put breakpoints inside the source inside the .jar file. If you do "Find in Path"->Scope->Directory "/Users/james/projects/react-native/mobilepos/node_modules/react-native/android/com/facebook/react/react-native/0.37.0/react-native-0.37.0-sources.jar!/com/facebook/react/" Text To Find: "NativeViewHierarchyManager" . In the search results, you can double click to open the NativeViewHierarchyManager.java file. There must be an easier way to do this, but that is how I did it.
I then put a breakpoint on the dropView method, to log the Id of every view being dropped.
There were no duplicates being dropped. I put the exact same log on the windows code, and it does get duplicates, and that is what's causing the crash.
Does this point to a bug in the Facebook.CSSLayout.dll ? I am stuggling to find the source for it on github. The official page looks out of date, the latest release is only 1.1 last November.
@jmcginty I don't think this has anything to do with css-layout. Views are dropped either by actions from JavaScript via the UIManager or from the LayoutAnimation manager. I'll do some of my own investigations in Android Studio as well.
@rozele It's a bit of a mystery to me where the calls come from, is there some facebook doco on the architecture/ process flow you know of? I just can't help feeling we should keep the android & c# code as similar as possible so that keeping up with changes from the android project is easier. Making this change in c# to not crash will work, but the root cause is elsewhere, I don't know enough to track it down though. I have the same problem in #711, which crashes but the stack trace is no help. I would love to find this bug and submit a pull request, as I can see no way to work around that one.
This error is thrown in my app when you do a certain combination of naviagtions. It is consistently reproducible, but I don't have small example. Our app is about 70+ js files. If someone is able to assist in debugging this, I can share the sources.
We are using React Native/ React Native Windows 0.36.0, but I have tried 0.37.0-rc.0 and get the same problem.
The same app doesn't crash on ios.
Here is the stack trace: '''
'''
From debugging what I can tell is a view is getting dropped twice, and it is crashing on the second DropView() call.