Crash when cancelAll by SDWebImageManager.

[maeji]

In tableView, I used SDWebImageManager. When I was try to cancel all download, My app was crashed at "self.cancelBlock = nil;" in SDWebImageManager.m (317 line).

I think that is released object problem.

SDWebImageCombinedOperation has cancelBlock. And It set like that,

operation.cancelBlock = ^{
     [subOperation cancel];
                @synchronized (self.runningOperations) {
                    [self.runningOperations removeObject:weakOperation];
                }
};

Also it was copied by below function in code.

- (void)setCancelBlock:(void (^)())cancelBlock {
    if (self.isCancelled) {
        if (cancelBlock) cancelBlock();
    }
    else {
        _cancelBlock = [cancelBlock copy];
    }
}

It was crashed at "self.cancelBlock = nil;" in cancel function in SDWebImageCombinedOperation. In my thought that was SDWebImageCombinedOperation was released before set nil.

- (void)cancel {
    self.cancelled = YES;
    if (self.cacheOperation) {
        [self.cacheOperation cancel];
        self.cacheOperation = nil;
    }
    if (self.cancelBlock) {
        self.cancelBlock();
        self.cancelBlock = nil;
    }
}

When I removed "self.cancleBlock = nil;". It was working perfect. Is it right?

[bpoplauschi]

Mi @maeji. We will look into this crash soon.

[rromanchuk]

can confirm crashing when canceling prefetcher.

Application Specific Information:
objc_msgSend() selector name: setCancelBlock:

Thread 0 Crashed:
0   libobjc.A.dylib                      0x3861f636 objc_msgSend + 22
1   CoreFoundation                       0x2ddf2a95 -[NSArray makeObjectsPerformSelector:] + 178
2   Frontback β                          0x0018165d -[SDWebImageManager cancelAll] (SDWebImageManager.m:286)
3   Frontback β                          0x001825d7 -[SDWebImagePrefetcher cancelPrefetching] (SDWebImagePrefetcher.m:135)

If you guys need help testing with a real sample size, I can help by pushing out HEAD before SDWebImage cuts a release. Right now there are two show stopping crashes that took less than a minute to discover in a real world environment.

[bpoplauschi]

@rromanchuk I have tried to reproduce this crash, but I doesn't occur to me. Could you help me with that? Do you have a scenario that works all the time?

[bpoplauschi]

