Fix crash for web socket in some race conditions #22439

zhongwuzw · 2018-11-28T02:19:23Z

This PR fixes a crash caused by a race condition when webSocket deallocated and NSStream delegate callback, because NSStream's delegate callback be called on RCTSR_networkRunLoop.

This PR mainly changes:

Remove unnecessary nil operation in dealloc method.
Add a new method _scheduleCleanUp to schedule webSocket cleanup also on RCTSR_networkRunLoop.
In stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode delegate method, add a wself to make safe further.

Test Plan:

Reproduce steps:
Run any react-native project, and keep it running, it would crash at some point.

Changelog:

[iOS] [BUGFIX] [RCTSRWebSocket] - Fix crash for web socket in some race conditions.

pull-bot · 2018-11-28T02:20:41Z

	Warnings
⚠️	📋 Changelog - This PR may have incorrectly formatted Changelog.

Generated by 🚫 dangerJS

kelset · 2018-11-28T09:09:52Z

Thanks for opening this new PR :)

cpojer · 2018-12-04T04:12:49Z

This PR unfortunately doesn't come with a reproducible test plan of the before/after behavior and the change itself is quite large without any form of automated testing. In order to land this, would you mind working both on the test plan and possibly adding some form of tests?

zhongwuzw · 2018-12-13T02:41:41Z

@cpojer Emm, I did try to find a way to test it, but I have no idea currently, because it's a race condition, RCTSRWebSocket is deallocating and another runloop calls its method.

zhongwuzw · 2018-12-13T08:16:09Z

Runloop callback the delegate method, but delegate object already released:

qingluanchou · 2018-12-16T03:07:24Z

