react-native: RCTImageLoader crashes when canceling image loads (RN 0.36)

Image Loader Crashing at OSAtomicOr32Barrier (EXC_BAD_ACCESS)

Start of Stack is

- (void)didMoveToWindow
{
  [super didMoveToWindow];

  if (!self.window) {
    // Cancel loading the image if we've moved offscreen. In addition to helping
    // prioritise image requests that are actually on-screen, this removes
    // requests that have gotten "stuck" from the queue, unblocking other images
    // from loading.
    **[self cancelImageLoad];**
  } else if ([self shouldChangeImageSource]) {
    [self reloadImage];
  }
}

image

image

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

  • React Native version: Latest 0.36.rc.0
  • Platform(s) (iOS, Android, or both?): iOS
  • Operating System (macOS, Linux, or Windows?): macOS

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Reactions: 22
  • Comments: 106 (59 by maintainers)

Commits related to this issue

Most upvoted comments

We (G2i) are going to match @huitsing 's $250 bounty!! So it’s now at $500 total. That will buy you a very large turkey for your feast tomorrow (for those in the USA).

+18
gabegreenberg on Nov 23, 2016

Hey everyone, we (ZeeMee) are willing to pay a $250 bounty to anyone who fixes this bug, and gets their fix merged into master! Let’s kill this thing.

+17
huitsing on Nov 23, 2016

I created a product pain. Please, vote to pay attention of the core team members to the bug.

+15
gitim on Nov 3, 2016

@gabegreenberg Thanks for the offer but I decline the bounty. I worked on this issue just because it seemed to be affecting a number of people and it looked like an interesting bug. Let me know whether my PRs resolve the crash for you.

+14
rigdern on Dec 7, 2016

@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.

+13
gitim on Oct 28, 2016

I created a PR (https://github.com/facebook/react-native/pull/11145) which I believe fixes this bug. I can no longer repro the crash using the steps I described in my earlier comment. If anyone wants to test it, let me know if it resolves the crash for you.

+9
rigdern on Nov 26, 2016

Hey everyone, I’ve just put up https://github.com/javache/react-native/commit/fb2f5c50a912aa0dd4ea4551ac448d7b7e171940 which is a potential fix. I have not been able to consistently reproduce a crash, but I could find a significant amount of leaks in NSURLSessionTask which are usually indicating some state is messed up (and could cause crashes).

Summary of my changes:

  • rewrote -[RCTImageLoader dequeueTasks] to be more thread-safe and efficient
  • made RCTNetworkTask operations thread-safe
  • made all RCTHTTPRequestHandler calls happen from the same dispatch queue and tweaked its cancellation logic

Please try out these changes by pointing your local react-native install at that github commit. Once we get some feedback that this solves the issues people have been seeing we’ll land it on master and potentially do a new release of 0.37.

+9
javache on Nov 8, 2016

I submitted PR #11296 which I believe fixes the dequeueTasks crash. If anyone wants to test it, let me know if it resolves the crash for you.

+7
rigdern on Dec 5, 2016

+1 on this issue, this is by far our biggest crash (on RN 0.36)

+7
huitsing on Nov 14, 2016

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 😉

+5
mrspeaker on Oct 27, 2016

This issue has us still using 0.32. I’m not sure why this hasn’t become a more pressing issue to fix.

+5
chandlervdw on Oct 27, 2016

The bug is still there for us too ☹️

+4
runn3r85 on Nov 10, 2016

I confirm, unfortunately, the bug is still here.

+4
gitim on Nov 8, 2016

@gabegreenberg from 0.2x and now 0.37

+3
agumack on Nov 23, 2016

Thanks @gabegreenberg ! Specifically, our crash looks like: Specifically, ours manifests as

Crashed: com.facebook.react.ImageLoaderURLRequestQueue
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x0000000371c8beb8
---
RCTImageLoader.m line 258
__30-[RCTImageLoader dequeueTasks]_block_invoke
+3
huitsing on Nov 23, 2016

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: OSAtomicOr32Barrier(1, &cancelled). Which part is the "BAD_ACCESS"? I assume the issue is the &cancelled pointer is no longer valid, but how or why does that happen? I’d love to help, but this language isn’t my strong suit.

+3
seanadkinson on Oct 27, 2016

@booboothefool The crash in your screenshot was fixed by https://github.com/facebook/react-native/commit/2aca02150481ab558f136e816412cd0cff740c5f which is included in React Native 0.41 and higher.

+2
rigdern on May 4, 2017

@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.

+2
rigdern on Apr 4, 2017

@njafei as far as I know the last commit from @rigdern will be released with version 0.40

+2
gitim on Dec 30, 2016

@rigdern - We’ve now had your fixes in production for a few days - the crashes around dequeueTasks have DRASTICALLY been reduced, to the point that I consider this a huge success, so thanks a ton. I see you mentioned that you decline the bounty - but just wanting to be extra clear that we’d be totally happy to send the money your way.

We do still have some crashes in dequeueTasks though, so maybe there’s still a problem lurking in there. The number of crashes has gone into the noise rather than primary crash now though.

Thanks again!

+2
huitsing on Dec 16, 2016

Thanks @rigdern! We’re merging your two PRs in to a private React Native build and will push to production in the next day or two. I’ll definitely report back. I have high hopes! 👍

+2
huitsing on Dec 6, 2016

@nbyh100 I want to double check that I’ve understood you. There have been multiple image loader crashes reported across several versions of RN so it’s important we get the details right in terms of which crashes have been witnessed in which versions of RN.

You are saying that, even after applying my fix (PR #11145), RN 0.38 still crashes in dequeueTasks on line 254 (canceling the task).

After applying my fix and @SylviaRu’s fix to RN 0.38, you don’t see any other image loader crashes.

@nbyh100 is the above correct?

@SylviaRu can you provide an explanation for why dequeueTasks crashes and how your change fixes it?

+2
rigdern on Nov 29, 2016

I created a test app which consistently repros this crash for me in the simulator. The app just rapidly hides and shows a few images.

Repro Info

  • React Native 5850165795c54b8d5de7bef9f69f6fe6b1b4763d (from master on Nov 23)
  • iPhone 6s+ Simulator (iOS 9.2)
  • Using RN packager
  • Running app in debug mode

Repro Steps

The app crashes in under a minute. Usually, it crashes on one of these lines but I’ve seen it crash on a few other lines as well. Note that sometimes I see the images flickering and sometimes I don’t see them at all. Both eventually result in a crash for me.

I tried @SylviaRu’s suggestion but it still crashes.

+2
rigdern on Nov 25, 2016

@mkonicek Isn’t related to a device, a friend was able to reproduce the issue even in the iOS simulator. ( @pvsousalima )

+2
brenordr on Nov 24, 2016

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.

+2
pietropizzi on Oct 27, 2016

@aterribili Last fixes goes in 0.40 or 0.41, 0.39 and earlier will have the issue.

+1
brenordr on Jan 4, 2017

still crash in 0.39 😦

+1
njafei on Dec 30, 2016

Thanks @rigdern! We have merged your two PRs in to a private React Native build, we don’t see any other image loader crashes, but still crashes in dequeueTasks on line 254 (canceling the task),the same as @huitsing

+1
nmhuang on Dec 19, 2016

@huitsing, that’s great to here! That’s correct, I declined the bounty but thanks for the offer.

Are you sure that the dequeueTasks crashes you are still seeing are being hit in apps which have the fix? If yes, can you share the exception and stack trace?

+1
rigdern on Dec 16, 2016

Thanks @rigdern, if it works and is merged to master, how do we get in touch with you to pay our $250 towards the bounty?

+1
gabegreenberg on Dec 5, 2016

Nice! @rigdern we will give these a shot soon - excited about these changes! Thanks.

+1
huitsing on Dec 5, 2016

@rigdern You crash is this place?： crash

My plan is to solve this problem with crash info: [RCTImageLoader dequeueTasks]_block_invoke .

+1
SylviaRu on Nov 25, 2016

I don’t want to sound naive or not obvious but… Is this image loading cancelation really necessary? I mean… as you want them to load, not loading them would make a memory leak or a set of instructions for loading disposed? Me and my friend found a way to contour this and do not have a crash, and it worked for our purposes, however it does not look like a valid fix for the magnitude of this project

+1
pvsousalima on Nov 24, 2016

@animabear care to share your custom image View?

+1
pietropizzi on Nov 21, 2016

@javache I have tried, it didn’t help, the app is still crashing.

+1
gitim on Nov 7, 2016

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.

+1
Aaang on Oct 31, 2016

Is there a rnplay snippet that reproduces the crash?

+1
hramos on Oct 27, 2016
Read more comments on GitHub
← react-native: RCTFatal + 448 crash when testing via iOS TestFlight
react-native: RCTImageView / RCTImageLoader crash →