-
Notifications
You must be signed in to change notification settings - Fork 104
Fix NullPointerException when native class is not present #198
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
base: master
Are you sure you want to change the base?
Conversation
| { | ||
| var nativeClassStruct = UnityVersionHandler.Wrap((Il2CppClass*)Il2CppClassPointerStore<T>.NativeClassPtr); | ||
| if (!nativeClassStruct.HasFinalize) | ||
| if (!nativeClassStruct?.HasFinalize is true) |
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 think the ! shouldn't be there.
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.
!null is not true. if we get !false it is true like expected. Inversion operates only on bool values.
In this case:
- nativeClassStruct checked on null. If it is null - returns Nullable else gets property.
- Nullable cannot be inversed by operator
!. If property is present - it will be inversed - Comparison with true
But if you mean that the condition itself is false to begin with, then I'm not sure what that field points to.
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.
It seems to indicate the presence of a destructor. And if there is no destructor, then we add a reference to the object in GC to finalize it
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.
Oh, I initially meant is false
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.
if native class has finalize, we add it to GC, right?
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.
if (!nativeClassStruct.HasFinalize) should be changed to one of the following, and I'm not sure which:
if (nativeClassStruct?.HasFinalize is false)if (nativeClassStruct?.HasFinalize is false or null)
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 second one looks more correct. If I understood the logic correctly - if native class does not have destructor, we add a reference in GC. When native class pointer is null we also need to keep track of the created object. The problem may arise if a class without a pointer has a destructor
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.
@js6pak Which do you think it should be, or how would you suggest we address the problem?
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 real solution is to inject unstripped types, but for now we can just do nativeClassStruct != null && !nativeClassStruct.HasFinalize
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.
@ds5678 changed to first variant
|
@aceman1209 How did you encounter this null reference exception? Can you show us a stack trace? |
|
@ds5678 Sorry for the late reply I tested this on Miside. I think any game that doesn't use asset bundles will work. To get the error, you just need to try to load your asset bundle. Unity will load it successfully (trying to load it again will cause an exception that such a bundle is already loaded), but because of the further exception, we will not get an instance of the class. Stack trace. I used MelonLoader for it. |
|
@aceman1209 when I discussed this with @js6pak , we were concerned about two things:
|
|
@ds5678 If I understand correctly, metadata contains information on which unstripped classes are built. But because the required class is not used in the game code, it is not included in the metadata during compilation, since there are no references to it. Correct? |
If you remove "unstripped" from this sentence, everything you said in that message is correct. |
A part of this pull request.
Required more in-depth review. Probably unstripped classes do not have a pointer to the native class in Il2CppClassPointerStore context. With UnityEngine.AssetBundleModule.AssetBundle works fine, but i'm not sure about GC Finalize.