-
-
Notifications
You must be signed in to change notification settings - Fork 342
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: Crash when Getting Subclasses #1396
Conversation
Calling class_getSuperclass sometimes leads to EXC_I386_GPFLT. OneSignal had the same issue OneSignal/OneSignal-iOS-SDK#278 and it was fixed with OneSignal/OneSignal-iOS-SDK#301. This PR applies the changes they did to our code to fix the problem. Fixes GH-1395
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm, just not quite sure about the logic
superClass = class_getSuperclass(superClass); | ||
} while (superClass && superClass != parentClass); | ||
} | ||
|
||
if (superClass != nil) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering when/if this condition can ever be reached?
According to the while loop above, superClass
either needs to be nil
or equal to parentClass
to go over that. And above, you explicitly excluded the parentClass
case, so I’m unsure what the code is supposed to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parentClass was excluded before the loop. But using class_getSuperclass
we can get the parentClass
again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @Swatinem. class_getSuperclass
returns nil
if the passed in class is a root class, like NSObject
. Therefore, we need this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, what if we do the class image check here in the background minimizing the amount of classes in the indexesToSwizzle
reducing code to run in the main thread later.
I'm not sure if I follow you here @brustolin. We do everything on a background thread except the swizzling. |
Codecov Report
@@ Coverage Diff @@
## master #1396 +/- ##
==========================================
- Coverage 96.05% 95.86% -0.20%
==========================================
Files 152 152
Lines 6747 6764 +17
==========================================
+ Hits 6481 6484 +3
- Misses 266 280 +14
Continue to review full report at Codecov.
|
📜 Description
Calling class_getSuperclass sometimes leads to EXC_I386_GPFLT. OneSignal
had the same issue OneSignal/OneSignal-iOS-SDK#278
and it was fixed with OneSignal/OneSignal-iOS-SDK#301.
This PR applies the changes they did to our code to fix the problem.
💡 Motivation and Context
Fixes GH-1395
💚 How did you test it?
Waiting for validation in GH-1395
📝 Checklist
🔮 Next steps