Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CRASH] CoreFoundation _inputStreamCallbackFunc #773

Closed
nickoto opened this issue May 6, 2020 · 7 comments
Closed

[CRASH] CoreFoundation _inputStreamCallbackFunc #773

nickoto opened this issue May 6, 2020 · 7 comments

Comments

@nickoto
Copy link
Contributor

nickoto commented May 6, 2020

I did a little bit of investigation into this issue ( #676 ) since I encountered it. If the application releases the WebSocket instance before the disconnect is completed the FoundationTransport is released but it has not yet been stopped. So messages (from 'CFReadStream'/'CFWriteStream') to their delegates will still get fired and will result in this crash.

I'm not sure what the right solution is to the problem; however, deinit in FoundationTransport to do the right cleanup or ensuring that engine doesn't release the transport until it is finished stopping (since the stop is currently async, and only weakly holds onto self in the completion handler).

Otherwise the client needs to wait until the socket has been completely closed before releasing its instance (which would need docs -- but I'd much prefer it doesn't crash if you release the webSocket object).

A simple test for this problem:

Connect a websocket and then...

        webSocket.delegate = nil
        webSocket.disconnect()
        webSocket = nil
@shalyf
Copy link

shalyf commented Jun 2, 2020

I fixed it with these (#777) , thanks

deinit {
    if #available(iOS 12, *) { } else {
        if let mirror = Mirror(reflecting: self).superclassMirror {
            if let inputStream = mirror.descendant("engine", "transport", "inputStream") as? Stream {
                inputStream.delegate = nil
            }
            if let outputStream = mirror.descendant("engine", "transport", "outputStream") as? Stream {
                outputStream.delegate = nil
            }
        }
    }
}

@jaltreuter
Copy link

@shalyf that implementation seems very fragile. What's the reason behind all those extra checks?

Given that inputStream and outputStream are both private, and the implementation of disconnect() sets their delegates to nil without any check like you've done, I think it's safe to take the simpler approach I implemented in my #777 solution. I'm open to hearing other opinions though.

@shalyf
Copy link

shalyf commented Jun 5, 2020

@jaltreuter I have MyWebSocket inherited from WebSocket and implement deinit to fix this crash. My code is based on your solution, use Mirror to access private property, because I don't want to modify the framework. Before the author updates, this is my temporary code.

@aserdobintsev
Copy link

@shalyf thanks for the idea about a temporary solution, based on @jaltreuter PR 👍

@philfi
Copy link

philfi commented May 26, 2021

Just updated to 4.0.4 (which seems to have the #777 fix) as part of updating to a recent apollo-ios release and have started seeing this crash reported from our app. @jaltreuter can you confirm your fix resolved the issue for you? Just wondering if this issue still exists in the Starscream library or if its more likely an issue with how its being used within apollo-ios. Thanks in advance!

@jaltreuter
Copy link

@philfi yes I can confirm that's the case. I updated to 4.0.4 several months ago and haven't seen this crash since then.

@philfi
Copy link

philfi commented May 27, 2021

@jaltreuter Thanks for confirming. Will continue to dig into it on the apollo-ios side. Fwiw the issue does appear related to the migration from Starscream 3.1.1 to 4.0.4 as the crashes (reportedly) don't occur in apollo-ios releases that use Starscream 3.1.1 and the issue appears in the release that updated to 4.0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants