Fix a few idempotency and use-after-free issues #15
Conversation
- calls into the Java Context APIs directly (no temporary wrapper objects whose destructors free the ref we still need),
- stores a fresh global ref for the application context, and
- re-resolves the AssetManager, deleting the local reference afterwards.
… all kinds of call patterns
Include/AndroidExtensions/Globals.h
Outdated
| // Passing nullptr for appContext/assetManager clears cached globals so embedders can | ||
| // explicitly tear down between reloads, or supply only an asset manager when no stable | ||
| // Java Context is available (e.g., headless/unit-test harnesses). | ||
| void Initialize(JavaVM* javaVM, jobject appContext, jobject assetManager); |
There was a problem hiding this comment.
With the new usage, we call Initialize with nullptr to tear down. This seems awkward. Maybe it be better to create a separate function called Uninitialize?
| if (applicationContext != nullptr) | ||
| { |
There was a problem hiding this comment.
When is it valid for someone to pass in a non-null context but applicationContext is null? If this happened, wouldn't GetAppContext return null later and cause a problem? Or maybe we are saying GetAppContext can return null now? I couldn't find any official documentation on whether getApplicationContext can return null.
| android::content::Context contextWrapper{context}; | ||
| const auto applicationContextWrapper = contextWrapper.getApplicationContext(); | ||
| jobject applicationContext = applicationContextWrapper; |
There was a problem hiding this comment.
The point of the wrapper objects is to hide the details of interacting with Java to make it easier. Saving all the wrappers in a separate object and using the jobjects directly makes the code awkward. If we actually must check for the existence of the applicationContext object, we can add an implicit boolean operator to avoid having to save the wrapper in a different variable.
| android::content::Context contextWrapper{context}; | ||
| const auto assetsWrapper = contextWrapper.getAssets(); | ||
| SetAssetManager(static_cast<jobject>(assetsWrapper)); |
There was a problem hiding this comment.
Is this different than what you have?
| android::content::Context contextWrapper{context}; | |
| const auto assetsWrapper = contextWrapper.getAssets(); | |
| SetAssetManager(static_cast<jobject>(assetsWrapper)); | |
| SetAssetManager(android::content::Context{context}.getAssets()); |
Co-authored-by: Gary Hsu <[email protected]>
Co-authored-by: Gary Hsu <[email protected]>
2 participants
Found when running BabylonNative unit tests in Android simulator using NDK 28c, which automatically enables the
fdsansanitizer in clang. This was may have been generally causing instability during process exit on Android.Will take out of draft when I'm done integration testing in BabylonNative.