Fix crash for web socket in some race conditions by zhongwuzw · Pull Request #22439 · react/react-native

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

+
+ // 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

- 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

+- (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: Fixesreact#21086. Fixesreact#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: react#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: react/react-native#22439 Differential Revision: D13564247 Pulled By: cpojer fbshipit-source-id: 675c1b2805aa45c54d7708d796f5843ef7ea34e2

Summary: Fixesreact#21086. Fixesreact#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: react#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 dd209bbJan 2, 2019

react 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

Uh oh!

Conversation

zhongwuzw commented Nov 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan:

Changelog:

Uh oh!

pull-bot commented Nov 28, 2018

Uh oh!

kelset commented Nov 28, 2018

Uh oh!

cpojer commented Dec 4, 2018

Uh oh!

zhongwuzw commented Dec 13, 2018

Uh oh!

zhongwuzw commented Dec 13, 2018

Uh oh!

qingluanchou commented Dec 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janicduplessis commented Dec 17, 2018

Uh oh!

janicduplessis left a comment

Choose a reason for hiding this comment

Uh oh!

janicduplessisDec 17, 2018

Choose a reason for hiding this comment

Uh oh!

zhongwuzwDec 20, 2018

Choose a reason for hiding this comment

Uh oh!

janicduplessisDec 17, 2018

Choose a reason for hiding this comment

Uh oh!

zhongwuzwDec 20, 2018

Choose a reason for hiding this comment

Uh oh!

janicduplessisDec 17, 2018

Choose a reason for hiding this comment

Uh oh!

zhongwuzwDec 20, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelset commented Dec 18, 2018

Uh oh!

zhongwuzw commented Dec 20, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

react-native-bot commented Jan 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

zhongwuzw commented Nov 28, 2018•
edited
Loading

qingluanchou commented Dec 16, 2018•
edited
Loading

zhongwuzwDec 20, 2018•
edited
Loading