Bug and crash fixes #1784
Merged
Bug and crash fixes #1784
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
The previous cmake code accidentally set the INTERFACE_INCLUDE_DIRECTORIES property to the string "NATIVES_BLOB_INCLUDE_DIRECTORIES". We actually want the value of the variable. (I don't think this affects anything in the current build, but it complains that INTERFACE_INCLUDE_DIRECTORIES contains a relative path if you try to link anything else against libNativeScript.so.)
In the case where a Java exception is thrown from a JNI call, wrapped into a C++ NativeScriptException, and caught by C++ code that re-throws it to Java, we'd get a crash. (Admittedly, this does not happen very much. Initializing the ObjectManager is one of the few places where it can happen.)
Running the worker tests would occasionally hit a crash where the keywords set contained corrupt data. I believe this is because the keywords.empty() check was being hit in two different threads. We can supply the keywords in an initializer list so that the set is fully populated during static initialization.
Unlikely, but the ArgConverter calls at the beginning of Runtime::Init() could theoretically throw a Java exception, in which case they'd be wrapped into a C++ NativeScriptException and then re-thrown to Java. For that to work, NativeScriptException has to be initialized.
Particularly in NativeScriptException::ReThrowToJava(), we may need to call GetClassName before the ObjectManager is initialized. This would cause a crash when handling exceptions early in the NativeScript runtime's initialization process. Luckily, GetClassName doesn't actually need to be part of ObjectManager, it can be moved to JEnv instead.
Giving a wrong path to DexFactory can result in folders unexpectedly being wiped. Add a safety check to DexFactory.purgeDexesByThumb() to make sure we are only deleting files with the expected extensions.
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
This PR contains a few bug fixes for bugs which admittedly don't happen very often.
Recommend reviewing commit by commit.
Does your pull request have unit tests?
Testing most of the PR would depend on being able to simulate a failure early in runtime initialization - we don't currently have the infrastructure for this. Having C++ tests might help in the future.