Crashes on SessionDelegate.remove() when canceling Download Task

[jgarcia-mirego]

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

• I have read the wiki page and cheat sheet, but there is no information I need.
• I have searched in existing issues, but did not find a same one.
• I want to report a problem instead of asking a question. It'd better to use kingfisher tag in Stack Overflow to ask a question.

Issue Description

What

We are using Kingfisher to download images for our iOS app and I have encountered many crashes when trying to cancel a download task

EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x0000000000000020

Crashed: com.apple.main-thread
0  CFNetwork                      0x18436b058 HTTPParser::HTTPParser(__CFAllocator const*, HTTPParserClient*, HTTPParser*) + 44
1  CFNetwork                      0x18424b27c HTTPMessage::commonInitialization(unsigned char, HTTPMessage const*) + 212
2  CFNetwork                      0x18424b27c HTTPMessage::commonInitialization(unsigned char, HTTPMessage const*) + 212
3  CFNetwork                      0x18424b41c HTTPMessage::HTTPMessage(HTTPMessage const*) + 112
4  CFNetwork                      0x18436c1dc HTTPRequestMessage::HTTPRequestMessage(HTTPRequestMessage const*) + 36
5  CFNetwork                      0x18422a74c HTTPRequest::HTTPRequest(HTTPRequest const*) + 24
6  CFNetwork                      0x18422ce8c URLRequest::initialize(URLRequest const*, unsigned char) + 656
7  CFNetwork                      0x1841e2bd8 _createRequestCopy(__CFAllocator const*, _CFURLRequest const*, unsigned char) + 152
8  CFNetwork                      0x184355de0 -[NSURLRequest mutableCopyWithZone:] + 44
9  libswiftFoundation.dylib       0x105a95a0c URLRequest.init(_bridged:) + 77948
10 libswiftFoundation.dylib       0x105a99344 static URLRequest._unconditionallyBridgeFromObjectiveC(_:) + 92596
11 Kingfisher                     0x102fb5788 SessionDelegate.remove(_:) + 93 (SessionDelegate.swift:93)
12 Kingfisher                     0x102fb566c closure #1 in SessionDelegate.add(_:url:callback:) + 1000 (<compiler-generated>:1000)
13 Kingfisher                     0x102fb9eb0 partial apply for thunk for @escaping @callee_guaranteed (@guaranteed SessionDelegate, @unowned Int, @in_guaranteed SessionDataTask.TaskCallback) -> () + 84 (<compiler-generated>:84)
14 Kingfisher                     0x102f46734 specialized closure #1 in Delegate.delegate<A>(on:block:) + 96 (<compiler-generated>:96)
15 Kingfisher                     0x102fb9f34 partial apply for specialized closure #1 in Delegate.delegate<A>(on:block:) + 28 (<compiler-generated>:28)
16 Kingfisher                     0x102fb14d0 SessionDataTask.cancel(token:) + 356 (<compiler-generated>:356)
17 Kingfisher                     0x102f6de18 DownloadTask.cancel() + 74 (ImageDownloader.swift:74)
18 MyApp                          0x1011e55f4 UIImageView.cancel() + 4310799860 (<compiler-generated>:4310799860)
...
...

For example we call this method when reusing cells in a tableview, where we cancel requests for cells that are not longer visible. We have created an extension on UIImageView to define convenience methods like cancel()

What could be the reason for this crash? The download task in our code is an optional, so even when the object is not there anymore, calling image?.downloadTask.cancel() should not create crashes, should not even trigger those methods.

Here is a Sample code of what we do:

func fetchImage(fromURL url: URL, completion: @escaping (UIImage?, NSError?) -> Void) -> MyAppDownloadTask {    
	
	let resource = ImageResource(downloadURL: url)
	let options: KingfisherOptionsInfo = [.targetCache(cache), .downloader(downloader)]

	let task = KingfisherManager.shared.retrieveImage(with: resource, options: options, progressBlock: nil) { result in

	switch result {
		case .success(let value):
			completion(value.image, nil)
		case .failure(let error):
			completion(nil, error.toNSError())
	}
        }

	return MyAppDownloadTask(task: task)
}

private class MyAppDownloadTask {

	let task: DownloadTask? // KingFisher task

	init(task: DownloadTask?) {
		self.task = task
	}

	func cancel() {
		task?.cancel()
	}
}

We call the cancel method when reusing cells in our collection view:

override func prepareForReuse() {
	super.prepareForReuse()
	downloadTask.cancel()
}

Reproduce

I cannot share the URL that we call, but we've notice that it happens most of the time when the network request have been canceled from the server.

Info from New Relic:

networkErrorCode: -999
connectionType: LTE
networkError: cancelled
requestMethod: GET
typeOfImage: PNG

Other Comments

The UI updates (assign of UIImageViews) and the cancelation of request when are happening are being done in the Main Thread.

[cburnham4]

We are also getting the same issue and are using version "5.14.1" via carthage when canceling image requests within prepareForReuse()