[In environment Xcode10.1 react-native：0.55.4 react:16.3.1 ,I have modified RCTSRWebSocket.m and still crashes，What is the cause? @zhongwuzw

janicduplessis · 2018-12-17T22:08:20Z

I can confirm that I'm also seeing this crash in my app. No idea how to repro though.

OS Version: iOS 12.1 (16B92)
Report Version: 104

Exception Type: EXC_BAD_ACCESS (SIGBUS)
Exception Codes: BUS_NOOP at 0x0000000000000010
Crashed Thread: 13

Application Specific Information:
class > respondsToSelector: > stream:handleEvent: > XTUM >
Attempted to dereference garbage pointer 0x10.

Thread 13 Crashed:
0   libobjc.A.dylib                 0x3abb07d70         objc_msgSend
1   libobjc.A.dylib                 0x3abaebe7c         -[NSObject respondsToSelector:]
2   CoreFoundation                  0x3ad5aeed4         _inputStreamCallbackFunc
3   CoreFoundation                  0x3ad5db030         _signalEventSync
4   CoreFoundation                  0x3ad5d8134         _cfstream_solo_signalEventSync
5   CoreFoundation                  0x3ad5d7f14         _CFStreamSignalEvent
6   CFNetwork                       0x3ae3ade98         SocketStream::dispatchSignalFromSocketCallbackUnlocked(SocketStreamSignalHolder*)
7   CFNetwork                       0x3ae3b3768         SocketStream::socketCallback(__CFSocket*, unsigned long, __CFData const*, void const*)
8   CFNetwork                       0x3ae3af4a0         SocketStream::_SocketCallBack_stream(__CFSocket*, unsigned long, __CFData const*, void const*, void*)
9   CoreFoundation                  0x3ad5cf5a8         __CFSocketPerformV0
10  CoreFoundation                  0x3ad5c81cc         __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
11  CoreFoundation                  0x3ad5c814c         __CFRunLoopDoSource0
12  CoreFoundation                  0x3ad5c7a30         __CFRunLoopDoSources0
13  CoreFoundation                  0x3ad5c28fc         __CFRunLoopRun
14  CoreFoundation                  0x3ad5c21cc         CFRunLoopRunSpecific
15  Foundation                      0x3aea4a404         -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
16  th3rdwave                       0x202fe523c         _ZN5folly6detail8function14FunctionTraitsIKFbRKNS_7dynamicES5_EE10uninitCallERNS1_4DataES5_S5_
17  Foundation                      0x3aeb7d1ac         __NSThread__start__
18  libsystem_pthread.dylib         0x3acf7f2ac         _pthread_body
19  libsystem_pthread.dylib         0x3acf7f20c         _pthread_start

janicduplessis

Looks great overall, thanks for working on this. This code is pretty tricky so we need to be careful.

@cpojer The changes are actually less scary than I though, mostly just whitespace.

Were you able to test this in production to make sure the crash does not happen and that there are no regressions?

janicduplessis · 2018-12-17T22:11:06Z

Libraries/WebSocket/RCTSRWebSocket.m

+
+  // Cleanup NSStream's delegate in the same RunLoop used by the streams themselves:
+  // This way we'll prevent race conditions between handleEvent and SRWebsocket's dealloc
+  NSTimer *timer = [NSTimer timerWithTimeInterval:(0.0f) target:self selector:@selector(_cleanupSelfReference:) userInfo:nil repeats:NO];


Is there a reason why you can't use dispatch_async to schedule cleanup here?

@janicduplessis Emm, we need to schedule cleanup on RCTSR_networkRunLoop, seems we have no dispatch_queue here. Maybe we can use performSelector, but we can keep use timer, because they are the same actually.

janicduplessis · 2018-12-17T22:13:41Z

Libraries/WebSocket/RCTSRWebSocket.m

-  assert(_workQueue != NULL);
+
+  // _workQueue cannot be NULL
+  if (!_workQueue) { return; }


If work queue cannot be null can we just remove this?

@janicduplessis In theory, we don't need to check work queue nullable, the reason I add this is to add the further safe check, because if work queue is null, it would crash immediately.

janicduplessis · 2018-12-17T22:36:03Z

Libraries/WebSocket/RCTSRWebSocket.m

+- (void)_cleanupSelfReference:(NSTimer *)timer
+{
+  // Remove the streams, right now, from the networkRunLoop
+  [_inputStream close];


Should we check the stream status here before trying to close it like we do here https://github.com/facebook/react-native/blob/master/Libraries/WebSocket/RCTSRWebSocket.m#L1034

Might be worth extracting to a shared method to safely close streams.

@janicduplessis 👍 We can extract it to a separate method.

kelset · 2018-12-18T08:53:02Z

@janicduplessis we have been using this patch in our prod app for months now (more or less) and it's working super well for us.

And yes I don't have a way to repro either, it's a weird prod edge case.

zhongwuzw · 2018-12-20T03:07:22Z

[In environment Xcode10.1 react-native：0.55.4 react:16.3.1 ,I have modified RCTSRWebSocket.m and still crashes，What is the cause? @zhongwuzw

Emm, seems it's the apple's internal bug? maybe it's not related to this issue?

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-01-02T02:00:05Z

@zhongwuzw merged commit dd209bb into facebook:master.

Summary: Fixes facebook#21086. Fixes facebook#6117. This PR fixes a crash caused by a race condition when `webSocket` deallocated and `NSStream` delegate callback, because `NSStream`'s delegate callback be called on `RCTSR_networkRunLoop`. This PR mainly changes: * Remove unnecessary `nil` operation in `dealloc` method. * Add a new method `_scheduleCleanUp` to schedule `webSocket` cleanup also on `RCTSR_networkRunLoop`. * In `stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode` delegate method, add a `wself` to make safe further. Pull Request resolved: facebook#22439 Differential Revision: D13564247 Pulled By: cpojer fbshipit-source-id: 675c1b2805aa45c54d7708d796f5843ef7ea34e2

Summary: Fixes #21086. Fixes #6117. This PR fixes a crash caused by a race condition when `webSocket` deallocated and `NSStream` delegate callback, because `NSStream`'s delegate callback be called on `RCTSR_networkRunLoop`. This PR mainly changes: * Remove unnecessary `nil` operation in `dealloc` method. * Add a new method `_scheduleCleanUp` to schedule `webSocket` cleanup also on `RCTSR_networkRunLoop`. * In `stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode` delegate method, add a `wself` to make safe further. Pull Request resolved: #22439 Differential Revision: D13564247 Pulled By: cpojer fbshipit-source-id: 675c1b2805aa45c54d7708d796f5843ef7ea34e2

Summary: Fixes #21086. Fixes #6117. This PR fixes a crash caused by a race condition when `webSocket` deallocated and `NSStream` delegate callback, because `NSStream`'s delegate callback be called on `RCTSR_networkRunLoop`. This PR mainly changes: * Remove unnecessary `nil` operation in `dealloc` method. * Add a new method `_scheduleCleanUp` to schedule `webSocket` cleanup also on `RCTSR_networkRunLoop`. * In `stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode` delegate method, add a `wself` to make safe further. Pull Request resolved: facebook/react-native#22439 Differential Revision: D13564247 Pulled By: cpojer fbshipit-source-id: 675c1b2805aa45c54d7708d796f5843ef7ea34e2

Summary: Fixes facebook#21086. Fixes facebook#6117. This PR fixes a crash caused by a race condition when `webSocket` deallocated and `NSStream` delegate callback, because `NSStream`'s delegate callback be called on `RCTSR_networkRunLoop`. This PR mainly changes: * Remove unnecessary `nil` operation in `dealloc` method. * Add a new method `_scheduleCleanUp` to schedule `webSocket` cleanup also on `RCTSR_networkRunLoop`. * In `stream:(NSStream *)aStream handleEvent:(NSStreamEvent)eventCode` delegate method, add a `wself` to make safe further. Pull Request resolved: facebook#22439 Differential Revision: D13564247 Pulled By: cpojer fbshipit-source-id: 675c1b2805aa45c54d7708d796f5843ef7ea34e2

Fix crash for web socket in some race conditions

f592364

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 28, 2018

zhongwuzw mentioned this pull request Nov 28, 2018

Remove _workQueue of webSocket set to NULL which may leads to crash #19489

Closed

react-native-bot added ✅Release Notes labels Nov 28, 2018

hramos mentioned this pull request Dec 13, 2018

com.squareup.SocketRocket.NetworkThread(18): EXC_BAD_ACCESS #21086

Closed

3 tasks

janicduplessis suggested changes Dec 17, 2018

View reviewed changes

Combine close stream operation to one shared method

72f7777

Small code style fix.

c5ba42d

cpojer approved these changes Jan 2, 2019

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 2, 2019

facebook-github-bot approved these changes Jan 2, 2019

View reviewed changes

facebook-github-bot closed this in dd209bb Jan 2, 2019

facebook locked as resolved and limited conversation to collaborators Jan 2, 2019

react-native-bot added the Merged This PR has been merged. label Jan 2, 2019

zhongwuzw deleted the fix_webSocket_crash branch January 2, 2019 02:01

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

react-native-bot added the Bug label May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash for web socket in some race conditions #22439

Fix crash for web socket in some race conditions #22439

zhongwuzw commented Nov 28, 2018 •

edited

Loading

pull-bot commented Nov 28, 2018

kelset commented Nov 28, 2018

cpojer commented Dec 4, 2018

zhongwuzw commented Dec 13, 2018

zhongwuzw commented Dec 13, 2018

qingluanchou commented Dec 16, 2018 •

edited

Loading

janicduplessis commented Dec 17, 2018

janicduplessis left a comment

janicduplessis Dec 17, 2018

zhongwuzw Dec 20, 2018

janicduplessis Dec 17, 2018

zhongwuzw Dec 20, 2018

janicduplessis Dec 17, 2018

zhongwuzw Dec 20, 2018 •

edited

Loading

kelset commented Dec 18, 2018

zhongwuzw commented Dec 20, 2018

facebook-github-bot left a comment

react-native-bot commented Jan 2, 2019

Fix crash for web socket in some race conditions #22439

Fix crash for web socket in some race conditions #22439

Conversation

zhongwuzw commented Nov 28, 2018 • edited Loading

Test Plan:

Changelog:

pull-bot commented Nov 28, 2018

kelset commented Nov 28, 2018

cpojer commented Dec 4, 2018

zhongwuzw commented Dec 13, 2018

zhongwuzw commented Dec 13, 2018

qingluanchou commented Dec 16, 2018 • edited Loading

janicduplessis commented Dec 17, 2018

janicduplessis left a comment

Choose a reason for hiding this comment

janicduplessis Dec 17, 2018

Choose a reason for hiding this comment

zhongwuzw Dec 20, 2018

Choose a reason for hiding this comment

janicduplessis Dec 17, 2018

Choose a reason for hiding this comment

zhongwuzw Dec 20, 2018

Choose a reason for hiding this comment

janicduplessis Dec 17, 2018

Choose a reason for hiding this comment

zhongwuzw Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

kelset commented Dec 18, 2018

zhongwuzw commented Dec 20, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Jan 2, 2019

zhongwuzw commented Nov 28, 2018 •

edited

Loading

qingluanchou commented Dec 16, 2018 •

edited

Loading

zhongwuzw Dec 20, 2018 •

edited

Loading