Fixed crashes due to race conditions with NSMutableDictionary access in AFHTTPRequestSerializer

[alexbird]

This is a fix for an intermittent crash bug which is likely to occur in any app which sets header values with dynamic values over around six characters long. These strings are reference counted, but the counting goes wrong because the owner, the headers dictionary, is not a thread-safe collection, but will be used from multiple threads in normal use of the AFNetworking library. The frequency of crashes increases greatly if you have a header which needs to be updated constantly. The crashes appear in areas of the app which are almost always completely unrelated, but often have deallocation in the stack trace.

This fix has been included in a shipping app with ~400k daily launches since the end of February, and has reduced our crash rate to almost nil.

I have included a unit test which reproduces the issue, but more as a courtesy than as a proper unit test. There is nothing to positively test for, the test succeeds if it doesn't EXC_BAD_ACCESS when cleaning up the @autoreleasepool at the end.

[alexbird]

CI tests seem to be failing either with unrelated test failures, which fail for master also on my dev machine, some intermittently, or the test runner failing. If you think that's incorrect and my change has caused a problem, do let me know!

[kcharwood]

Thank you so much for this fix! I bet that was a nasty one to track down!

🍻

[alexbird]

Yay, my first public PR! 🎉🎈🍾 Cheers

[hovox]

@kcharwood can we have release with this fix ?

[MattFoley]

I also ran into this issue recently and have been pretty perplexed by the problem, so thanks to @alexbird for tracking it down!

[knutigro]

Sadly I have gotten some new crashes which I believe happen because of this fix 👎

https://github.com/AFNetworking/AFNetworking/issues/3636

[tmm1]

Shouldn't this be a SERIAL queue?

[tmm1]

Never mind, I see that dispatch_barrier_async is used with CONCURRENT queues.

[tmm1]

@aopod I tried to revert the dispatch_queue change and use your recommendation of [self.mutableHTTPRequestHeaders copy], but the test-case included in this PR is still crashing:

Test Case '-[AFHTTPRequestSerializationTests testThatHTTPHeaderValueCanBeSetToReferenceCountedStringFromMultipleThreadsWithoutCrashing]' started.
2017-03-03 12:00:03.861 xctest[28179:13860457] *** -[CFString release]: message sent to deallocated instance 0x604000053e90
Program ended with exit code: 9

So it seems your blog post is referring to a similar but unrelated crash which also happens due to multithreaded use. The fix in this PR is more correct and fixes both issues.

However, I am still observing a rare crash in setValue:forHTTPHeaderField: which has also been reported by others in #3636.

[aopod]

@tmm1 I have to admit that dispatch_queue is a better way to solve this problem while my solution may depend on specific platform and some other situations may not be well-considered. Thank you for your feedback.

[tewha]

Why would you use queues and synchronous dispatching over a simple call to @synchronized?

[alexbird]

@tewha Slightly higher performance. A call to @synchronized would lock on both read and write, whereas the queue allows reads to happen concurrently, and only writes lock the resource and happen consecutively.
I think at the time I was following a Mike Ash article on the subject, but I can't find the one just now.

I just found this thread hidden in an email folder, so have been oblivious to the conversation so far!

[tewha]

Alright, I guess that makes sense. Thanks! :) I was a bit concerned it was hiding a possible deadlock to most reading the code.