Crashed: com.apple.main-thread
0 CFNetwork 0x199af5434 _CFStreamErrorFromCFError + 3360
1 CFNetwork 0x199af5630 _CFStreamErrorFromCFError + 3868
2 CFNetwork 0x199bfe8fc CFHTTPServerResponseEnqueue + 55568
3 CFNetwork 0x199adf2ec CFURLConnectionCopyTimingData + 7676
4 CFNetwork 0x199a26b70 (Missing)
5 libswiftFoundation.dylib 0x1cd6cb688 $s10Foundation10URLRequestV36_unconditionallyBridgeFromObjectiveCyACSo12NSURLRequestCSgFZ + 68
6 Kingfisher 0x104fbde5c SessionDelegate.remove(:) + 93 (SessionDelegate.swift:93)
7 Kingfisher 0x104fbdd40 closure #1 in SessionDelegate.add(:url:callback:) + 75 (SessionDelegate.swift:75)
8 Kingfisher 0x104fc2564 partial apply for thunk for @escaping @callee_guaranteed (@guaranteed SessionDelegate, @unowned Int, @in_guaranteed SessionDataTask.TaskCallback) -> () + 4318127460 (:4318127460)
9 Kingfisher 0x104f6075c specialized closure #1 in Delegate.delegate(on:block:) + 39 (Delegate.swift:39)
10 Kingfisher 0x104fc25e8 partial apply for specialized closure #1 in Delegate.delegate(on:block:) + 4318127592 (:4318127592)
11 Kingfisher 0x104fc7684 SessionDataTask.cancel(token:) + 4318148228 (SessionDataTask.swift:4318148228)
12 Kingfisher 0x104fb4b88 KingfisherWrapper.cancelDownloadTask() + 4318071688 (ImageView+Kingfisher.swift:4318071688)
13 RecoverCoreUI 0x1049e2cfc MessageCell.prepareForReuse() + 202 (MessageCell.swift:202)

[jgarcia-mirego]

We are using 5.14.0 via CocoaPods

[onevcat]

Tried to fix in 5.15.5. If anyone can confirm it.

[danielbyon-hulu]

We encountered this same crash when cancelling a bunch of image requests. Our stack trace looks like:

Thread 0 Crashed:
0   CFNetwork                            0x00000001b74573d8 HTTPMessage::commonInitialization(unsigned char, HTTPMessage const*) + 436 (HTTPParser.cpp:64)
1   CFNetwork                            0x00000001b74573bc HTTPMessage::commonInitialization(unsigned char, HTTPMessage const*) + 408 (CFObject.h:189)
2   CFNetwork                            0x00000001b74575b0 HTTPMessage::HTTPMessage(HTTPMessage const*) + 108 (HTTPMessage.cpp:35)
3   CFNetwork                            0x00000001b7560440 HTTPRequestMessage::HTTPRequestMessage(HTTPRequestMessage const*) + 36 (HTTPRequestMessage.cpp:10)
4   CFNetwork                            0x00000001b7442610 URLRequest::initialize(URLRequest const*, unsigned char) + 560 (HTTPRequest.cpp:54)
5   CFNetwork                            0x00000001b7392eb8 -[NSURLRequest mutableCopyWithZone:] + 80 (NSURLRequest.mm:534)
6   libswiftFoundation.dylib             0x00000001cac731d8 static URLRequest._unconditionallyBridgeFromObjectiveC(_:) + 64 (URLRequest.swift:45)
7   HuluAPI                              0x0000000101e3e9cc _hidden#6805_ + 212 (__hidden#6906_:93)
8   HuluAPI                              0x0000000101e3e8b4 _hidden#6804_ + 892 (__hidden#6906_:75)
9   HuluAPI                              0x0000000101e41edc _hidden#6838_ + 60 (__hidden#591_:0)
10  HuluAPI                              0x0000000101e41fa4 _hidden#6839_ + 96 (__hidden#725_:39)
11  HuluAPI                              0x0000000101e3b4a4 __hidden#5908_ + 32 (__hidden#725_:44)
12  HuluAPI                              0x0000000101e3b4a4 _hidden#2197_ + 328 (__hidden#6781_:107)
13  HuluAPI                              0x0000000101df19ac __hidden#2553_ + 8 (__hidden#2554_:73)
14  HuluAPI                              0x0000000101df19ac KingfisherWrapper<A>.cancelDownloadTask() + 32 (__hidden#2331_:249)

From the email chain we had with DTS:

In the backtrace above we see the last two frames end with creating a new HTTPMessage.  What happens in 
HTTPMessage::commonInitialization is that a new HTTPParser object is created based on the original that is
passed into HTTPMessage.  This runs into a race condition with a NSURLSession task when the copy takes 
place for HTTPParser.  The issue does not exist in iOS 14 / tvOS 14 because a lock was placed around the
creation of the new HTTPParser object. 

So, how do you workaround this in iOS 13 / tvOS 13 for the use case you have described?  You can still cancel
 the task, but can’t call properties on the original task until it has completely cancelled.  This is the result of 
the crash and KERN_INVALID_ADDRESS exception you are getting.  Examples of such properties to stay away 
from before the request is cancelled would be currentRequest or originalRequest as these will cause this issue.
I would take a look at the Kingfisher library for any instances where task properties are used before the task is 
completely cancelled.  Look for the usage of currentRequest or originalRequest specifically.

I'm seeing references to URLRequest.originalRequest in SessionDelegate.swift (where it uses originalRequest.url as a tasks lookup key), this looks to be the source of the race condition crash. Since URLRequest is Hashable, can we use the entire request object as the dictionary key, instead of inspecting originalRequest.url?

[onevcat]

@danielbyon-hulu Ummm, that is really hidden pitfall. Nice to hear that it is fixed in iOS 14.

We cannot use the request directly as the key. The current implementation is an optimization to prevent multiple downloading from the same URL. Changing an object identifier key (the hash of request, etc) would break this feature.

May I know is it possible for you to build a stable reproducible sample? So we can use that to check whether a fix works or not.

Anyway, I tried to adjust some locks to make sure the accessing of URLRequest.originalRequest properties not happening at the same time as cancel. You can find these changes in #1558

Any feedback is welcome!

[danielbyon-hulu]

Unfortunately, we've only ever seen this crash in NewRelic monitoring, we've never been able to repro locally.

I looked at your PR and added comments.