Also, could you please check using the latest HEAD for the other crash (#808), it should be fixed now.

[rromanchuk]

Yes #808 is fine, this bug is hard to reproduce, and I only see it frequently with 400+ active users, it's still my number one crash at the moment though.

I reduced a majority of the crashes by removing calls to - (void)cancelPrefetching; from SDWebImagePrefetcher

I will try and make the crash reproducible by prefetching and canceling but you can also see prefetching is canceled when given a new set of urls

- (void)prefetchURLs:(NSArray *)urls progress:(SDWebImagePrefetcherProgressBlock)progressBlock completed:(SDWebImagePrefetcherCompletionBlock)completionBlock {
    [self cancelPrefetching]; // Prevent duplicate prefetch request
    self.startedTime = CFAbsoluteTimeGetCurrent();
    self.prefetchURLs = urls;
    self.completionBlock = completionBlock;
    self.progressBlock = progressBlock;

    // Starts prefetching from the very first image on the list with the max allowed concurrency
    NSUInteger listCount = self.prefetchURLs.count;
    for (NSUInteger i = 0; i < self.maxConcurrentDownloads && self.requestedCount < listCount; i++) {
        [self startPrefetchingAtIndex:i];
    }
}

[rromanchuk]

My guess is the best way to make it crash is by having active prefetching URLS, and interrupt them before they have finished

It looks like I fixed the crash by changing self.cancelBlock = nil to _cancelBlock = nil as the crash is happening because self does not respond to the setter, something must be releasing too early. This is a similar finding that @maeji is reporting

- (void)cancel {
    self.cancelled = YES;
    if (self.cacheOperation) {
        [self.cacheOperation cancel];
        self.cacheOperation = nil;
    }
    if (self.cancelBlock) {
        self.cancelBlock();
        _cancelBlock = nil; // <------ fixes the crash
    }
}

[bpoplauschi]

I think I found the issue. self.cancelBlock = nil; will trigger a call to our custom setter setCancelBlock:. Looking at that code

- (void)setCancelBlock:(SDWebImageNoParamsBlock)cancelBlock {
    if (self.isCancelled) {
        if (cancelBlock) cancelBlock();
    }
    else {
        _cancelBlock = [cancelBlock copy];
    }
}

• the check of self.isCancelled passes since we just had self.cancelled = YES;
• then if (cancelBlock) will not pass, as the call to the setter was made using nil as param
• and then we exit, without ever setting _cancelBlock to nil
• so if after this, an attempt to use self.cancelBlock will lead to a crash.

[bpoplauschi]

@rromanchuk Thanks for the support. Could you also check my latest fix (28109c4) against this crash? It shouldn't crash anymore (your clue that _cancelBlock = nil; got me on the right track). As soon as I get the approval, I will release that patch.

[rromanchuk]

@bpoplauschi thanks! Let me rebuild with this commit, looks good though

[bpoplauschi]

@rromanchuk did you get a chance to try this out? I want to release the patch asap.

[rromanchuk]

@bpoplauschi sending a release out now, should know soon

[rromanchuk]

looks like i'm still getting it :(

Thread 0 Crashed:
0 libobjc.A.dylib 0x3a262626 objc_msgSend + 6
1 CoreFoundation 0x2fa33ad5 -[NSArray makeObjectsPerformSelector:] + 178
2 Frontback β 0x001f2ce9 -SDWebImageManager cancelAll
3 Frontback β 0x001f3c7b -SDWebImagePrefetcher cancelPrefetching
4 Frontback β 0x001f3afd -SDWebImagePrefetcher prefetchURLs:progress:completed:
5 Frontback β 0x001f3ac1 -SDWebImagePrefetcher prefetchURLs:

[bpoplauschi]

Until we can figure out the exact cause, made a temp fix (e95224b) with the suggested _cancelBlock = nil

[bpoplauschi]

3.7.1 just got out. Could you please check and see if this crash still reproduces or is fixed as it should be?

[maeji]

Still reproduce that crash.

[bpoplauschi]

What version are you using? I'm asking because that code is a bit different now

[rromanchuk]

Bogdan, things seem OK with the patch, will let you know if I see anything

Sent from my iPhone

On Jul 24, 2014, at 12:12 AM, Bogdan Poplauschi notifications@github.com wrote:

What version are you using? I'm asking because that code is a bit different now

—
Reply to this email directly or view it on GitHub.

[bpoplauschi]

Great, thanks.

[farhanumer]

I just got the same crash with 3.7.1

[justjimmy]

I seem to occasionally have this issue too.

When I do:

[[SDWebImageManager sharedManager] cancelAll];

I seem to get this exception:

* Terminating app due to uncaught exception 'NSGenericException', reason: '* Collection <__NSArrayM: 0x17024c390> was mutated while being enumerated.'

[justjimmy]

[justjimmy]

this seems to fix it for me.

Pull request: #838

[DmitrijMaz]

same thing happens for cacheOperation setter.

• (void)cancel {
  self.cancelled = YES;
  if (self.cacheOperation) {
  [self.cacheOperation cancel];
  self.cacheOperation = nil; // <---- crash here
  }
  if (self.cancelBlock) {
  self.cancelBlock();

  // TODO: this is a temporary fix to #809.
  // Until we can figure the exact cause of the crash, going with the ivar instead of the setter
  

  // self.cancelBlock = nil;
  _cancelBlock = nil;
  }
  }

[matteogobbi]

It continue to crash.

[dmiedema]

I can consistently get this to crash by prefetching different sets of images very quickly.

I created an example project to show the crash with instructions in the README on how to trigger the crash also. It currently crashes for me 100% of the time

https://github.com/dmiedema/SDWebImageCrashExample

[justjimmy]

I had a pull request that fixed it for me. It was merged in but you have to grab it using the pod with a specific commit. It's not in 3.7.1 but will be in 3.7.2. Can you try in and see?—
Sent from Mailbox

On Wed, Sep 3, 2014 at 3:54 AM, Daniel Miedema notifications@github.com
wrote:

I can consistently get this to crash by prefetching different sets of images very quickly.
I created an example project to show the crash with instructions in the README on how to trigger the crash also. It currently crashes for me 100% of the time

https://github.com/dmiedema/SDWebImageCrashExample

Reply to this email directly or view it on GitHub:
#809 (comment)

[dmiedema]

@justjimmy looks like referencing that commit fixes the issue in the example project.

Thanks

For anyone curious and reading this, until its put into a new release putting

pod 'SDWebImage', :git => 'https://github.com/rs/SDWebImage', :commit => '3587b8d1e0f51710dfd1cec0c3a723048f7473f4'

in your Podfile will fix this particular issue.

[justjimmy]

@dmiedema awesome. I think it'll be part of 3.7.2 whenever that is ready.

[rromanchuk]

@dmiedema any reason you are not using pod 'SDWebImage', :head? I recently dropped down to 3.6 because other issues were popping up, besides the cancel all issue, but haven't verified it's due to SDWebImage, needs much more investigation. Just can't afford to upgrade until it has served at least million images on production. Swamped with ios8 stuff right now, come back to it and hopefully help out when i have more time.

[dmiedema]

@rromanchuk theres no particular reason im not using :head. I personally tend to prefer releases since i feel & hope theyre more tested & verified to be working.

Currently we've worked around the cancel all bug. i just didnt know it was resolved so i wanted to make a test project where it was reproducable.

Its our most common crash for now, however with the work around & known fix it should become a non-issue. At least for us.

[rsanchezsaez]

Commit b59c2bb doesn't fix this. I just got it:

[drumnkyle]

Any idea when 3.1.2 is going to come out with this fix? We are trying to push an update to the SlideShare app tomorrow and this in our top 3 crashes.

I think I'm just going to change our pod to reference the specific commit. Is 3587b8d still the right one to use to fix all these issues, including 798?

[timkoma]

I'm using 3.7.1 and I'm still getting crash.
Why it was closed ?

[mythodeia]

@timkoma this issues is not closed
you got confused of the reference of the other issue. this is still open

[timkoma]

I think that in my case it is a concurency problem. I called [[SDWebImagePrefetcher sharedImagePrefetcher] prefetchURLs:url];
several times from background threads.
I wasn't aware of the cancelAll at the beginning of the prefetchURLs. Also there is no warning in the doc that I have to call prefetchURLs from main thread. Anyway I think that prefetchURLs should work in the background (at least it will be very nice feature).