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

fix(android): use C++14 when building native modules #12481

Merged
merged 7 commits into from Feb 24, 2021

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Feb 18, 2021

  • Use c++14 when building native modules
  • Update V8 to 8.8.278.17 that includes compatibility patch to revert v8/v8@50ddb12
NOTE: We should check modules built with TiSDK 10.0 work on TiSDK 9.3.X

@build
Copy link
Contributor

build commented Feb 18, 2021

Fails
🚫 Tests have failed, see below for more information.
🚫

Test suite crashed on iOS simulator. Please see the crash log for more details.

Warnings
⚠️ There is no linked JIRA ticket in the PR body. Please include the URL of the relevant JIRA ticket. If you need to, you may file a ticket on JIRA
Messages
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 2 tests have failed There are 2 tests failing and 675 skipped out of 13035 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.UI.View"after all" hook (5.0.2)20.48
Error: timeout of 10000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)
android.emulator.Titanium.UI.View"after each" hook (5.0.2)10.466
Error: timeout of 10000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)

Generated by 🚫 dangerJS against d5e6d50

@jquick-axway
Copy link
Contributor

jquick-axway commented Feb 18, 2021

We still have a V8 backward compatibility issue in 10.0.0 with previously built modules. We currently fail the invokeCallbackSync and invokeCallbackAsync unit tests in the following PR. It's an issue with module methods. The 1st argument is getting through, but the rest of the arguments are not. This is not a callback issue, but a method argument handling issue.
https://github.com/appcelerator/titanium_mobile/pull/12311/files#diff-353594100a924d948f537a44e9b97d19934cf70e5c2435303732abff7d2df6d7R222-R248

I'm seeing the following get logged when a method with multiple arguments gets called.

[WARN]  TypeConverter: jsValueToJavaObject returning null.

Also, 10.0.0 built modules will have their min API Level set to 21 (matching Titanium 10.0.0's min API Level), which is a problem if you want to use that module in 9.x.x built apps. It will cause an app build failure. This has nothing to do with our V8 change, but we should probably work-around it anyways. Simple solution is to hard-code the module's "build.gradle" minSdkVersion to 19.

@jquick-axway
Copy link
Contributor

I've isolated the issue. As of V8 version 8.8, Google has made a breaking-change where method arguments are now stored in reverse order.

If you look in the "v8.h" file, look for the Local<Value> FunctionCallbackInfo<T>::operator[](int i) function. Note that this is an "inlined" function.

In version 8.8, it looks like this...

template<typename T>
Local<Value> FunctionCallbackInfo<T>::operator[](int i) const {
  // values_ points to the first argument (not the receiver).
  if (i < 0 || length_ <= i) return Local<Value>(*Undefined(GetIsolate()));
  return Local<Value>(reinterpret_cast<Value*>(values_ + i));
}

In version 8.7, it looks like the below. And we're not defining the V8_REVERSE_JSARGS definition so we're ending up in the #else block.

template<typename T>
Local<Value> FunctionCallbackInfo<T>::operator[](int i) const {
  // values_ points to the first argument (not the receiver).
  if (i < 0 || length_ <= i) return Local<Value>(*Undefined(GetIsolate()));
#ifdef V8_REVERSE_JSARGS
  return Local<Value>(reinterpret_cast<Value*>(values_ + i));
#else
  return Local<Value>(reinterpret_cast<Value*>(values_ - i));
#endif
}

Unfortunately, this method call is inlined. This explains why the method's 2nd argument returns the "this" object for older modules running in Titanium 10.0.0. And I would expect 10.0.0 built modules to have the reverse problem when running in a Titanium 9.x.x app.

I can't think of any work-around to maintain backward compatibility other than to revert V8 back to version 8.7.

@garymathews
Copy link
Contributor Author

garymathews commented Feb 19, 2021

I made a patch to disable v8_enable_reverse_jsargs in 8.7, but they removed the parameter altogether in 8.8 😞
tidev/v8_titanium@2971598

I'll try to revert their commit v8/v8@50ddb12

@cb1kenobi
Copy link
Contributor

I literally just read about this the other day: https://v8.dev/blog/v8-release-89. They reverse the arguments to optimize for when the number of arguments mismatches the function signature.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR/FR: Pass

I ran the following tests:

  • Exercised all JS types with 9.2.0 built module from PR test: module backward compatibility #12311 with 10.0.0 built app.
  • Built "ti.imagefactory" module with 10.0.0 and ran its example app.
  • Used 10.0.0 built "ti.imagefactory" module in 10.0.0 built app on x86, x86_64, ARM32, ARM64.
  • Used 10.0.0 built "ti.imagefactory" module in 9.0.0 built app on x86, x86_64, ARM32, ARM64.
  • Built kitchensink-v2 with 10.0.0 and ran on x86_64 and ARM64. (Verified maps worked.)

@sgtcoolguy sgtcoolguy merged commit 738b2c2 into tidev:master Feb 24, 2021
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

5 participants