fix: Avoid calling 'removeFromRunLoop' on 'NSStream' object when disconnecting from peer

[tikhop]

Issue being fixed or feature implemented

This PR fixed the infamous crash that occurs when the app attempts to disconnect from a peer. The main problem lies in certain scenarios where the -[DSPeer disconnectWithError:] method can be invoked on the same peer from multiple threads, leading to a crash in the NSStream object:

#1 Stack trace:
0. CFArrayGetCount //Array here is nil
1. _CFStreamClose
2. -[DSPeer disconnectWIthError:]
3. ...

#2 Stack trace:
0. CFArrayGetCount //Array here is nil
1. _CFStreamUnscheduleFromRunLoop
2. -[DSPeer disconnectWIthError:]

Currently, to disconnect from the peer, we have the following code:

[self.inputStream close];
[self.outputStream close]; // Crash happens here
[self.inputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];
[self.outputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes]; // Or here (randomly)

Looking inside of _CFStreamClose methods we can understand why the crash happens:

...
0x115c11704 <+220>: adrp   x24, 1386
0x115c11708 <+224>: ldr    x0, [x24, #0x648]         ; dictionary 
0x115c1170c <+228>: mov    x1, x19                   ; x1 is a key, x1 = <__NSCFInputStream: 0x6000016b07e0>
0x115c11710 <+232>: bl     0x115ba96b8               ; CFDictionaryGetValue
0x115c11714 <+236>: mov    x21, x0                   ; x21 = <__NSArrayI 0x600002412740> or nil when crash happens
0x115c11718 <+240>: ldr    x0, [x24, #0x648]         ; dictionary
0x115c1171c <+244>: mov    x1, x21                   ; x21 = <__NSArrayI 0x600002412740> 
0x115c11720 <+248>: bl     0x115ba96b8               ; CFDictionaryGetValue
0x115c11724 <+252>: mov    x22, x0
0x115c11728 <+256>: bl     0x115b6e508               ; CFArrayGetCount
0x115c1172c <+260>: mov    x20, x0
...
0x115c117ec <+452>: mov    x1, x19
0x115c117f0 <+456>: bl     0x115baa08c               ; CFDictionaryRemoveValue // Remove <__NSCFInputStream: 0x6000016b07e0> from the dict
0x115c117f4 <+460>: add    x21, x19, #0x38
0x115c117f8 <+464>: mov    x0, x21
...

Basically, it tries to access the value of the dictionary that was already removed. The same code we have inside _CFStreamUnscheduleFromRunLoop and it crashes at the same logical place.

To fix the issue, simple avoid calling:

[self.inputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];
[self.outputStream removeFromRunLoop:self.runLoop forMode:NSRunLoopCommonModes];

Based on Apple docs the stream will be removed from run loop inside -[NSStream close] method:

Closing the stream terminates the flow of bytes and releases system resources that were reserved for the stream when it was opened. If the stream has been scheduled on a run loop, closing the stream implicitly removes the stream from the run loop. A stream that is closed can still be queried for its properties.

To prove that it is true, we can look inside of _CFStreamClose:

CoreFoundation._CFStreamClose:
    ...
    0x115a056e4 <+188>: mov    x0, x21
    **0x115a056e8 <+192>: bl     0x1159e2790               ; CFRunLoopSourceInvalidate**
    0x115a056ec <+196>: mov    x0, x21
    0x115a056f0 <+200>: bl     0x1159e7e50               ; CFRelease
    ...
    0x115a057ac <+388>: mov    x0, x23
    0x115a057b0 <+392>: mov    x1, x22
    **0x115a057b4 <+396>: bl     0x1159e1104               ; CFRunLoopRemoveSource**
    0x115a057b8 <+400>: mov    x0, x22
    0x115a057bc <+404>: bl     0x1159e7e50               ; CFRelease
    ...
    0x115a058a0 <+632>: ret    

What was done?

• Stop calling -[NSStream removeFromRunLoop:forMode] when peer gets disconnected

How Has This Been Tested?

To reproduce (simulate) the issue, simply try to call disconnect method from another thread right after we open the streams:

[self.inputStream open];
[self.outputStream open];
[NSThread detachNewThreadSelector:@selector(disconnect) toTarget:self withObject:nil];

Breaking Changes

N/A

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone