Crash in `TaskFetchOriginalImageData.swift`

[amirdew]

Hi Alexander, thanks for the great library.

I noticed a crash related to TaskFetchOriginalImageData.swift:107 (data.append(chunk))

Could it be related to private lazy var data = Data()? I thought if it's not thread-safe and if it's possible to access it from multiple threads then that might be the cause.

Let me know if you need more information

SIGTRAP (#0): Application crash: SIGTRAP (Trace/BPT trap)
Thread 8 Crashed:
0  libswiftFoundation.dylib   0x0000000185565ee4 specialized __DataStorage.init(bytes: UnsafeMutableRawPointer?, length: Int, copy: Bool, deallocator: (UnsafeMutableRawPointer, Int)?, offset: Int) + 360
1  libswiftFoundation.dylib   0x0000000185451714 Data.InlineSlice.ensureUniqueReference() + 156
2  libswiftFoundation.dylib   0x000000018545a944 Data.InlineSlice.append(contentsOf: UnsafeRawBufferPointer) + 40
3  libswiftFoundation.dylib   0x000000018545b960 Data._Representation.append(contentsOf: UnsafeRawBufferPointer) + 580
4  Transfer                   0x0000000105192228 TaskFetchOriginalImageData.dataTask(didReceiveData: Data, response: NSURLResponse) + 900 /Users/vagrant/git/SourcePackages/checkouts/Nuke/Sources/Core/Tasks/TaskFetchOriginalImageData.swift:107
5  Transfer                   0x0000000105193e10 partial apply for closure #1 () in closure #1 (Data, NSURLResponse) in TaskFetchOriginalImageData.loadData(urlRequest: URLRequest, finish: ()) + 28 <compiler-generated>:0
6  Transfer                   0x0000000105170f10 thunk for @escaping @callee_guaranteed () -> () + 28 <compiler-generated>:0
7  libdispatch.dylib          0x00000001809fae68 _dispatch_call_block_and_release + 32
8  libdispatch.dylib          0x00000001809fca2c _dispatch_client_callout + 20
9  libdispatch.dylib          0x0000000180a04124 _dispatch_lane_serial_drain + 668
10 libdispatch.dylib          0x0000000180a04c80 _dispatch_lane_invoke + 392
11 libdispatch.dylib          0x0000000180a0f500 _dispatch_workloop_worker_thread + 648
12 libsystem_pthread.dylib    0x00000001f1d1a0bc _pthread_wqthread + 288
13 libsystem_pthread.dylib    0x00000001f1d19e5c start_wqthread + 8

SIGTRAP (#0): Application crash: SIGTRAP (Trace/BPT trap)
Thread 10 Crashed:
0  libswiftFoundation.dylib   0x00000001db9b2ee4 0x1db892000 + 1183460
1  libswiftFoundation.dylib   0x00000001db89e6ec 0x1db892000 + 50924
2  libswiftFoundation.dylib   0x00000001db8a791c 0x1db892000 + 88348
3  libswiftFoundation.dylib   0x00000001db8a8938 0x1db892000 + 92472
4  Transfer                   0x000000010450a228 TaskFetchOriginalImageData.dataTask(didReceiveData: Data, response: NSURLResponse) + 900 /Users/vagrant/git/SourcePackages/checkouts/Nuke/Sources/Core/Tasks/TaskFetchOriginalImageData.swift:107
5  Transfer                   0x000000010450be10 partial apply for closure #1 () in closure #1 (Data, NSURLResponse) in TaskFetchOriginalImageData.loadData(urlRequest: URLRequest, finish: ()) + 28 <compiler-generated>:0
6  Transfer                   0x00000001044e8f10 thunk for @escaping @callee_guaranteed () -> () + 28 <compiler-generated>:0
7  libdispatch.dylib          0x00000001d6e3ae6c 0x1d6e39000 + 7788
8  libdispatch.dylib          0x00000001d6e3ca30 0x1d6e39000 + 14896
9  libdispatch.dylib          0x00000001d6e44124 0x1d6e39000 + 45348
10 libdispatch.dylib          0x00000001d6e44c80 0x1d6e39000 + 48256
11 libdispatch.dylib          0x00000001d6e4f500 0x1d6e39000 + 91392
12 libsystem_pthread.dylib    0x000000024833c0bc 0x24833b000 + 4284
13 libsystem_pthread.dylib    0x000000024833be5c 0x24833b000 + 3676

[kean]

This code is designed to be confined to the pipeline's serial dispatch queue. Let me check. What framework version are you using?

[amirdew]

These crashes are from 10.10.0

[kean]

Thanks. By looking at the code, it is confined to the queue and this code hasn't changed in a long while. Will need to investigate more.

[kean]

I re-ran thread-safety tests with TSan enabled, and it's not pointing to any synchronization issues. I'm confident in this code because it's been in production for a while and is structurally solid – everything is synchronized on a single serial dispatch queue.

I found a few similar reports, online and they match my theory that this is due to high memory usage.

[amirdew]

Thanks for looking into this.
Makes sense to me since we used the same framework version in the past and there was no crash.

I'll search for memory leaks in other parts of our app.

[AvdLee]

I'm looking closer into this issue and I can't find indications of crazy high memory usage.

For example:

Isn't this about a Swift runtime error?

Picture taken from Apple's documentation

I'm looking closer into this now

[AvdLee]

The crash happened on this line:
https://github.com/kean/Nuke/blob/10.10.0/Sources/Core/Tasks/TaskFetchOriginalImageData.swift#L107

While @kean your conclusions are great regarding thread safety, I do wonder how the lazy var data = Data() might affect this.

data is written inside dataTask(didReceiveData chunk: Data, response: URLResponse) as well as read from func dataTaskDidFinish(error: Swift.Error?) in which it can't be that these access at the same time due to the ImagePipeline serial queue.

However, within the same method, it could be that the lazy var is triggered while also writing. I wonder whether that might result in the Swift runtime error we're seeing 🤔

Nevertheless, Nuke 11 got released. I think it's worth updating and verifying whether the crash still occurs before taking next steps.

[Ogerets]

Same issue.
And we have the same situation, where we used the same framework version for a while, but crashes started occurring.
Can be related to iOS updates

[kean]

I don't think it's likely that lazy is the problem, but let's remove it in a patch release just to be safe.

This data is transferred across queues when progressive decoding is enabled, and that might be the problem. However, the default decoders are used on the same pipeline's serial queue, so it's still thread-safe. It's never accessed anywhere outside to the serial queue during mutating, except for a few corner cases.

Are you loading any progressive JPEG or other types of progressive images? Are you using custom decoders (ImageDecoding)?

[AvdLee]

I don't think it's likely that lazy is the problem, but let's remove it in a patch release just to be safe.

Good idea!

Are you loading any progressive JPEG or other types of progressive images? Are you using custom decoders (ImageDecoding)?

We are not; we're doing nothing special other than just basic image loading.

[kean]

I removed lazy in Nuke 11.1.0. It's not mentioned in the release notes because I'm not sure it does anything. I would appreciate if you could update and let me know if the crashes stop.

[AvdLee]

I can at least confirm that it still occurs with Nuke 11.0. I'll update to 11.1 now and check in with you later!

[kean]

I found a new prefersIncrementalDelivery option in URLSessionTask. I'm going to use it to make sure there are no incremental appends.