Skip to content
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

[TIMOB-23903] Build against V8 5.7.492.71 #8382

Merged
merged 30 commits into from
May 23, 2017

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Sep 14, 2016

https://jira.appcelerator.org/browse/TIMOB-23903

This updates Android's runtime to build against V8 5.7.492.71

V8 introduced yet more breaking API changes and I was unable to keep that from propagating out to our native modules. So this is a BREAKING NATIVE MODULE CHANGE!

Specifically, V8 removed WeakCallbackData and moved to WeakCallbackInfo. That was a minor update and I think could have avoided breaking module compatibility.

The larger change they made was to restrict values you can set as properties on a Template. We were setting actual Function objects on the prototype and instance templates for the types - we needed to change to setting the FunctionTemplate itself, not the function it generated. Secondly, it meant we could set an External wrapping the class (java class in JNI) that the proxy represented. This was used to instantiate instances of the Java class itself when constructing a proxy object in JS-world. To workaround this, instead of explicitly setting that External as the value for javaClass, I now set a "native data property" for that property name with a getter callback. This means it's a property that always goes straight to the native callback (can't be overridden in JS). And the data value we set with it gets passed to every callback invocation, so we use that as a sneaky "hack" to pass along the External wrapping the java class. The callback method simply grabs that value and returns it.

Relates to tidev/v8_titanium#12

@sgtcoolguy sgtcoolguy added this to the 7.0.0 milestone Sep 14, 2016
@sgtcoolguy sgtcoolguy changed the title Build against V8 5.3.332.41 [TIMOB-23903] Build against V8 5.3.332.41 Sep 14, 2016
@sgtcoolguy sgtcoolguy changed the title [TIMOB-23903] Build against V8 5.3.332.41 [TIMOB-23903] Build against V8 5.7.492.71 Apr 7, 2017
Local<Function> constructor;
MaybeLocal<Function> maybeConstructor = pt->GetFunction(context);
if (!maybeConstructor.ToLocal(&constructor)) {
V8Util::fatalException(isolate, tryCatch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

titanium::V8Util::fatalException(isolate, tryCatch);

MaybeLocal<Object> maybeInstance = constructor->NewInstance(context);
Local<Object> moduleInstance;
if (!maybeInstance.ToLocal(&moduleInstance)) {
V8Util::fatalException(isolate, tryCatch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

titanium::V8Util::fatalException(isolate, tryCatch);

…lates, so the Set*Method calls needed to set the generated FunctionTemplate (and not it's function). Also need to hack around hanging an External wrapping a jclass on the __javaClass_ property by using a natiev data property getter callback that gets the External passed as Data and returning that.
…gher version/moduleApiVersion against a build of the SDK
…mplate must be throwing some uncaught excpetion, so the toLocalChecked() on GetFunction is failing.
…with a proxy that we then go back and kill the proxy/JS object on the C++ side. Tehre's still an issue here where we're not cleaning up jclass references somehow that I need to fix...
Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 👍

CR: PASS
FT: PASS

Although we may need to use 5.7.54.0.

NOTE: These changes are also valid for 5.9

@sgtcoolguy
Copy link
Contributor Author

@garymathews Given that they've moved away from the older debug protocol, I think e should look into simply updating studio's debugger for Android to work with the new inspector protocol so we can update V8 to latest. It shouldn't be too bad since I already had to write the inspector protocol plus extra layers for iOS/JSCore.

Here's the studio ticket for updating the Android debugger (again!): https://jira.appcelerator.org/browse/TISTUD-8750

@sgtcoolguy sgtcoolguy merged commit 193e13e into tidev:master May 23, 2017
@sgtcoolguy sgtcoolguy deleted the v8-5.3.332.41 branch May 23, 2017 18:58
@hansemannn
Copy link
Collaborator

@sgtcoolguy So it's no breaking change? Otherwise, we cannot cause all native modules on Github and our marketplace to be recompiled again, especially not in a non-major release version :(.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants