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 swizzling subclass of already swizzled class #1284

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Jul 26, 2023

Description

One Line Summary

Fixing an issue where OneSignal would swizzle a subclass of an already swizzled class

Details

Fixes #1272
Fixes #1143
The bug is only reproducible with both Instabug and Firebase libraries.
The crucial factor here is that Google's libraries create GULApplicationDelegate classes which subclass the App's AppDelegate class.
OneSignal swizzles AppDelegate on load, then the Google libraries set their GULApplicationDelegate classes as the application's delegate, which triggers OneSignal to swizzle again.
This was not creating an issue with only firebase, but when also including Instabug there was an infinite loop. This is because when we swizzle the second time, the original implementation already includes Instabug calling OneSignal's AppDelegate. This means that our new implementation will include calling Instabug, which calls OneSignal, which calls Instabug, etc.

Motivation

Fixes infinite loop issues with other libraries

Scope

AppDelegate swizzling

Testing

Unit testing

New unit test to detect this case

Manual testing

I tested by integrating both Instabug and Firebase. The bug is only reproducible with both of those libraries. I tested on the branch named "instabug_conflict" if reviewers would like to manually test with that integration setup.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@emawby emawby requested a review from jkasten2 July 26, 2023 23:28
@emawby emawby force-pushed the Fix/swizzling_subclass_of_already_swizzled_class branch from 23236c5 to c692754 Compare July 26, 2023 23:41
@emawby emawby merged commit 789b339 into main Jul 26, 2023
2 checks passed
@emawby emawby deleted the Fix/swizzling_subclass_of_already_swizzled_class branch July 26, 2023 23:48
@emawby emawby mentioned this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants