[Fix] swizzling not forwarding with apps that use UIApplicationDelegateAdaptor #1091
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
Add testAddsMissingSelectors to ensure we add all selectors we need to get events from iOS. The change made to setOneSignalDelegate was required to make it testable. This equality check is already done in setOneSignalUNDelegate so this is also helping match our logic between these.
Added one failing test for the case where forwardingTargetForSelector is not accounted for in swizzling. The most common case for this is a SwiftUI app with @UIApplicationDelegateAdaptor. Just added one test and fixed one case to keep this PR small and readable, will add the rest in a follow up commit.
iOS 9 selectors we only swizzle on iOS 9 since UIApplicationDelegateAdaptor requires a newer version of iOS.
This fixes a bug where an infinite loop happens when we call the origin implementation if delegates we swizzle if something attempts to set the delegate back to the original class. This would only happen if the class didn't have the target selector to begin with when we run this swizzle logic, but then it is called a 2nd time with same target class. We have a check in the setOneSignalDelegate and setOneSignalUNDelegate but if the app developer or another library changes the delegate then puts it back this scenario can happen. Added a test for this scenario, ensuring it fails and then passing after. This also fixes an issue in the last commit where this bug was causing tests after UIApplicationDelegateSwizzingTest to infinitely loop.
This test broke in the last commit since the test was relying on a 2nd call to injectSelector to unswizzle. This was intentionally done as we don't want app or other SDKs swapping delegates back and forth to cause OneSignal to no longer receive events. This forced me to rewrite the OneSignalUNUserNotificationCenterHelper used for tests to not reply on this behavior and in the process made much simpler. It is declarative of the methods to swap back and therefore much easier to understand.
Fix missing selector warns in this file from the last commit since.
Add test to ensure our swizzling still calls the original methods.
jkasten2
changed the title
emawby
approved these changes
May 24, 2022
18 tasks
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
Fix some
AppDelegatemethods not being called when the app uses@UIApplicationDelegateAdaptor. Includes a secondary fix for rare stack overflow bug due to duplicate implementations with swizzling theUNUserNotificationCenterDelegate.Details
Motivation
OneSignal should not cause side effects to AppDelegate events. The following are not firing when
@UIApplicationDelegateAdaptoris used:application(_:didReceiveRemoteNotification:fetchCompletionHandler:)application(_:didFailToRegisterForRemoteNotificationsWithError:)application(_:didRegisterForRemoteNotificationsWithDeviceToken:)applicationWillTerminate(_:)The motivation to fix the stack overflow bug due to double swizzling in this PR is that it was required to address to write tests. It's possible to run into in an app setup too, however it's questionable if such a situation would happen.
Scope
Effects swizzling logic, includes double swizzling detection to prevent infinite loops for both
AppDelegateandUNUserNotificationCenterDelegate. Also calling original implementation ofAppDelegateselectors noted above in the Motivation section.Related Issues
fixes #1045
Apple's @UIApplicationDelegateAdaptor and SwiftUI.AppDelegate
The inter workings of
@UIApplicationDelegateAdaptoris unknown it was discovered that theAppDelegateset is an privateSwiftUI.AppDelegateclass. This class doesn't include selectors for the events noted above in the Motivation section. When these selectors are added to this class via our swizzling code the methods on the app's@UIApplicationDelegateAdaptorinstance are no longer called. We discovered however that callingforwardingTargetForSelector:onSwiftUI.AppDelegatereturns the instance of@UIApplicationDelegateAdaptorset by the app. We ended up using this as a fallback ifrespondsToSelector:is false.Follow up PR, use SwizzlingForwarder consistency
This PR added a new
SwizzlingForwarderclass to make sure we forward the event to prevent side effects. However this PR did not add this toUNUserNotificationCenterDelegateto keep this scope down.Testing
Unit testing
Manual testing
Tested on Xcode 13.3.1 on an iOS simulator and an iPhone 6s with iOS 14.4.1.
Tested with Objective-C and Swift with SwiftUI.
Tested to ensure
didFailToRegisterForRemoteNotificationsWithError:anddidRegisterForRemoteNotificationsWithDeviceToken:fire in the app's AppDelegate.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is