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.

Sign up for GitHub

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

Jump to bottom

[TIMOB-25963] Android: Improve KrollRuntime error output #10003

Merged
merged 14 commits into from May 1, 2018
Merged

[TIMOB-25963] Android: Improve KrollRuntime error output #10003

merged 14 commits into from May 1, 2018

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Apr 13, 2018 •
edited

  • Provide access to current stack trace using KrollRuntime.getInstance().getStackTrace()
  • Update error output to match node.js
  • Update exception dialog to show more details including stack trace
  • Include Javascript stack trace with Java exceptions
TEST CASE #1
var win = Ti.UI.createWindow({ backgroundColor: 'gray' });

win.addEventListener('open', function (e) {
   Ti.API.info(e.test.crash);
});

win.open();
/app.js:12
Ti.API.info(e.test.crash);
                   ^
Uncaught TypeError: Cannot read property 'crash' of undefined
    at (/app.js:12:23)
    at value(ti:/events.js:49:21)
    at value(ti:/events.js:101:19)

TEST CASE #2
var win = Ti.UI.createWindow({ backgroundColor: 'gray' });

win.addEventListener('open', function (e) {
   Ti.Geolocation.accuracy = null;
});

win.open();
/app.js:4
  Ti.Geolocation.accuracy = null;
                          ^
   at (/app.js:4:28)
   at value(ti:/events.js:49:21)
   at value(ti:/events.js:101:19)

Uncaught Unable to convert null
   org.appcelerator.titanium.util.TiConvert.toInt(TiConvert.java:415)
   ti.modules.titanium.geolocation.GeolocationModule.propertyChangedAccuracy(GeolocationModule.java:414)
   ti.modules.titanium.geolocation.GeolocationModule.propertyChanged(GeolocationModule.java:384)
   org.appcelerator.kroll.KrollProxy.firePropertyChanged(KrollProxy.java:969)
   org.appcelerator.kroll.KrollProxy.onPropertyChanged(KrollProxy.java:1058)
   org.appcelerator.kroll.runtime.v8.V8Object.nativeFireEvent(Native Method)
   org.appcelerator.kroll.runtime.v8.V8Object.fireEvent(V8Object.java:63)
   org.appcelerator.kroll.KrollProxy.doFireEvent(KrollProxy.java:962)
   org.appcelerator.kroll.KrollProxy.handleMessage(KrollProxy.java:1186)
   org.appcelerator.titanium.proxy.TiViewProxy.handleMessage(TiViewProxy.java:394)

TIMOB-25963
TIMOB-25965

@build build added the android label Apr 13, 2018
@build build added the android label Apr 13, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
@garymathews garymathews force-pushed the TIMOB-25963 branch 2 times, most recently from 952a91d to 18329ae Compare April 18, 2018 18:18
@build build added the android label Apr 18, 2018
@build build added the android label Apr 18, 2018
sgtcoolguy
sgtcoolguy suggested changes Apr 23, 2018
View reviewed changes
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

The changes look OK to me. But I think you should be able to generate mocha unit tests from your manual examples here.

We can't test any changes to the dialog, but we should be able to test that accessing a undefined property throws an Error and that the stack generally looks like we expect it to.

android/runtime/v8/src/native/JSException.cpp Outdated

env->ReleaseStringUTFChars(javaMessage, messagePtr);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we don't just do this cleanup right away after appending the messagePtr to the stream? (i.e. line 45 or so)

@build build added the android label Apr 26, 2018
sgtcoolguy
sgtcoolguy reviewed Apr 26, 2018
View reviewed changes
android/runtime/v8/src/native/V8Util.cpp Outdated

