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

Fixed crash by adding a nil-pointer check. #1098

Merged
merged 2 commits into from Apr 26, 2021

Conversation

mikepulaski
Copy link
Contributor

Calling [ARTRealtime transitionSideEffects:] while in a failed connection state returns a nil reference to ARTEventListener*.

This PR adds a nil-pointer check to avoid a crash, which fixes #1083.

@ricardopereira
Copy link
Contributor

Hi @mikepulaski, thank you for your contribution. About your changes, in my opinion, it doesn't need a nil-pointer check because Objective-C won't produce a crash when the listener reference is nil.

@mikepulaski
Copy link
Contributor Author

Hi @mikepulaski, thank you for your contribution. About your changes, in my opinion, it doesn't need a nil-pointer check because Objective-C won't produce a crash when the listener reference is nil.

@ricardopereira Yeah, I think you're right because we still get the crash I was trying to fix with this :)

I suspect a similar fix is probably needed, though. I suspect the __weak _eventHandler reference in ARTEventListener is nil when trying to start the timer, as the timer depends on its queue. It might make sense to make a strong reference to it and checking for nil wherever it's used so that it doesn't unexpectedly become nil after passing a nil check.

Something like:

- (void)startTimer {
    ARTEventEmitter *eventHandler = _eventHandler;
    if (eventHandler == nil) {
        return;
    }
    
    // ...
}

@ricardopereira
Copy link
Contributor

@mikepulaski Did you tried the tentative fix from #1089?

@mikepulaski
Copy link
Contributor Author

mikepulaski commented Feb 4, 2021

@mikepulaski Did you tried the tentative fix from #1089?

@ricardopereira Nope! It's not something we've been able to reproduce ourselves, unfortunately. We have quite a few people running our app daily so we see some weird edge cases pop up in the wild. This one is our most frequent, unfortunately.

It looks like your fix is more or less what I was suggesting, but a potential problem is that a nil-check alone wouldn't fix the crash if it happened to dealloc on another queue/thread while this code block is running, which is possible since it's a weak reference. That's why I thought making a local strong reference before checking if it's nil might do the trick.

@QuintinWillison
Copy link
Contributor

I'm hoping to attend to this soon. See my comment on your other pull request.

@QuintinWillison
Copy link
Contributor

@mikepulaski Sorry for the delay. Is there any chance you could rebase atop our updated main branch?
Now that #1103 has landed I'm hoping that this pull request will pass green in CI.
FYI, @SpencerCDixon.

@QuintinWillison QuintinWillison merged commit eb87043 into ably:main Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Common crash related to websocket failing with error
3 participants