RCTImageLoader crashes when canceling image loads (RN 0.36) #10433
Comments
|
Thanks for submitting an issue. Can you update your original post and ensure all the fields required by the template are filled in? |
SunnyGurnani
changed the title
|
@hramos update the issue. Let me know if you need any more information. |
|
@SunnyGurnani you probably mean to have 0.36.rc.0 in the title instead of 0.30.rc.0? |
SunnyGurnani
changed the title
|
@anttimo thats correct I have changed the title |
|
This is also similar #10473 |
ide
changed the title
|
This issue has us still using 0.32. I'm not sure why this hasn't become a more pressing issue to fix. |
|
Same as @chandlervdw here. We were constantly updating to the newest version to try if it is fixed. Now that we need to release an update to our app, we downgraded back to React Native 0.31 (for us 0.32 crashes as well with a related network issue) to be able to release an update. That was a sad moment since we also had to downgrade a couple of other libraries. |
|
Same boat - this is a huge issue for us, getting hundreds of crash reports each week from it. I was realllly hoping not to have to got back to 0.31 because there's so many other good bug fixes in there - so I come here daily hoping for a miracle patch ;) |
|
Similar story here. We upgraded to 0.33 awhile back, and found this issue during testing. I assumed it would be addressed within the month or two before we were ready to release, but now that date it quickly approaching, and I really don't want to downgrade. Can this issue not be patched somehow, even something dirty? The entire app crashing is really not good... even if the images just didn't load, or didn't get cleaned up would be better. I'll have to learn some more objective-c to see how I can help. Maybe there's someone who can do a quick technical writeup for why this is actually happening. For example, what is trying to be achieved with the line: |
|
Is there a rnplay snippet that reproduces the crash? |
|
@hramos I created an app to reproduce the bug https://github.com/gitim/ListViewApp. To reproduce you need scroll intensively, sometimes the bug happens quickly, sometimes you need to scroll more, I think it depends on your network connection. |
|
I can also confirm that this is still crashing, even though the crash rate got much better after upgrading to version 0.36.0.rc.1. We upgraded our production app from 0.28.0 to 0.34.0 two weeks ago. After that, we experienced an enormous increase of crashes, so we decided to release a new app update with 0.36.rc.1 as soon as possible (hence the release candidate and not the stable version) as there were two promising commits in 0.35.0. and 0.36.0,rc.1. After evaluating the crash rate of this crash after a week on production, the crash rate was reduced by 88%, so it really helped a lot. But still, a proper fix would be amazing. |
|
Did anybody try to disable removeClippedSubviews of ListView, does it reduce crashes? |
|
@gitim yes it does. I also found that if the server has error or Not found for any image url in the list it increases these crashes even when removeClippedSubviews is disabled. So I would suggest to set one of the URL which responds as 4xx or 5xx |
|
I created a product pain. Please, vote to pay attention of the core team members to the bug. |
|
@gitim voted. |
|
Thanks @gitim, I'm trying to reproduce with your app right now. With Xcode's memory analyzer I can see a number of zombie NSCFURLSessionTask but I can't reliably repro the crash. |
|
I didn't find reliable way to reproduce, but it is definitely reproducible in this app. After about 3 minutes of attempts it crashed http://www.giphy.com/gifs/l0MYw9QKCyg2ESvsI. |
|
@gitim: one theory I'm trying to verify now. Could you try removing the The other thing I'm investigating is that when running your app in Instruments, it looks like all of the __NSCFLocalDataTask are actually being leaked. |
|
still crash in 0.39 :( |
|
It's still crash on 0.38 and 0.39 |
|
@aterribili Last fixes goes in 0.40 or 0.41, 0.39 and earlier will have the issue. |
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
Summary: This fixes a crash occurring [on this line](https://github.com/facebook/react-native/blob/f9f32eb426b5385e199ef6e1d2b610dfe20e60e7/Libraries/Image/RCTImageLoader.m#L253). It was reported in a comment in facebook#10433. The problem is that `task` is deallocated at this point and is unsafe to use. Removing it from `_pendingTasks` dropped its ref count to 0. [The ARC docs](http://clang.llvm.org/docs/AutomaticReferenceCounting.html#fast-enumeration-iteration-variables) state that, by default, loop variables in fast enumeration loops are not retained. That's why `task`'s ref count is 0. It's likely we ran into this bug because the code disobeyed the [reverseObjectEnumerator docs](https://developer.apple.com/reference/foundation/nsarray/1415734-reverseobjectenumerator) which state that "you must not modify the array during enumeration". The default retention policy for fast enumeration seems to assume you follow this. To fix this bug and avoid other potential pitfalls, the code now follows the docs and `_pendingTa Closes facebook#11296 Differential Revision: D4277167 Pulled By: javache fbshipit-source-id: 1211b32935bab7f4080dc00b88d85348786e859a
|
The fixes were merged before 0.40. My understanding is that there are still some crashes but they should be a lot less frequent. |
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
Summary: Fixes facebook#10433 The code didn't account for the fact that cancelLoad is set by two different threads. It gets set on the URL request queue when the request completes successfully and it gets set on the UI queue during cancelation. This oversight lead to a couple of different kinds of crashes. 1. Attempt to invoke a nil function -- We check that cancelLoad is non-nil and on the next line we call it. However, cancelLoad could have been set to nil by the other thread between the time it was verified to be non-nil and the time it was called. Consequently, the program will attempt to call nil and crash. 2. Block deallocated while it's executing -- Suppose cancelLoad points to a block, it is verified to be non-nil, and it is successfully invoked. In the middle of executing the block, cancelLoad, the last reference to the block, is set to nil. This causes the block to be deallocated and its captured values to be freed. However, the block continues executing and the next time it attempts to use a captured value Closes facebook#11145 Differential Revision: D4261499 Pulled By: javache fbshipit-source-id: 46424c6dd8cfa085cef32d945308de07798040bc
|
Linking a crash report incase this can provide further details http://crashes.to/s/ee7279ad0b8 ReactNative version |
|
This is happening frequently for many users any ideas what would be main cause for this. |
|
@myusuf3 What version of React Native are you seeing the crashes on? What callstack are you seeing? This information is important in order to be able to give you relevant suggestions. A couple of fixes (2342377 and 2aca021) went into React Native 0.41 for fixing image loader crashes. If you are using an older version of React Native, you can backport these 2 fixes into your version of React Native. If you are seeing these crashes on React Native 0.41 or newer then I don't have any tips for you. Somebody needs to investigate the remaining image loader crash more deeply. |
|
@rigdern we are on |
|
@myusuf3 Yes, 0.41 was released at the beginning of February. As described in this blog post, a new version of React Native is released at the beginning of each month. 0.43 should be released sometime near the beginning of this month. You can see the full list of releases at https://github.com/facebook/react-native/releases. The image loader crashes are race conditions and it can be challenging to get the conditions just right to reproduce them consistently. In general, they trigger when an image is canceled while it's in the middle of being loaded. |
|
@rigdern so do those commits fix the issue? We aren't able to reproduce but a significant chunk of users are hitting the issue. |
|
@myusuf3 Probably, those 2 commits fixed the most common image loader crashes. What's the callstack at the time of the crash? |
|
|
@myusuf3 I'm not sure if that particular crash is fixed by the 2 commits. Based on this comment, somebody else crashed with this stack as well but never reported back whether my fixes resolved his issue. |
|
@tianxind Your stacktrace is unrelated to the issue in this thread. Yours appears to be a crash in |
|
@mikelambert ok I will delete my post |
|
Oh man, this thing has been plaguing me for months now, since I started using React Native, and I finally found this thread! (Just posting to make sure this is what everyone else is getting? The crash seems to appear almost random/happens if you're unlucky when loading/scrolling down a list with images.) I'm on RN 0.37. Pulling in @rigdern 's two commits should do the trick?
Can someone tell me if these stack traces are related? I do not know how to read them. Instabug/Instabug-iOS#150 My first stack trace (in the screenshot) looks similar to #10433 (comment), but the other two (copy pasted) don't look like the ones posted in this thread. Wouldn't want to be bugging the good people over there for a React Native image/list problem. UPDATE: Posting screenshot here for convenience: |
|
@booboothefool The crash in your screenshot was fixed by 2aca021 which is included in React Native 0.41 and higher. |
|
Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally! If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:
If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution. |
|
We are still getting multiple crashes in production on v0.41:
|
and others
Image Loader Crashing at OSAtomicOr32Barrier (EXC_BAD_ACCESS)
Start of Stack is
Steps to Reproduce / Code Snippets
Put some Network Images on the Page (Specially in a List View) then scroll or change tabs so Images which are not loaded yet are out of the View Port and go back to same tab/viewport and you will see this error.
Expected Results
No Error
Additional Information
The text was updated successfully, but these errors were encountered: