Issue #156: Prevents crash in handleEvent method #169

jleandroperez · 2014-04-25T20:23:53Z

We've been seeing crashes with the stack trace posted below. Our app is crashing during a block_copy operation, performed inside the method [SRWebSocket stream:handleEvent:].

Although i've been unable to directly reproduce this, i've added a couple safety measures that would prevent this from happening.

Suspected scenario:

NSInputStream executes stream:handleEvent: in a BG thread.
A the exact same time, SRWebSocket instance gets dealloc'ed.
When stream:handleEvent: calls the dispatch_async routine, reference to self is no longer valid.

Workaround:

Updated stream:handleEvent: method: part of the code was moved to a second method, with a self-strong reference, stored in the stack.
Replaced 'self' with 'weakSelf', when appropriate.
NSStream delegate(s) are now being cleaned up in SocketRocket's NSRunLoop.

Any feedback will be very welcome. Thanks in advance, and congratulations on building this awesome project!

Crash Stack Trace:

Crashed Thread:  3

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Thread 0:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib          0x00007fff894a9a1a mach_msg_trap + 10
1   libsystem_kernel.dylib          0x00007fff894a8d18 mach_msg + 64
2   com.apple.CoreFoundation        0x00007fff94743fc5 __CFRunLoopServiceMachPort + 181
3   com.apple.CoreFoundation        0x00007fff947435e9 __CFRunLoopRun + 1161
4   com.apple.CoreFoundation        0x00007fff94742f25 CFRunLoopRunSpecific + 309
5   com.apple.HIToolbox             0x00007fff912eda0d RunCurrentEventLoopInMode + 226
6   com.apple.HIToolbox             0x00007fff912ed7b7 ReceiveNextEventCommon + 479
7   com.apple.HIToolbox             0x00007fff912ed5bc _BlockUntilNextEventMatchingListInModeWithFilter + 65
8   com.apple.AppKit                0x00007fff8fc603de _DPSNextEvent + 1434
9   com.apple.AppKit                0x00007fff8fc5fa2b -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
10  com.apple.AppKit                0x00007fff8fc53b2c -[NSApplication run] + 553
11  com.apple.AppKit                0x00007fff8fc3e913 NSApplicationMain + 940
12  libdyld.dylib                   0x00007fff8b3c95fd start + 1

Thread 1:: Dispatch queue: com.apple.libdispatch-manager
0   libsystem_kernel.dylib          0x00007fff894ae662 kevent64 + 10
1   libdispatch.dylib               0x00007fff91d0743d _dispatch_mgr_invoke + 239
2   libdispatch.dylib               0x00007fff91d07152 _dispatch_mgr_thread + 52

Thread 2:
0   libsystem_kernel.dylib          0x00007fff894a9a1a mach_msg_trap + 10
1   libsystem_kernel.dylib          0x00007fff894a8d18 mach_msg + 64
2   com.apple.CoreFoundation        0x00007fff94743fc5 __CFRunLoopServiceMachPort + 181
3   com.apple.CoreFoundation        0x00007fff947435e9 __CFRunLoopRun + 1161
4   com.apple.CoreFoundation        0x00007fff94742f25 CFRunLoopRunSpecific + 309
5   com.apple.AppKit                0x00007fff8fe0016e _NSEventThread + 144
6   libsystem_pthread.dylib         0x00007fff907cc899 _pthread_body + 138
7   libsystem_pthread.dylib         0x00007fff907cc72a _pthread_start + 137
8   libsystem_pthread.dylib         0x00007fff907d0fc9 thread_start + 13

Thread 3 Crashed:
0   libobjc.A.dylib                 0x00007fff88bff7a2 objc_retain + 18
1   libsystem_blocks.dylib          0x00007fff89371580 _Block_copy_internal + 308
2   libdispatch.dylib               0x00007fff91d05546 _dispatch_Block_copy + 43
3   libdispatch.dylib               0x00007fff91d0b13f dispatch_async + 17
4   com.simperium.Simperium-OSX     0x0000000107fcd3aa -[SRWebSocket stream:handleEvent:] + 801
5   com.apple.CoreFoundation        0x00007fff94792c81 _signalEventSync + 385
6   com.apple.CoreFoundation        0x00007fff94792ac8 _cfstream_solo_signalEventSync + 328
7   com.apple.CoreFoundation        0x00007fff9479293f _CFStreamSignalEvent + 623
8   com.apple.CFNetwork             0x00007fff949ba16e CoreWriteStreamCFStreamSupport::coreStreamWriteEvent(__CoreWriteStream*, unsigned long) + 102
9   com.apple.CFNetwork             0x00007fff949ba0dd CoreWriteStreamClient::coreStreamEventsAvailable(unsigned long) + 53
10  com.apple.CFNetwork             0x00007fff94aeda65 CoreStreamBase::_callClientNow(CoreStreamClient*) + 53
11  com.apple.CFNetwork             0x00007fff949e3558 ___ZN14CoreStreamBase34_streamSetEventAndScheduleDeliveryEmh_block_invoke + 33
12  com.apple.CFNetwork             0x00007fff949d32ec ___ZNK17CoreSchedulingSet13_performAsyncEPKcU13block_pointerFvvE_block_invoke + 25
13  com.apple.CoreFoundation        0x00007fff9471db44 CFArrayApplyFunction + 68
14  com.apple.CFNetwork             0x00007fff949d31cb RunloopBlockContext::perform() + 115
15  com.apple.CFNetwork             0x00007fff949d3073 MultiplexerSource::perform() + 269
16  com.apple.CFNetwork             0x00007fff949d2ea2 MultiplexerSource::_perform(void*) + 72
17  com.apple.CoreFoundation        0x00007fff94752661 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
18  com.apple.CoreFoundation        0x00007fff94743d12 __CFRunLoopDoSources0 + 242
19  com.apple.CoreFoundation        0x00007fff9474349f __CFRunLoopRun + 831
20  com.apple.CoreFoundation        0x00007fff94742f25 CFRunLoopRunSpecific + 309
21  com.apple.Foundation            0x00007fff8d0e1adc -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 253
22  com.simperium.Simperium-OSX     0x0000000107fce528 -[_SRRunLoopThread main] + 291
23  com.apple.Foundation            0x00007fff8d0df76b __NSThread__main__ + 1318
24  libsystem_pthread.dylib         0x00007fff907cc899 _pthread_body + 138
25  libsystem_pthread.dylib         0x00007fff907cc72a _pthread_start + 137
26  libsystem_pthread.dylib         0x00007fff907d0fc9 thread_start + 13

marianoabdala · 2014-05-08T14:04:54Z

Hi @jleandroperez, so have the crash reports stopped after applying this? Thanks!

jleandroperez · 2014-05-08T14:14:15Z

@marianoabdala So far i have not received any further crash reports. I'll keep you posted (if it happens ever again!).

Conflicts: SocketRocket/SRWebSocket.m Up to PR facebookincubator#169 merged

Conflicts: SocketRocket/SRWebSocket.m

lukeredpath · 2015-04-05T08:04:32Z

Any updates on getting this merged?

jtreanor · 2015-04-28T20:48:22Z

I am receiving crash reports just like these in my app. Is there any possibility of getting this merged?

frranck · 2015-04-29T08:26:47Z

Same here, why is it not merged ?

dgatwood · 2015-08-11T18:09:41Z

NSStream doesn't retain its delegate. If the code isn't retaining the delegate elsewhere, it will become a zombie. It is not safe to allow the delegate to go away until you completely tear down the stream. So if I'm reading the description of the "workaround" correctly, it isn't a workaround so much as a bug fix.

RoyalPineapple · 2015-09-19T00:06:27Z

SocketRocket.xcodeproj/project.xcworkspace/xcshareddata/SocketRocket.xccheckout

@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="UTF-8"?>


This file looks like it should be in your .gitignore. Please remove.

Sorry about that, removing right away!

mikelikespie · 2015-09-19T00:08:51Z

Sorry for the long delay. I'd rather avoid using @synchronized. Could we make it just dispatch that work on the _workQueue?

Thanks,
Mike

jleandroperez · 2015-09-21T18:45:28Z

On the contrary @mikelikespie, thanks for the review (+ for opensourcing this project, we use it quite a lot!).

I'm afraid that simply replacing the @synchronized blocks with a dispatch on the _workQueue might not do the trick, since we really need to execute the _cleanupSelfReference method on the same thread (/runloop) as the NSStream instances.

Perhaps we could switch to NSLock instead?.

Thank you sir!

mikelikespie · 2015-10-16T23:49:25Z

SocketRocket/SRWebSocket.m