V8Runtime::exceptionStackTrace.Reset(isolate, message->GetStackTrace());
// obtain javascript stack trace
if (jsStack->IsNullOrUndefined() || jsStack.IsEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

reverse the order here? I was worried if trying to call IsNullOrUndefined() on an empty handle would crash.

sgtcoolguy
sgtcoolguy reviewed Apr 26, 2018
View reviewed changes
android/runtime/v8/src/native/V8Util.cpp Outdated

if (exception->IsObject()) {
Local<Object> error = exception.As<Object>();
jsStack = exception.As<Object>()->Get(STRING_NEW(isolate, "stack"));
Copy link
Contributor

Choose a reason for hiding this comment

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

use the error objet reference you assign just above here rather than call .As again

sgtcoolguy
sgtcoolguy reviewed Apr 26, 2018
View reviewed changes
android/runtime/v8/src/native/V8Util.cpp Outdated

jstring title = env->NewStringUTF("Runtime Error");
jstring errorMessage = TypeConverter::jsValueToJavaString(isolate, env, message->Get());
jstring resourceName = TypeConverter::jsValueToJavaString(isolate, env, message->GetScriptResourceName());
jstring sourceLine = TypeConverter::jsValueToJavaString(isolate, env, message->GetSourceLine());
jstring javaStack = TypeConverter::jsValueToJavaString(isolate, env, stack);
jstring jsStackString = TypeConverter::jsValueToJavaString(isolate, env, jsStack);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to guard here for empty handle and/or undefined. TypeConverter handles null -> NULL, but I think it converts undefined to 'undefined'. And I think it'll just crash if the handle is empty.

I assigned to a bool up above for when the stack/nativeStack were not null/undefined/empty and if they were, I assigned NULL here and avoided the DeleteLocalRef below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should improve TypeConverter to handle this validation? This allows us to validate the other conversions too errorMessage, resourceName, sourceLine etc..

Updated PR

@build build added the android label Apr 26, 2018
@build build added the android label Apr 26, 2018
@build build added the android label Apr 26, 2018
sgtcoolguy
sgtcoolguy reviewed Apr 30, 2018
View reviewed changes
android/runtime/v8/src/native/TypeConverter.cpp
@@ -114,7 +114,7 @@ jstring TypeConverter::jsValueToJavaString(v8::Isolate* isolate, v8::Local<v8::V

jstring TypeConverter::jsValueToJavaString(v8::Isolate* isolate, JNIEnv *env, v8::Local<v8::Value> jsValue)
{
if (jsValue->IsNull()) {
if (jsValue.IsEmpty() || jsValue->IsNullOrUndefined()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is OK, but beware this is a behavioral change here. Before we'd change an undefined to 'undefined'

sgtcoolguy
sgtcoolguy reviewed Apr 30, 2018
View reviewed changes
android/runtime/v8/src/native/V8Util.cpp Outdated

if (exception->IsObject()) {
Local<Object> error = exception.As<Object>();
jsStack = error->Get(context, STRING_NEW(isolate, "stack")).ToLocalChecked();
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the next line could cause a crash if the handle is empty. You could do something like this to better handle it:

jsStack = error->Get(context, STRING_NEW(isolate, "stack")).FromMaybe(Undefined(isolate).As<Value>());
javaStack = error->Get(context, STRING_NEW(isolate, "nativeStack")).FromMaybe(Undefined(isolate).As<Value>());

In that code if the handle is empty due to some error, we'll default to undefined.

sgtcoolguy
sgtcoolguy reviewed Apr 30, 2018
View reviewed changes
android/runtime/v8/src/native/V8Util.cpp Outdated
if (jsStack->IsNullOrUndefined() || jsStack.IsEmpty()) {
Local<StackTrace> frames = message->GetStackTrace();
if (frames.IsEmpty() || !frames->GetFrameCount()) {
frames = StackTrace::CurrentStackTrace(isolate, 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we extract the 10 constant here like you did in JSException?

@build build added the android label Apr 30, 2018
@build
Copy link
Contributor

build commented Apr 30, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy merged commit 49a0998 into tidev:master May 1, 2018
@sgtcoolguy sgtcoolguy added this to the 7.2.0 milestone May 1, 2018
@hansemannn
Copy link
Collaborator

QE?

@sgtcoolguy sgtcoolguy mentioned this pull request May 3, 2018
Merged
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@m1ga m1ga mentioned this pull request Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgtcoolguy sgtcoolguy sgtcoolguy approved these changes

@jquick-axway jquick-axway Awaiting requested review from jquick-axway

Assignees
No one assigned
Labels
android improvement in-qe-testing 🕵
Projects
None yet
Milestone
7.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@garymathews @sgtcoolguy @build @hansemannn