[Change] Omit objc_getClassList calls from swizzling #1099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have logic in our swizzling checking for parent classes however there were no tests covering this. testAppDelegateInheritsFromBaseWhereBothHaveSelectorsButSuperIsNotCalled is a test that was added here that fails, uncovers a bug with our swizzling! The issue is injectToProperClass takes in a list of child classes however it only swizzles the first one instead of all of them, which we my swizzle one that isn't even in use. However this raises the question why did we not keep it simple and just swizzle the passed in delegate instance? That will be something to solve in the next commit!
The class that was passed in is the only thing that needs to be swizzled, the checks to try to swizzle the parent class or child classes was found to be unnecessary. In the last commit a number of tests starting with testAppDelegateInheritsFromBase* were added to cover all possible inheritance scenarios and these still pass after making this change. testAppDelegateInheritsFromBaseWhereBothHaveSelectorsButSuperIsNotCalled was one test that was actually failing but now passes after this change. Research was done to find the reason behind getClassWithProtocolInHierarchy to see why it was added incase I overlooked something. The root of the change was found in the following: GameThrive/ios-push-plugin@e656e99#diff-7ebd209501fe17a14bf53eaa7a246d9fe9f7b4b1d5139cdd2eb672fb3ae2fc29R900 Unfortunately the details are light, it notes that "...a class that inherits from your AppDelegate class", however we have added a unit test for this and things are working as expected. Since this was added ~8 years ago so many things have change both in this code base as well as iOS itself the original cause my no longer be an issue. Since getClassWithProtocolInHierarchy was removed we no longer need ClassGetSubclasses. This is because [delegate class] is the concrete class.
The last commit removed the usage of getClassWithProtocolInHierarchy and ClassGetSubclasses in swizzling and this PR is cleaning up the code itself. Was also able to remove checkIfInstanceOverridesSelector, since it was only used in relation to these methods. injectToProperClass can be removed, however will do so in a different comment since this will touch a number of files.
Since the last commit no more rule is needed to for sub classes so injectSelector was replace everywhere in the code base. The parameters were reordered on injectSelector to make it more readable. Starting with the original class and selector then followed by the replacement class and selector is easier see what is targeting and what it is changing to.
jkasten2
force-pushed
the
omit_objc_getClassList_calls_for_swizzling
branch
from
a44350b
to
cde72e0
Compare
emawby
approved these changes
Jun 2, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Remove calls to
objc_getClassListfrom swizzling code as it creates issues with other libraries as it can trigger+initializeto be called before+loadfor some classes.Details
Motivation
Fixes compatibility with Kotlin pre-1.7.0 (issue #1042) as well as fixes another similar crash. This also should improve startup time, by preventing the SDK from loading all classes at once on the main thread, issue #307.
Scope
This effect which classes are swizzled, implementation has been simplified to swizzle only the class of the instance passed in, instead of trying look at the parent and sub classes.
Testing
Unit testing
Added new tests for inheritance cases to ensure the simplified logic can still cover all cases.
Manual testing
Tested on an iPhone 6s with iOS 14.4.1
Tested the the following projects:
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is