+- (void)safeHandleEvent:(NSStreamEvent)eventCode stream:(NSStream *)aStream
+{
+    __strong typeof(self) strongSelf = self;
+    ((void)strongSelf);


Self is always strong. Don't need this.

You're right Mike. Pre-arc school says 'that's not accurate', but post arc school says otherwise. Thank you, updating =)

mikelikespie · 2015-10-16T23:54:57Z

@jleandroperez I feel like you could do most of it by dispatching on the _workQueue (at least making sure you only schedule the cleanup once). I think you can call close from any thread too.

jleandroperez · 2015-10-23T14:23:25Z

Hello there @mikelikespie!

I'm afraid that moving the cleanup code to the _workQueue might not do the trick, since we really need to execute the _cleanupSelfReference method on the same thread (/runloop) as the NSStream instances.

mikelikespie · 2015-10-31T00:12:42Z

@jleandroperez Can you elaborate on why _cleanupSelfReference needs to be called on the same thread please?

dfed · 2015-11-14T00:44:21Z

Ping @jleandroperez :)

jtreanor · 2015-11-15T19:44:38Z

I have described an alternative solution to this here: #156 (comment).

I'd love to hear any thoughts on it 😃

jleandroperez · 2015-11-16T13:25:55Z

Hello @mikelikespie and @dfed,

Apologies about the delay gentleman!. The reason why the cleanup sequence needs to be called on the same thread used by NSStream was kindly pointed out by @dgatwood up here.

NSStream's delegate property is not weak, but assign. Invalidating that pointer from elsewhere can explain the race condition we've been seeing (and the associated crashes).

That's the reason why, i'm afraid, moving the cleanup call back to the _workQueue queue neutralizes the fix.

I'll merge the conflicts shortly (and will address the strongSelf comment as well.

And again, thank you both for sharing this project!

jleandroperez · 2015-11-16T14:26:06Z

@mikelikespie ready for another round sir!. All comments have been addressed.

In the aims of making the reviewer's life happier, the safeHandleEvent method has an extra tab. (Otherwise, github diff has a very rough time spotting just the delta).

(If) this PR is approved, i'd be happy to remove that tab (just temporarily there).

@jtreanor i'm not a big fan of Singletons, but i'll definitely take a look at your solution. Sounds less invasive, thanks for sharing!

maxovtsin · 2016-02-11T11:43:14Z

Are you planning merge it? This request potentially can fix very popular crash.

dfed · 2016-02-17T02:08:02Z

Hey folks! Our turn to be slow. We've got a merge/reviewfest this Thursday and we'll take a look then.

jleandroperez · 2016-02-17T15:20:35Z

Thank you @dfed, much appreciated!

dfed · 2016-02-19T00:38:30Z

This looks reasonable to me. Thanks for the poke folks. Merging!

Issue #156: Prevents crash in handleEvent method

jleandroperez · 2016-02-19T00:43:11Z

Pure awesomeness, thanks a lot @dfed!!

facebookincubator#169

Issue facebookincubator#156: Prevents crash in handleEvent method

Prevents crash in handleEvent method

c02f775

This was referenced Apr 25, 2014

Crashing at -[SRWebSocket stream:handleEvent:] (SRWebSocket.m:1405) #156

Closed

[SRWebSocket stream:handleEvent:] crash Simperium/simperium-ios#259

Closed

Cloudef mentioned this pull request Apr 29, 2014

Fixed a crash when closing socket with nil delegate #167

Closed

Cleaning up NSStream delegates in SR-RunLoop

08f26e4

jleandroperez added 3 commits May 27, 2014 17:54

Prevents sending close x2

194bc17

Prevents multiple cleanup calls

7e5129f

Nukes redundant stream close call

b434079

taiyangc added a commit to quatanium/SocketRocket that referenced this pull request Jun 4, 2014

Merge branch 'master' of https://github.com/jleandroperez/SocketRocket

e65dd61

Conflicts: SocketRocket/SRWebSocket.m Up to PR facebookincubator#169 merged

jleandroperez added 8 commits July 10, 2014 17:56

Fixes Podspec source url

cb476d4

Fixed podspec

1bb4048

Merge remote-tracking branch 'upstream/master'

3244d81

Conflicts: SocketRocket/SRWebSocket.m

Merge remote-tracking branch 'original/master'

bcac6bd

Conflicts: SocketRocket/SRWebSocket.m

Prevents bad access on dealloc

ad7890a

Implements cleanup failsafe

04b9c1d

Merge remote-tracking branch 'upstream/master'

c13b301

Prevents concurrent runloop unschedule glitch

3ff6038

Igor-Palaguta mentioned this pull request Apr 3, 2015

Crash: [SRWebSocket _failWithError:]_block_invoke_2 raw pusher/libPusher#174

Closed

Merge remote-tracking branch 'upstream/master'

3c867d8

RoyalPineapple reviewed Sep 19, 2015
View reviewed changes

jleandroperez added 2 commits September 21, 2015 15:23

Removes SocketRocket.xccheckout

b4c58f8

Merge remote-tracking branch 'upstream/master'

8449f84

mikelikespie reviewed Oct 16, 2015
View reviewed changes

jleandroperez added 4 commits November 16, 2015 10:58

Merging upstream

2c95c13

Merge remote-tracking branch 'upstream/master'

b11f4be

Nukes strongSelf workaround

60cec3a

Re-applies scheduleCleanup patch

a770a5f

dfed added a commit that referenced this pull request Feb 19, 2016

Merge pull request #169 from jleandroperez/master

541f7b6

Issue #156: Prevents crash in handleEvent method

dfed merged commit 541f7b6 into facebookincubator:master Feb 19, 2016

nlutsenko mentioned this pull request May 12, 2016

Race condition that leads to a crash? #332

Closed

benshan pushed a commit to benshan/SocketRocket that referenced this pull request May 18, 2016

Manually merge in flattened form of PR 169 to fix crash in handleEvent.

c8c3c88

facebookincubator#169

madlymad pushed a commit to madlymad/SocketRocket that referenced this pull request Jul 6, 2017

Merge pull request facebookincubator#169 from jleandroperez/master

c209686

Issue facebookincubator#156: Prevents crash in handleEvent method

This was referenced Aug 28, 2020

fix(IoT): Backport crash fix into AWSSRWebSocket aws-amplify/aws-sdk-ios#2984

Merged

Fix overcommit crash in AWSSRWebSocket aws-amplify/aws-sdk-ios#2921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #156: Prevents crash in handleEvent method #169

Issue #156: Prevents crash in handleEvent method #169

jleandroperez commented Apr 25, 2014

marianoabdala commented May 8, 2014

jleandroperez commented May 8, 2014

lukeredpath commented Apr 5, 2015

jtreanor commented Apr 28, 2015

frranck commented Apr 29, 2015

dgatwood commented Aug 11, 2015

RoyalPineapple Sep 19, 2015

jleandroperez Sep 21, 2015

mikelikespie commented Sep 19, 2015

jleandroperez commented Sep 21, 2015

mikelikespie Oct 16, 2015

jleandroperez Nov 16, 2015

mikelikespie commented Oct 16, 2015

jleandroperez commented Oct 23, 2015

mikelikespie commented Oct 31, 2015

dfed commented Nov 14, 2015

jtreanor commented Nov 15, 2015

jleandroperez commented Nov 16, 2015

jleandroperez commented Nov 16, 2015

maxovtsin commented Feb 11, 2016

dfed commented Feb 17, 2016

jleandroperez commented Feb 17, 2016

dfed commented Feb 19, 2016

jleandroperez commented Feb 19, 2016

Issue #156: Prevents crash in handleEvent method #169

Issue #156: Prevents crash in handleEvent method #169

Conversation

jleandroperez commented Apr 25, 2014

marianoabdala commented May 8, 2014

jleandroperez commented May 8, 2014

lukeredpath commented Apr 5, 2015

jtreanor commented Apr 28, 2015

frranck commented Apr 29, 2015

dgatwood commented Aug 11, 2015

RoyalPineapple Sep 19, 2015

Choose a reason for hiding this comment

jleandroperez Sep 21, 2015

Choose a reason for hiding this comment

mikelikespie commented Sep 19, 2015

jleandroperez commented Sep 21, 2015

mikelikespie Oct 16, 2015

Choose a reason for hiding this comment

jleandroperez Nov 16, 2015

Choose a reason for hiding this comment

mikelikespie commented Oct 16, 2015

jleandroperez commented Oct 23, 2015

mikelikespie commented Oct 31, 2015

dfed commented Nov 14, 2015

jtreanor commented Nov 15, 2015

jleandroperez commented Nov 16, 2015

jleandroperez commented Nov 16, 2015

maxovtsin commented Feb 11, 2016

dfed commented Feb 17, 2016

jleandroperez commented Feb 17, 2016

dfed commented Feb 19, 2016

jleandroperez commented Feb 19, 2016