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

Fix DelegateProxy crash by implementing methodSignatureForSelector #2546

Merged
merged 3 commits into from Oct 4, 2023

Conversation

sejal-hotstar
Copy link
Contributor

No description provided.

@yan-zaitsev-hs
Copy link

yan-zaitsev-hs commented Sep 29, 2023

Solution

me and @sejal-hotstar investigated the crash reported in #2428, we found that there is a bug in _RXDelegateProxy implementation. The issue arises when non-delegate methods are invoked on the DelegateProxy, which is not originally designed to handle them. When such non-delegate methods are called, and the corresponding method signature is not properly defined, the _RXDelegateProxy returns nil, thereby leading to a crash. To address this, it's essential to override both methodSignatureForSelector and forwardInvocation to handle methods that the object doesn't itself recognise.


Few more details

DelegateProxy works fully correctly when handling only the methods defined in related Delegate protocols.
These methods are handled by DelegateProxy itself or forwarded to _forwardToDelegate object.

But what happens when I try to call method that is not defined by any delegate protocol, but is implemented by native delegate object?

In our case with that text scenario it was UITableViewCell with horizontal collection view inside of it.
collectionView.delegate = cell.
UITableViewCell implements the text property natively (it is deprecated).

And there is some new Apple internal framework that was introduces in iOS 15.6, that probably relates to accessability features. This framework iterate over each element on the screen (and its delegate methods) to find string by using objc runtime and method responseToSelector: Selector("text").
DelegateProxy return true for Selector("text") because UITableViewCell has this method.
But when objc runtime trying to call this method, it first invokes the methodSignatureForSelector to fine the signature with list of argument types, return type, etc.
But, DelegateProxy does not know any signature for method text. It only knows the signatures for delegate protocol methods and return nil for this request. It is leading to crash.
The fix adding the second call of methodSignatureForSelector to try get the signature from the _forwardToDelegate instance.


Overally, this issue can be easily found if text selector (or any other non-delegate selector) is called by developer and app immediately crash.
But this method is called by internal Apple framework. We was correctly debug it, because we was able to reproduce the issue on iOS 17 / Xcode 15 / real device.

cc @SlashDevSlashGnoll @freak4pc

@mkj-is
Copy link
Contributor

mkj-is commented Oct 2, 2023

@yan-zaitsev-hs @sejal-hotstar Great find! Should we revert the patch introduced in #2445?

Can we write a short test to verify this is no longer crashing? By calling those methods in the order we expect them to be called by internal Apple frameworks.

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I'm also wondering if we should revert the other fix or it's "fine" to leave both. Any thoughts?

RxCocoa/Runtime/_RXDelegateProxy.m Outdated Show resolved Hide resolved
@freak4pc freak4pc merged commit 473b7a7 into ReactiveX:main Oct 4, 2023
4 checks passed
@freak4pc
Copy link
Member

freak4pc commented Oct 4, 2023

I'll try to cut a release later tonight 💪

@ggiri0630
Copy link

It appears that 'Sources/RxCocoa/RxDelegateProxyCrashFix.swift' is referencing a file that has been deleted in this PR. Can you please confirm?

@Arlindo-g
Copy link

I'll try to cut a release later tonight 💪

@freak4pc nudging this verrrrrrrrrry slightly. Any way this is going out in a release within the next week or so?

@freak4pc
Copy link
Member

I'm sorry, we've been through many sleepless nights these past few weeks and I haven't been able to get much work done. I'll do my best to get to this soon.

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

Successfully merging this pull request may close these issues.

None yet

7 participants