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-25546] Simple Code Crash on Android after installing Titanium SDK 7.0.0 Beta #9632

Merged
merged 6 commits into from Nov 28, 2017

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Nov 28, 2017

JIRA: https://jira.appcelerator.org/browse/TIMOB-25546

Description:
Prevent Android from crashing when Ti.API.log() is called with non-String parameters.

Test case:
app.js

Ti.API.log({key: 'value'});
Ti.API.log('debug', {key: 'value'});

@@ -24,7 +24,7 @@ Utf8Value::Utf8Value(v8::Local<v8::Value> value)
if (value.IsEmpty())
return;

v8::Local<v8::String> string = value.As<String>();
v8::Local<v8::String> string = value->ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

This variant of ToString is deprecated (and will be removed in V8 6.4), and so is the variant node uses right now that takes the isolate as an argument.

They want you to use the ToString(Local<Context>): https://v8docs.nodesource.com/node-8.0/dc/d0a/classv8_1_1_value.html#a1afa7fc2578fde13194e602513180a65

Unfortunately that isn't easy because we're not passing in a context or the isolate right now.

I believe the easiest way is to actually totally remove our titanium::Utf8Value and titanium::TwoByteValue usage and replace them with v8::String::Utf8Value and v8::String::Value usage (which do exactly what we're attempting to do here, but are now official APIs).

should(Ti.API.warn('warn')).be.undefined;
});

it('Ti.API.log with one non-String parameter', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

just create this file as tests/Resources/ti.api.addontest.js with only the new/added tests inside the describe block. It's easier to do that to add new tests and handle merging them back to the suite instead of overriding the existing tests (which really should be done if you're editing existing test cases).

@sgtcoolguy sgtcoolguy self-assigned this Nov 28, 2017
@sgtcoolguy
Copy link
Contributor

@ypbnv I'll try and make this change, since it's low-level V8 stuff.

@build
Copy link
Contributor

build commented Nov 28, 2017

Messages
📖

👍 Hey!, You deleted more code than you added. That's awesome!

📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

…e modules already rebuilt against 7.0. Add note that it's deprecated and to remove it in 8.0. Update native module template to no longer refer to that type.
@lokeshchdhry
Copy link
Contributor

FR Passed.

Android does not crash when Ti.API.log() is called with non-String parameters.

Studio Ver: 5.0.0.201711280737
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.11-2
Appc CLI: 7.0.0-master.49
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.9
Node Ver: 8.9.1
Java Ver: 1.8.0_101
Devices: ⇨ samsung SM-G955U1 --- Android 7.0

@sgtcoolguy sgtcoolguy merged commit 8e3cc69 into tidev:master Nov 28, 2017
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

4 participants