Fix crashes when using Aspects, jsPatch or waxPatch…

[doggy]

…whom hook functions with forwardInvocation:.

Forwarded PR

In common case, it's efficiently if we preserving the implementation of forwardInvocation: in rac_signalForSelector: and using it later.

But the preserved IMP should be updated if it has been replaced after calling rac_signalForSelector:.

Holding and invoke the old implementation may leads to an unexpected situation if an function added with jsPatch or waxPatch. (infinite loops, or crashes)

Library such as Aspects, jsPatch or waxPatch will swizzle forwardInvocation: at any time during the app running, probably.

P.S. This is an issue reported at jsPatch's issue list
And I decided to make this PR to RAC after reviewed all related codes between RAC and jsPatch.

[andersio]

Have you deployed this in development, and tried it with instances that has been isa-swizzled e.g. KVO? Would you mind to write a reduced test case for this?

I am not quite sure if the implementation is correct, since [self class] could be a dynamic subclass created by RAC, which results in the newForwardInvocation IMP itself being retrieved.

[doggy]

Yup, I know the KVO logic that also created a dynamic subclass on the fly.
This patch has been tested in our app project (with waxPatch, jsPatch and KVO logic applied).
I have not encountering any issue(or any performance slow-down) with it and it should be applied in out next release.

Okay, I will commit a new test case to show you the issue later.

[doggy]

I am not quite sure if the implementation is correct, since [self class] could be a dynamic subclass created by RAC, which results in the newForwardInvocation IMP itself being retrieved.

In my opinion, objc_getClass(self) returns the dynamic subclass created by RAC.
But [self class] is not. The method class of each dynamic subclass has been swizzed to return the original class.

I will double check with it later.

[doggy]

I checked NSObject+RACDescription.m again and I doubt that [self class] returns the original class rather than 'dynamic subclass'.

The logic exists in RACSwizzleMethodSignatureForSelector() already.
Both KVO and RAC dynamic subclass comply with it.

Unless [self class] has been swizzled by others again.... really creepy

[doggy]

@andersio Test case committed. Protection logic also committed.

[andersio]

Oh yeah, just checked again the RAC implementation - [self class] is swizzled to return the original subclass like how KVO does its swizzling. Sorry for missing the bits.

[andersio]

Got another question though. AFAIU this solves the clash in swizzled forwardInvocations of the frameworks. What about the IMP swizzling? Is that a problem in practice? (Perhaps not, assuming you guys do all the swizzling at launch time before rac_signalForSelector is used)

[doggy]

do all the swizzling at launch time before rac_signalForSelector is used
This scenario you mentioned is fine even without this PR.

AFAIK, ObjC IMP swizzling was handled by objc-runtime directly. All hot-patch SDKs can take care of it very well.

The main purpose of this PR fix the app crash when we re-implement an IMP function after rac_signalForSelector: is called. (similar to the newly created test case)
It fix the logic in RACSwizzleForwardInvocation and it won't affect pure ObjC IMP swizzling logic... harmless at least

In practice, I have tested PR in our app project (with waxPatch, jsPatch and KVO logic applied). (locally tested, not yet submitted to AppStore)
Briefly, Hot-patch now can be applying disorderly and NO MORE CRASH with this PR. :)

---

Hot-patch is used to fix small(really/) issues without re-submit the app. It overrides the OC function by a script function. (jsPatch using javascript, or waxPatch using lua)

It's a little bit tricky:

1. Firstly, app downloads a script file(.js or .lua) from internet which contains the functions which will override native IMP functions.
2. Then, analyse the file and re-implement all script functions with forwardInvocation:
3. Lastly, re-route native IMP functions to _objc_msgForward

After all, each re-routed native IMP function send to the object will be mapping to the script function.

There is the main process of hot-patch~

[doggy]

@andersio
I have deployed this patch in our develop environment several days.

[andersio]

Mind to replicate the test case for the case where the intercepted selector by rac_signalForSelector, i.e. lifeIsGood:, is also the one being hot-patched?

[doggy]

Oops, I missed it.
The short answer is NO, currently.

Hot-patch implements forwardInvocation: on original class. But the calling process of lifeIsGood: will be intercepted by RACForwardInvocation on dynamic subclass:

BOOL matched = RACForwardInvocation(self, invocation);
if (matched) return;

Good news is, it can be identified by checking (aliasSelector on dynamic subclass != originalSelector on original class) to know whether it has been hot-patched (same situation with method swizzle) or not.

One more step forward, we can just checking (originalSelector on original class == _objc_msgForward) to know it. But I don't recommend it.

[doggy]

It's done.

[doggy]

@andersio IMP-swizzling supported.
Patching/swizzling to a selector which already created and held an alias selector now should working.
New test cases committed, also.

CI reported a build error... what's going on?

Looks like a bug of CI, the xcodebuild did not started on arch x86_64~

[doggy]

@andersio CI Rebuilt finished.

... no code changed

[andersio]

Thank you for all the information and the test cases, which do help as I am interested in searching for a potentially cleaner approach.

/cc @ReactiveCocoa/reactiveobjc

Not approving yet.

[doggy]

Thanks for your advice and time :)

[andersio]

Just found that it had been brought up before, and had been well explained why it wasn't addressed. With all the attempts I've done so far, I don't feel the situation has changed.

I would let others determine if this should be fixed.

Meanwhile, the advice is either avoiding rac_signalForSelector completely, or at least having all swizzling/patching done before using it.

@mdiep @ReactiveCocoa/reactivecocoa

[mdiep]

I am definitely not a swizzling expert. I think fixing it is great if we can, but we should be confident that we haven't broken anything.

[andersio]

@doggy Mind to try #33? I had taken a quick look at jsPatch, and it appears to do IMP swizzling only. #33 would be fine in this case.

Interoperability with Aspects is definitely a no-go though.

[doggy]

Sure, maybe few days later?
I am busy in these days.

[andersio]

Sure, thanks!

[doggy]

Apple sent us a warning mail about hot-patch More discusse in other repo

The patch logic testing is meaningless since it is disallowed by Apple.

But I will test if this logic working with Aspects

[andersio]

Well perhaps it still worths a test in terms of interoperability. That's said I do not expect it to be sufficient with work only on RAC's end.