chrfalch
commented
2 years ago Thanks for the report, @damienherve! Is this something always happening in a specific scenario, or do you have a reproduction for this?
Closed damienherve closed 2 years ago
chrfalch
commented
2 years ago Thanks for the report, @damienherve! Is this something always happening in a specific scenario, or do you have a reproduction for this?
damienherve
commented
2 years ago The problem is only for iOS (Android is OK in debug and release) When we run the app in debug mode (on a real device or in the simulator) we have no problem.
The crash only occurs when we build the iOS app in release mode, and run it on a real device.
I don't have a reproduction for this issue, we only use the new Shadow element (which replace the old DropShadow).
In the first place, i thought that the problem was with the new Shadow implementation (since 0.1.118).So I downgraded to 0.1.119: the crash disappears but the view stacking issue (resolved in 0.1.120) is still present as expected.
The crash seems related to this PR: #392
damienherve
commented
2 years ago I investigated a bit more, and the problem occurs in a more specific case than i thought before:
We use skia to draw a shadow around cards, contained in a collapsible section list (we use react-native-collapsible for this purpose). The crash occurs when the section goes from collapsed to opened (the cards appear).
I don't know exactly what's happening here, it seems to work well in debug mode. react-native-collapsible is based on the Animated API, maybe we should investigate in that direction...
wcandillon
commented
2 years ago @chrfalch this may be worth trying again once we release the fix for the Skia value destructors?
laurens-lamberts
commented
2 years ago FYI; We experienced the same issue with 120 and 121, but also still on version 118. Reverting to v 0.1.115 brought our implementation back to a stable state, without regular Skia related crashes.
Looking forward to a fix for these crashes. If we can be of any help, like providing a reproduction example, please let us know!
wcandillon
commented
2 years ago @laurens-lamberts any reproducible example would be extremely helpful. Do you know also if the issue is on Android/iOS or both?
chrfalch
commented
2 years ago Yes, a simple reproduction would be awesome!
laurens-lamberts
commented
2 years ago During our testing this week it was very noticeable on iOS. I don't believe we saw that many crashes on Android.
I will try to make a reproducible example today. The crashes appeared to be quite random though. It seemed to me that after a certain amount of Skia renders, the app suddenly crashed (with a crashlog mentioning the RNSkDrawView performDraw function). Maybe this has something to do with the new Shadow api? We use that one extensively since v118.
The crashes never really happened at the same screen or event in our app. Probably when a mayor part of our app context changed, and many Skia canvasses had to render simultaneously, the app started crashing.
If I come up with an example consistently crashing, I will let you know.
wcandillon
commented
2 years ago @laurens-lamberts our new release (which should be published today) contains a substantial memory management fix which based on your description might help (of course to be confirmed). Is anything special you are doing with Shadows? Are you using box shadows? inner shadows?
laurens-lamberts
commented
2 years ago Looking forward to that! I will definitely test it as soon as it is released.
Below is our custom 'Shadow' JSX component, for which we started using the Skia Shadow api from v118. We use this custom component as a wrapper around components we would like to have a drop shadow.
With v115 we use about the same component as below, but there we drawn our own shadows with a Blur inside Paint, next to a RoundedRect. This runs stable.
I believe there could be about 40 shadow renderings like below rendered at the same time in our app currently.
<View
onLayout={(event) => {
if (isMeasured) return;
var { width: layoutWidth } = event.nativeEvent.layout;
if (layoutWidth <= 0) return;
setWidth(layoutWidth);
setIsMeasured(true);
}}
pointerEvents="none"
>
{isMeasured && (
<Canvas
style={{
height: HEIGHT,
left: -10,
right: -10,
position: 'absolute',
bottom: position === 'bottom' ? -28 - offset : undefined,
top: position === 'top' ? -HEIGHT - offset : undefined,
opacity: 0.7,
}}
>
<RoundedRect
x={36}
y={position === 'bottom' ? 0 : HEIGHT}
width={(width ?? 30) - 50}
height={30}
r={radius}
color="#fff"
>
<Shadow dx={0} dy={-10} blur={10} color={'rgba(11,34,46,0.20)'} shadowOnly />
</RoundedRect>
</Canvas>
)}
{children}
</View>The crash occurs in approximately the same case that @laurens-lamberts:
The crash always occurs when we open a list of cards wrapped with the new Shadow element (previously, we used DropShadow).
Will try the new release too when available
damienherve
commented
2 years ago More testing using a Card component wrapped with a Shadow element similar to the one used by @laurens-lamberts
The problem seems related to Animated so...
wcandillon
commented
2 years ago @laurens-lamberts can you try 0.1.122 alpha and let us if you are still experiencing issues?
damienherve
commented
2 years ago Same crash with 0.1.122 with React Animated
wcandillon
commented
2 years ago @damienherve could you make a PR with an example in the example app that reproduces the crash?
chrfalch
commented
2 years ago Would it be possible for us to get a simple and complete RN project that shows these errors? I'm really curious to find out what's going on here :)
A link to a Github repo shows this behaviour and nothing else (with React Animated) would be the best.
damienherve
commented
2 years ago I'll try to do this, the thing is you'll have to generate an AdHoc/Appstore profile to test the app in release mode
chrfalch
commented
2 years ago I'll try to do this, the thing is you'll have to generate an AdHoc/Appstore profile to test the app in release mode
I can create those manually on my side and test in release mode on my device - think that'll work if you create a project without any dependencies on a specific backend or anything.
laurens-lamberts
commented
2 years ago I just made an example in a blank project with reanimated & skia, that instantly crashes in release mode. I will upload it to Github and share it in a few minutes.
laurens-lamberts
commented
2 years ago Here you go; https://github.com/laurens-lamberts/react-native-skia-crash
I just tested the same example without the Animated view surrounding the Skia canvasses, and did not experience crashes on release mode with that setup. Really looks like an issue when Skia is used in combination with reanimated.
Just press the button in the example twice, and the app should crash. Hope this helps!
Edit; Just read that @damienherve had the issue with the core react-native Animated library only, but this example project also shows issues when using reanimated v2...
Edit 2; Reanimated in this example project is configured for iOS only.
chrfalch
commented
2 years ago Thanks for helping out, I'm working on this one now.
damienherve
commented
2 years ago Edit; Just read that @damienherve had the issue with the core react-native Animated library only, but this example project also shows issues when using reanimated v2...
Edit; I implemented the complete solution, and i have the problem with Reanimated & Animated too. Can't understand it worked first on a simple example, and then not...
chrfalch
commented
2 years ago Still working on solving this issue. It is a tough one…
thespacemanatee
commented
2 years ago I'm getting a few similar crashes logged on Sentry as well, each time with slightly different causes but they were all caused by Skia library. Also all on iOS and Release builds, version 0.1.120 - 0.1.122.
Logs: https://pastebin.com/0LLe0ryn https://pastebin.com/kJkLfNXW https://pastebin.com/hvyMZpD3 https://pastebin.com/GjUsRuyy
chrfalch
commented
2 years ago Would it be possible to test out #435 and see if the error is reproducible? More explanation on what's done in the PR (still in draft mode).
laurens-lamberts
commented
2 years ago Definitely!
~~Can I install the commit directly into my project as a library like below, or do I need to take other steps building the library?
"@shopify/react-native-skia": "https://github.com/Shopify/react-native-skia#a9301c36d416b99e9c5740033e93b09db87299a1",~~
Edit; I guess i'll have to build your branch locally using build-skia-ios, and refer to my local instance from the other project package.json. I will give that a try!
laurens-lamberts
commented
2 years ago @chrfalch, I'm trying to build the Skia library using your latest branch, following the 'Building' steps in the ReadMe, but I stumble upon the following error directly at the beginning of the build;
laurenslamberts@MBP-LaurensLamberts-269 react-native-skia % yarn build-skia
yarn run v1.22.17
warning ../package.json: No license field
$ yarn build-skia-ios && yarn build-skia-android
warning ../package.json: No license field
$ ts-node ./scripts/build-skia-ios.ts && yarn build-skia-ios-framework
Building skia for iOS...
[iOS]: warning ../package.json: No license field
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
laurenslamberts@MBP-LaurensLamberts-269 react-native-skia % I've got Ninja, XCode, CMake installed, and performed the preparation steps installing packages. Any idea what could be wrong here?
wcandillon
commented
2 years ago This was no expected to work, you can try this package: https://firebasestorage.googleapis.com/v0/b/react-native-ui-kits.appspot.com/o/shopify-react-native-skia-0.1.42.tgz?alt=media&token=04b387f6-9047-4b1a-96e2-36103774c1b2
laurens-lamberts
commented
2 years ago @wcandillon, thanks, I will give it a try in both my crash-example project and our larger client-solution project.
laurens-lamberts
commented
2 years ago I keep getting this iOS build error using your URL as a reference to the react-native-skia package in package.json;
I already installed pods, and cleaned the build folder. Any idea how to fix it?
chrfalch
commented
2 years ago We'll try to make another package that fixes this! Sorry!!
wcandillon
commented
2 years ago it looks like something when wrong when building the package, can you try this one: https://firebasestorage.googleapis.com/v0/b/react-native-ui-kits.appspot.com/o/shopify-react-native-skia-0.1.42.tgz?alt=media&token=6d2c37d4-fd16-4057-84b8-e97e63b3828d
laurens-lamberts
commented
2 years ago Thanks, will do!
laurens-lamberts
commented
2 years ago Awesome! No more crashes in the example project I shared (iOS, release mode). I will test it with our larger project, and let you know the results in a bit.
chrfalch
commented
2 years ago I’m so happy to hear! Thanks for testing it!!!
laurens-lamberts
commented
2 years ago Testing update;
If I really stress the example project I made (spamming the show-hide button), I do get a (different) crash eventually;
Just so you know; this was me purposely trying to crash the example, including heavy operations. I don't think this will likely appear in a real-world project.
Edit; just noticed this is actually the react-bridge dying. Don't think this is something that can be prevented.
laurens-lamberts
commented
2 years ago Looks like I found an issue with this version though, in combination with react navigation. When I navigate to another (native) screen, and return to where I came from, the Skia canvasses are gone. This didn't happen in previous versions.
chrfalch
commented
2 years ago Will test, thanks for thoroughly testing this!
chrfalch
commented
2 years ago Testing update; If I really stress the example project I made (spamming the show-hide button), I do get a (different) crash eventually;
Just so you know; this was me purposely trying to crash the example, including heavy operations. I don't think this will likely appear in a real-world project.
Edit; just noticed this is actually the react-bridge dying. Don't think this is something that can be prevented.
This seems to be caused by providing invalid color values to Skia - I got them a few places myself. Seems like #6bdb is missing a number? Is this something you recognize @wcandillon ?
chrfalch
commented
2 years ago I've made a new fix (also fixing the color issue) for the navigation issue you saw - now kindly provided by @wcandillon as a temporary package here: https://firebasestorage.googleapis.com/v0/b/react-native-ui-kits.appspot.com/o/shopify-react-native-skia-0.1.43.tgz?alt=media&token=7a7da248-e206-4174-b856-14dfd54481fb
laurens-lamberts
commented
2 years ago Great! I will test it immediately.
laurens-lamberts
commented
2 years ago Both issues are fixed! I can't get it to crash anymore, even when pushing it to the limits. Thanks a lot for fixing this!
I will test Android as well, and let you know in a bit.
chrfalch
commented
2 years ago Wow! Awesome :) Thanks! Just checking out some potential memory leaks on Android now after this refactor.
laurens-lamberts
commented
2 years ago Sounds good!
I guess I just found an Android issue in debug mode.
When I start the app in debug mode from a closed state, I get to see all my Skia images nicely rendered on Android. As soon as I reload the JS bundle without a force-close of the app, all images are gone and won't appear within the app's lifecycle. A new force-close and app start, makes the images appear again.
chrfalch
commented
2 years ago Not sure if I understand correctly regarding force-close?
laurens-lamberts
commented
2 years ago Force-close as in opening the multitask menu of the Android phone, and closing the app. So that next time you open the app, it is not retrieved from the background, but started fresh.
chrfalch
commented
2 years ago Yeah, this one is to reproduce, looking into it now!
chrfalch
commented
2 years ago Think that one should be fixed, was me being a bit eager to remove a few things that shouldn't have been removed. @wcandillon has once again helped out building a temporary package:
chrfalch
commented
2 years ago Release 0.1.123 alpha contains the fixes tested in the issue, please make sure it works as expected and then we'll close this issue!
Thanks for the patience :)
damienherve
commented
2 years ago I just tested the last release on iOS (debug & release mode) and it seems to work perfectly well !
Thanks for the big work guys
Hi,
The fix applied in version 0.1.120 seems to work pretty well for the piling of views, at least in debug :)
We have a crash of the app, only in release, here is the Sentry log:
I tried to rollback to 0.1.119, no more crash.