RTCImageLoader crash when no connexion

[machard]

Issue Description

When no connexion, RTCImage loader crash on cancelling a previous load request

Steps to Reproduce / Code Snippets

I have a scrollview with network images. I disable wifi (on my real device) and relaunch the app, scroll a little (until one of the image becomes out of screen) and it crashes. it seems to come from request cancellation

Expected Results

no crash :)

Additional Information

• React Native version: 0.34.0
• Platform(s) (iOS, Android, or both?): ios
• Operating System (macOS, Linux, or Windows?): macOS

[machard]

1a62b66 does not fix it for me

[machard]

@javache

[gre]

experiencing the exact same bug. EXC_BAD_ACCESS at cancelLoad. very critical to us.

And i don't actually have to reload the app. I just need a bad network and some re-rendering that change some images (e.g. on a searching list).

I think you probably can reproduce the bug easily by loading random images and having an intensive rerendering loop.

[pfeiffer]

@gre would you mind testing the patch in #10147 and see if it solves the issue?

[gre]

will do

[gre]

@pfeiffer i'm afraid that didn't fixed it to me.

[pfeiffer]

Allright. I've done some further research and I'm pretty sure the issue is in the dequeueTasks method, which references self in an async queue without the usual weakify/strongify dance first:

- (void)dequeueTasks
{
  dispatch_async(_URLRequestQueue, ^{
    // Remove completed tasks
    for (RCTNetworkTask *task in self->_pendingTasks.reverseObjectEnumerator) {
      switch (task.status) {
        case RCTNetworkTaskFinished:
          [self->_pendingTasks removeObject:task];
          self->_activeTasks--;
          break;
...

My objective-c is fairly limited - but I'm pretty sure that ARC can deallocate self while the block is running, which I think can cause eg. [self->_pendingTasks removeObject:task]; to crash.

The commit that introduced these changes seem to be an automated commit (bcf4bb6)

What do you think?

[javache]

@pfeiffer If you reference self in a block, ARC will actually correctly retain self from the block object until the block is released. The automated commit made the usage of self explicit, since we were previously just using the instance variable directly (which the compiler translates to a self reference under the hood).

[pfeiffer]

Got it. It might be something else, then. The crash I'm seeing in relation to dequeueTasks is:

Crashed: com.facebook.react.ImageLoaderURLRequestQueue
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x00000000a000000c

0  libobjc.A.dylib                0x24af7a66 objc_msgSend + 5
1  App                            0x1a87f9 __30-[RCTImageLoader dequeueTasks]_block_invoke (RCTImageLoader.m:240)
2  libdispatch.dylib              0x24ebd823 _dispatch_call_block_and_release + 10
3  libdispatch.dylib              0x24eca423 _dispatch_queue_drain$VARIANT$mp + 1758
4  libdispatch.dylib              0x24ec9a61 _dispatch_queue_invoke$VARIANT$mp + 284
5  libdispatch.dylib              0x24ecc5e9 _dispatch_root_queue_drain + 1560
6  libdispatch.dylib              0x24ecbfcd _dispatch_worker_thread3 + 96
7  libsystem_pthread.dylib        0x25081b29 _pthread_wqthread + 1024
8  libsystem_pthread.dylib        0x25081718 start_wqthread + 8

[pfeiffer]

I managed to consistently reproduce the error on a test device.

As @machard mentions, the fix in 1a62b66 doesn't (completely) solve the problem. There is an additional place where we need to check that the request hasn't already been cancelled:

react-native/Libraries/Image/RCTImageLoader.m

Line 508 in 1a62b66

if (cancelLoad) {

I haven't been able to reproduce the crash after changing the condition to include !cancelled:

    if (cancelLoad && !cancelled) {
      cancelLoad();
      cancelLoad = nil;
    }
    OSAtomicOr32Barrier(1, &cancelled);

Could it be a possible race condition issue, where we call cancelLoad although the request has already been cancelled?

[gre]

I'm still experiencing a crash with the !cancelled code you shared

I can easily reproduced it if I test on a real device (iPhone 6, iOS 10) and with a bad network (I'm far from the wifi, so have like 500ms ping ^^)

[pfeiffer]

I can confirm that this is still happening in "the wild" with 0.34 even with the patches mentioned above.

[i198622]

I also have this bug.

[senthilsivanath]

Experiencing same bug.

[gre]

anyone have tried if #10280 fixes this?

[machard]

@gre The crash I described in this issue does not seem to happen anymore to me :)

[alexpolzien]

What is the recommended fix until this fix is released? I have an app in production where this crash is affecting roughly 10% of users.

I tried cherry-picking 26be005 into RN 0.34, but I am still getting crashes on some calls to RCTImageLoader dequeueTasks.

[gre]

@alexpolzien have you tried cherry-picking b2dac83 as well ?

[alexpolzien]

@gre Yes, sort-of. In my most recent patch, I just snapshotted the whole RNImage library at 26be005, which includes that fix. So far I have only seen one crash from RCTImageLoader, albeit on a fairly small number of sessions. So it seems to be better, but not completely fixed.

[hilkeheremans]

Even after cherry picking the commit @alexpolzien mentions, we were still experiencing crashes using the Image component (wrapped into a custom component of ours).

This might not be 100% related, but mentioning this here for future reference and perhaps more insight by others: in our case, removing the defaultSource attribute greatly improved stability (no crashes seen so far).

It appears that as the Image is loaded and replaces the "placeholder" provided by defaultSource, this often crashes the entire app (with wildly varying exceptions thrown, which makes it extremely hard to debug). It feels like there is a problem with already deallocated references as RN tries to reconcile, but too hard for us to diagnose completely due to limited ObjC knowledge here.

YMMV of course, but worth a try if you are using defaultSource.

[alexpolzien]

@hilkeheremans Thanks but I am not using defaultSource at all. I am, however, making pretty aggressive use of Image.prefetch, and I suspect that may be the issue - or just trying to load a lot of images over HTTP at a time in general.

[lacker]

The original bug reporter has reported this issue as fixed, so I am going to close this issue. It does sound like other people might have similar problems still though. If that is the case I encourage people to open a new issue to report these new problems. Thanks!