-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(android): Update V8 to 7.8.279.23 #11294
Conversation
New dependencies added: request-promise-nativeAuthor: Nicolai Kamenzky Description: The simplified HTTP request client 'request' with Promise support. Powered by native ES6 promises. Homepage: https://github.com/request/request-promise-native#readme
|
CreateDefaultPlatform() was deprecated and removed by v8
don't attempt to load core module if request begins with '.' or '/'
|
@@ -239,7 +240,7 @@ jobject TypeConverter::jsObjectToJavaFunction(Isolate* isolate, JNIEnv *env, Loc | |||
{ | |||
Local<Function> func = jsObject.As<Function>(); | |||
Persistent<Function, CopyablePersistentTraits<Function>> jsFunction(isolate, func); | |||
jsFunction.MarkIndependent(); | |||
// jsFunction.MarkIndependent(); // Method has been removed! |
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.
Do we need to do anything here?
If this is where we box anonymous JS callbacks, then we shouldn't make it a weak reference since it might get garbage collected before invoked on the Java side.
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 this is okay, it looks like we store a persistent reference to the JS function so it can't be garbage collected and release the reference when the Java function is destroyed Java_org_appcelerator_kroll_runtime_v8_V8Function_nativeRelease
// Set correct permissions for 'mksnapshot' | ||
await fs.chmod(MKSNAPSHOT_PATH, 0o755); | ||
// Obtain snapshot `id` and start new snapshot generation. | ||
const id = await request.post(SNAPSHOT_URL, { body: { v8, script }, json: 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.
If you don't have Internet access, then the await request.post()
will reject and throw an exception. Perhaps we should catch it, log the reject's error, and continue without the snapshot?
Although for Jenkins builds of our SDK (namely release builds), we don't want this behavior.
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.
We're catching the exception here, so this shouldn't be an issue.
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.
CR: Pass
FR Passed. Checked liveview, kitchensink & native modules 7.0.0+. Studio Ver: 6.0.0.201911251516 |
Can someone please resolve the conflict. |
JIRA: https://jira.appcelerator.org/browse/TIMOB-27178
Description:
This is a work in progress to update to the latest V8 builds we can. Current Android SDK is on top of V8 7.3.492.26.
This PR currently updates us to V8 7.8.279.23
Current stable android v8 is 7.8.279.23 via https://omahaproxy.appspot.com
What I did:
'/'
or'.'
. When I turned on some debug logging in the c++ code, I saw that it kept trying to load bindings for'.'
. This does mean that technically we'd "lose" the ability to load native modules starting with a'.'
, but I'm not even sure we allow that in module id naming.(Note that I also noticed a bug in the building of the require path used for add-on tests in the test suite where it was trying to load add-on test files with no
'/'
prefix, so it hit the "core" module code path and failed)