Android crash: This dynamic value has been recycled #806
Comments
|
Hi @janicduplessis, I'm also seeing a fair amount of crash reports with a similar error since updating to |
|
I wonder if it could be related to this: c39b326 But, it seems more like the ViewManagerPropertyUpdater uses an incorrect FallbackShadowNodeSetter to set the fill property, somehow it seems to confuse the fill property with a MutableYogaValue and calls LayoutShadowNode.setPositionValues on it, which leads to this line: https://github.com/facebook/react-native/blob/0.57-stable/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java#L65 It seems to be missing a check for the number type before calling dynamic.asDouble() else if (dynamic.getType() == ReadableType.Number) {
unit = YogaUnit.POINT;
value = PixelUtil.toPixelFromDIP(dynamic.asDouble());
}I wonder why it doesn't use the setter from the view manager: and the shadow node: Tricky to analyse without a reproduction. |
|
Currently, the Views on Android are only used to get access to the shadow nodes for native animation: https://github.com/react-native-community/react-native-svg/blob/master/android/src/main/java/com/horcrux/svg/RenderableView.java#L8 |
|
Actually I think there might be 2 separate issues. I linked the wrong stacktrace in the initial issue. Here’s the one that seems to be related to the race condition I was talking about. |
|
@Almouro 7.0.3 |
|
Thanks for this, could you try with the latest commit from the master branch? |
|
Could you post the code or a small reproduction that can produce it? I'm not sure how the dynamic value could be recycled, perhaps it isn't even a string? At least react-native-svg doesn't make any recycle calls on any dynamic values. |
|
Here's the code for the component https://gist.github.com/janicduplessis/87a859eda656806bd2dea0db0a8940f1 I'm planning a new release in the next few days, I will update to master. |
|
Actually I think the first crash is related to changes I made locally to try to fix the 2nd stack trace I linked. I'll update the issue to only include the |
janicduplessis
changed the title
|
I've been testing this code for some time now and haven't been able to observe any exceptions. Not sure how to trigger it: // @flow
import * as React from "react";
import { Button, StyleSheet, View, Animated } from "react-native";
import Svg, { Line, Circle } from "react-native-svg";
const AnimatedLine = Animated.createAnimatedComponent(Line);
const AnimatedCircle = Animated.createAnimatedComponent(Circle);
type Props = {
style?: any,
backgroundIsTurquoise?: boolean,
};
type State = {
anim: Animated.Value,
};
class Loading extends React.Component<Props, State> {
state = {
anim: new Animated.Value(0),
hide: false,
};
componentDidMount() {
Animated.loop(
Animated.timing(this.state.anim, {
duration: 2000,
toValue: 1,
isInteraction: false,
useNativeDriver: true,
}),
).start();
}
render() {
const opacity = this.state.anim.interpolate({
inputRange: [0, 0.93, 1],
outputRange: [1, 1, 0],
});
const yellowBarOffset = this.state.anim.interpolate({
inputRange: [0, 0.2, 1],
outputRange: [22, 0, 0],
});
const greenBarOffset = this.state.anim.interpolate({
inputRange: [0, 0.19, 0.4, 1],
outputRange: [30.01, 30.01, 0, 0],
});
const pinkBarOffset = this.state.anim.interpolate({
inputRange: [0, 0.4, 0.78, 1],
outputRange: [107, 107, 50, 50],
});
const secondLineColor = this.props.backgroundIsTurquoise
? "white"
: "turquoise";
const { hide } = this.state;
return (
<View>
<Button
title="Toggle"
onPress={() => this.setState(({ hide }) => ({ hide: !hide }))}
/>
<Animated.View style={[{ opacity }, this.props.style]}>
{hide ? null : (
<Svg width={70} height={70}>
<AnimatedLine
stroke={secondLineColor}
strokeWidth="8"
strokeLinecap="round"
strokeDasharray={[30]}
strokeDashoffset={greenBarOffset}
x1="48"
y1="15"
x2="29"
y2="34.2"
/>
<AnimatedCircle
fill="transparent"
stroke="pink"
strokeWidth="8"
strokeLinecap="round"
strokeDasharray={[107]}
strokeDashoffset={pinkBarOffset}
rotation="-127"
origin="36, 44"
cx="36"
cy="44"
r="12"
/>
<AnimatedLine
stroke="yellow"
strokeWidth="8.5"
strokeLinecap="round"
strokeDasharray={[22]}
strokeDashoffset={yellowBarOffset}
x1="26"
y1="14.5"
x2="48"
y2="14.5"
/>
</Svg>
)}
</Animated.View>
</View>
);
}
}
export default class App extends React.Component {
render() {
return (
<View style={styles.container}>
<Loading />
</View>
);
}
}
const styles = StyleSheet.create({
container: {
flex: 1,
flexDirection: "column",
backgroundColor: "beige",
},
}); |
|
@msand Thanks for trying it out, I can't reproduce the issue neither on my end and it seems to happen very rarely. Hopefully we can figure something out from the stacktrace or other people hit this crash too. |
|
Hi guys, Yesterday got ~1k crashes quite similar for about ~20k users on the app yesterday. Crashes we are getting are usually:
These are new bugs in our last release that included an upgrade (among other things of course) of All of those bugs seem to touch the Investigating with Android Studio debugger by setting breakpoints in
I also saw that So it seems like the issue could be:
I can try downgrading What do you guys think? Where is |
|
I guess it's related to the native animation driver when it's setting the animated properties, I think it runs on the main ui thread. It's quite likely that the PR would fix it, would be great if you can test it :+1 |
|
@msand I confirm that this issue has disappeared 🎉 for those 10% including the PR on React Native (facebook/react-native#17842) Our biggest crash on that newest release is now: I can open a new issue for it, I don't think it's related |
|
@Almouro Great! Thanks for the info. |
|
@janicduplessis Could you champion getting that PR merged for 0.58? or 0.57.4? |
|
@msand Sure, I was about to also ship the PR to test if it fixes the issue but it seems like it does :) |
|
Native Animated does not work with layout props so its probably just those ones that use Dynamic. |
|
@janicduplessis Are there any drawbacks to using Dynamic? There is at least some performance penalty, because of the branching on type in the view property setters, and wrapping the JavaOnlyMap in a DynamicFromMap. But other than that, I can't see any significant differences between Dynamic and otherwise. At least from a quick reading of: |
|
@msand AFAIK Dynamic was implemented to support props with multiple possible types like for example |
|
@Almouro Can you try with the latest commit from the master branch? I've simplified the dynamic property setters in android and improved the null handling logic. |
|
There's a potentially significant optimisation opportunity in the property handling, for any props which currently take a string to allow for units. There's a redundant round-trip from a double to a string back to a double, when animating the properties (or whenever the given value is a double / Number). The common case is probably a unit-less number anyway, so it would make sense to optimise for that. To maintain spec-conformance, it would require adding an enum for the units, another field signifying the unit for each property, and, that the property setters would handle the case when the value is a string, such that it sets the unit correctly. And finally, the relativeOnWidth/Height/Other (css units to pixels resolvers) would need to accept a number and a enum unit instead of a string. |
|
Another optimisation opportunity would be to rename the fields such that the propertyNameToFieldName could be removed. |
|
Also, as @janicduplessis wrote. The shadow nodes could be removed, by creating separate View classes and moving the logic from the shadow nodes there instead. Because the SvgViewManager returns true for needsCustomLayoutForChildren anyway, and we're already creating a View for each element (to allow for animation). So it should actually conserve memory, and possibly improve performance as well. Or are there any benefits to having the shadow nodes I'm unaware of? As we're not using yoga for layout (except for the root svg element position and dimension) they should just be redundant, right? |
|
At least currently, having the shadow nodes causes an extra de-referencing for each time a property is set using the ViewManagers, and we keep a private final SparseArray<T> mTagToShadowNode = new SparseArray<>();to get a reference to it (costing memory and accounting on View creation/destruction), which wouldn't be needed if the logic was simply inside the Views instead. |
|
I now have a branch where I've moved the logic into the Views and almost completely removed the ShadowNodes. 0bdca66 |
|
@msageryd Could you try your app with the latest commit from master and the one from the plain-views branch to see if there's any significant changes? https://github.com/react-native-community/react-native-svg/compare/plain-views |
|
@msand I opened a new issue for the new bug. I don't think it has to do with this issue anymore :) |
|
@msand I think removing shadow nodes on Android makes sense, iOS already uses views only. |
|
The current latest commit in the plain-views branch seems to work in all tests I have quickly testable. Don't see any reasons not to cut another release at this point. Unless you know of any regressions? |
|
@janicduplessis Would it be possible to get that PR cherrypicked for v0.57.4? Seems it should probably be the recommended minimum for any version of react-native-svg using Dynamic. And the plain views branch should probably become the recommended v7 version, to eliminate the null exceptions. |
|
I've implemented the optimizations in the SVGLength branch now: https://github.com/react-native-community/react-native-svg/compare/SVGLength Removes redundant round-trip (double > string > double) when animating. |
|
The css value resolvers could still be made to get the canvas width/height and font-size only when needed, instead of every time. |
|
I've fixed all known remaining regressions from the rewrite now: https://github.com/react-native-community/react-native-svg/compare/SVGLength |
|
@msand I can post in the release repo to get it included in the next minor release if there is one. Nice! Is that on npm? I'm planning a release of my app soon so I can test it. |
|
@janicduplessis Excellent! Now it is, v7.2.0 just published 🎉 https://www.npmjs.com/package/react-native-svg/v/7.2.0 |
|
@msand Sorry for being late to the game. Every performance optimisation is great news to me. Unfortunately I ran into a wall here. I don't know where to start searching. This is what I get when I mount the "drawing view" in my app using RNSVG 7.2 I have searched through my code in the hope to find any place where I have used strings instead of numbers. I only found one place, but changing this to numbers didn't fix the problem. The reason I have one place where I'm using strings is that width and height of Here is my previous workaround in order to get Do you have any clues of what to look for? |
|
I have dug into this. I managed to isolate the problem to a very simple line drawing. This works: This does not work: The coordinates must be strings to work. The strokeWidth can be numerical, though. I also got the below warning:
This is how the properties of my Svg element is set: |
|
@msageryd Hmm, both those examples (Lines) work fine for me in v7.2.0 |
|
I've made the Svg element return a G element and set some of the styles and props from the root element on it, to allow inheritance of values without another significant refactor, should perhaps omit a bit more of the properties, but don't know of any actual issues that could result from it as of now. So, all Svg content currently have an implicit G element in the root. |
|
@msageryd Regarding numbers and strings, I recommend using numbers, but both should work, it's just that numbers are cheaper with regards to memory, cpu and energy. Much of the recent work has been to optimize for the case when the arguments are plain numbers rather than string encoded. |
|
@msageryd From the error message, it seems like these two lines would be missing from the code you've built: https://github.com/react-native-community/react-native-svg/blob/75204047c9424c591ed42d9763d6547709cf2b8b/ios/ViewManagers/RNSVGNodeManager.m#L176-L177 |
|
@msageryd perhaps you were using the plain-views branch? I tested in the master branch (essentially the SVGLength branch + some minor fixes, which builds on the plain-views one). |
|
Good news, the fix will make it in the next rn minor react-native-community/releases#48 (comment) |
|
@msand I'm seeing a visual issue with the component I sent you earlier (https://gist.github.com/janicduplessis/87a859eda656806bd2dea0db0a8940f1) on iOS 8.0.0, haven't tested android yet. |
|
Thanks for reporting, will look at it later tonight once I get back to a computer |
|
@msand Thanks!! |
|
@janicduplessis I think I've fixed the issue, really wouldn't expect this to be necessary changes to the code. But oh well, yet another day to learn about obj-c/ios intricacies. e576813 |
|
@msand Awesome, thanks! I just tested master and everything works fine. |
|
i have the same problem sometimes,but after reloading the react native app the crash will disappear .I don't know Why? |
@janicduplessis this bug(com.facebook.react.bridge.ReadableNativeArray cannot be cast to java.lang.Double ) is solved ? I have the same problem |
|
@janicduplessis And my react native version is 0.56.0, if I upgrade the version from my current version to 0.57.4, I can solve this bug(ReadableNativeArray cannot be cast to java.lang.Double) |
|
This should be fixed in the latest versions, closing the issue now. |
I'm seeing this crash log on Android in production since updating to ^7.0.0. I'm not sure how to reproduce locally but it comes a svg component that is animated using native animated. However I can't reproduce the crash reliably so there must be some kind of race condition, it happens rather rarely, ~20 instances in the past week for a few thousand users.
I noticed the implementation uses shadow nodes to save the svg data but if I understand correctly it could just be normal uithread view manager props. Shadow nodes run on a different thread so it might be what causes this race condition.
Here's the full stacktrace if that can be useful, the interesting part is the inner exception.
The text was updated successfully, but these errors were encountered: