Merge pull request #1525 from NativeScript/trifonov/error-reporting-f…

…ixes Improve error reporting
NativeScript · Oct 23, 2019 · 5c86811 · 5c86811
2 parents ff679d6 + f8a21f1
commit 5c86811
Show file tree

Hide file tree

Showing 6 changed files with 59 additions and 24 deletions.
diff --git a/.gitignore b/.gitignore
@@ -22,3 +22,4 @@ bin/
 .classpath
 android-runtime.iml
 test-app/build-tools/*.log
+test-app/analytics/build-statistics.json
diff --git a/test-app/app/src/main/assets/app/tests/exceptionHandlingTests.js b/test-app/app/src/main/assets/app/tests/exceptionHandlingTests.js
@@ -302,7 +302,7 @@ describe("Tests exception handling ", function () {
 	        errMsg = e.toString();
 	    }
 	    expect(exceptionCaught).toBe(true);
-	    expect(errMsg).toBe("Error: Unknown error. Cannot get error message.");
+	    expect(errMsg).toBe("Error: com.tns.tests.ExceptionHandlingTest$BadException");
 	});
 
 	it("should successfully catch syntax errors", function () {
@@ -320,18 +320,18 @@ describe("Tests exception handling ", function () {
 	    expect(errMsg).toContain("File: (file:///data/data/com.tns.testapplication/files/app/tests/syntaxErrors.js:3:4)");
 	});
 
-    // run this test only for API level bigger than 25 as we have handling there
-    if(android.os.Build.VERSION.SDK_INT > 25 && android.os.Build.CPU_ABI != "x86" && android.os.Build.CPU_ABI != "x86_64") {
-        it("Should handle SIGABRT and throw a NativeScript exception when incorrectly calling JNI methods", function () {
-            let myClassInstance = new com.tns.tests.MyTestBaseClass3();
-            // public void callMeWithAString(java.lang.String[] stringArr, Runnable arbitraryInterface)
-            try {
-                myClassInstance.callMeWithAString("stringVal", new java.lang.Runnable({ run: () => {} }))
-            } catch (e) {
-                android.util.Log.d("~~~~~", "~~~~~~~~ " + e.toString());
-
-                expect(e.toString()).toContain("SIGABRT");
-            }
-        });
-    }
+   // run this test only for API level bigger than 25 as we have handling there
+   if(android.os.Build.VERSION.SDK_INT > 25 && android.os.Build.CPU_ABI != "x86" && android.os.Build.CPU_ABI != "x86_64") {
+       it("Should handle SIGABRT and throw a NativeScript exception when incorrectly calling JNI methods", function () {
+           let myClassInstance = new com.tns.tests.MyTestBaseClass3();
+           // public void callMeWithAString(java.lang.String[] stringArr, Runnable arbitraryInterface)
+           try {
+               myClassInstance.callMeWithAString("stringVal", new java.lang.Runnable({ run: () => {} }))
+           } catch (e) {
+               android.util.Log.d("~~~~~", "~~~~~~~~ " + e.toString());
+
+               expect(e.toString()).toContain("SIGABRT");
+           }
+       });
+   }
 });
diff --git a/test-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.java b/test-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.java
@@ -31,7 +31,7 @@ public void uncaughtException(Thread thread, Throwable ex) {
                 Runtime runtime = Runtime.getCurrentRuntime();
 
                 if (runtime != null) {
-                    runtime.passUncaughtExceptionToJs(ex, ex.getMessage(), Runtime.getJSStackTrace(ex), stackTraceErrorMessage);
+                    runtime.passUncaughtExceptionToJs(ex, ex.getMessage(), stackTraceErrorMessage, Runtime.getJSStackTrace(ex));
                 }
             } catch (Throwable t) {
                 if (Util.isDebuggableApp(context)) {

diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp
@@ -140,6 +140,9 @@ void NativeScriptException::Init() {
 
     NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID = env.GetStaticMethodID(NATIVESCRIPTEXCEPTION_CLASS, "getStackTraceAsString", "(Ljava/lang/Throwable;)Ljava/lang/String;");
     assert(NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID != nullptr);
+
+    NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID = env.GetStaticMethodID(NATIVESCRIPTEXCEPTION_CLASS, "getMessage", "(Ljava/lang/Throwable;)Ljava/lang/String;");
+    assert(NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID != nullptr);
 }
 
 // ON V8 UNCAUGHT EXCEPTION
@@ -204,7 +207,8 @@ Local<Value> NativeScriptException::WrapJavaToJsException() {
 
 Local<Value> NativeScriptException::GetJavaExceptionFromEnv(const JniLocalRef& exc, JEnv& env) {
     auto errMsg = GetExceptionMessage(env, exc);
-    DEBUG_WRITE("Error during java interop errorMessage %s", errMsg.c_str());
+    auto stackTrace = GetExceptionStackTrace(env, exc);
+    DEBUG_WRITE("Error during java interop errorMessage: %s\n stackTrace:\n %s", errMsg.c_str(), stackTrace.c_str());
 
     auto isolate = Isolate::GetCurrent();
     auto objectManager = Runtime::GetObjectManager(isolate);
@@ -223,6 +227,10 @@ Local<Value> NativeScriptException::GetJavaExceptionFromEnv(const JniLocalRef& e
     auto context = isolate->GetCurrentContext();
     errObj->Set(context, V8StringConstants::GetNativeException(isolate), nativeExceptionObject);
 
+    string jsStackTraceMessage = GetErrorStackTrace(Exception::GetStackTrace(errObj));
+    errObj->Set(context, V8StringConstants::GetStack(isolate), ArgConverter::ConvertToV8String(isolate, jsStackTraceMessage));
+    errObj->Set(context, V8StringConstants::GetStackTrace(isolate), ArgConverter::ConvertToV8String(isolate, jsStackTraceMessage + stackTrace));
+
     return errObj;
 }
 
@@ -362,15 +370,17 @@ string NativeScriptException::GetErrorStackTrace(const Local<StackTrace>& stackT
         auto lineNumber = frame->GetLineNumber();
         auto column = frame->GetColumn();
 
-        ss << "\t" << (i > 0 ? "at " : "") << funcName.c_str() << "(" << srcName.c_str() << ":" << lineNumber << ":" << column << ")" << endl;
+        auto startString = i == 0 ? "" : "\t";
+
+        ss << startString << (i > 0 ? "at " : "") << funcName.c_str() << "(" << srcName.c_str() << ":" << lineNumber << ":" << column << ")" << endl;
     }
 
     return ss.str();
 }
 
 string NativeScriptException::GetExceptionMessage(JEnv& env, jthrowable exception) {
     string errMsg;
-    JniLocalRef msg(env.CallStaticObjectMethod(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID, exception));
+    JniLocalRef msg(env.CallStaticObjectMethod(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID, exception));
 
     const char* msgStr = env.GetStringUTFChars(msg, nullptr);
 
@@ -381,9 +391,23 @@ string NativeScriptException::GetExceptionMessage(JEnv& env, jthrowable exceptio
     return errMsg;
 }
 
+string NativeScriptException::GetExceptionStackTrace(JEnv& env, jthrowable exception) {
+    string errStackTrace;
+    JniLocalRef msg(env.CallStaticObjectMethod(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID, exception));
+
+    const char* msgStr = env.GetStringUTFChars(msg, nullptr);
+
+    errStackTrace.append(msgStr);
+
+    env.ReleaseStringUTFChars(msg, msgStr);
+
+    return errStackTrace;
+}
+
 jclass NativeScriptException::RUNTIME_CLASS = nullptr;
 jclass NativeScriptException::THROWABLE_CLASS = nullptr;
 jclass NativeScriptException::NATIVESCRIPTEXCEPTION_CLASS = nullptr;
 jmethodID NativeScriptException::NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID = nullptr;
 jmethodID NativeScriptException::NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID = nullptr;
+jmethodID NativeScriptException::NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID = nullptr;
 jmethodID NativeScriptException::NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID = nullptr;
diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.h b/test-app/runtime/src/main/cpp/NativeScriptException.h
@@ -52,10 +52,15 @@ class NativeScriptException {
         JniLocalRef TryGetJavaThrowableObject(JEnv& env, const v8::Local<v8::Object>& jsObj);
 
         /*
-         * Gets java exception stack message from jthrowable
+         * Gets java exception message from jthrowable
          */
         std::string GetExceptionMessage(JEnv& env, jthrowable exception);
 
+        /*
+         * Gets java exception stack trace from jthrowable
+         */
+        std::string GetExceptionStackTrace(JEnv& env, jthrowable exception);
+
         /*
          * Gets the member m_javaException, wraps it and creates a javascript error object from it
          */
@@ -92,6 +97,7 @@ class NativeScriptException {
         static jclass NATIVESCRIPTEXCEPTION_CLASS;
         static jmethodID NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID;
         static jmethodID NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID;
+        static jmethodID NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID;
         static jmethodID NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID;
 
         static void PrintErrorMessage(const std::string& errorMessage);

diff --git a/test-app/runtime/src/main/java/com/tns/NativeScriptException.java b/test-app/runtime/src/main/java/com/tns/NativeScriptException.java
@@ -39,12 +39,11 @@ public String getIncomingStackTrace() {
 
     @RuntimeCallable
     public static String getStackTraceAsString(Throwable ex) {
-        String errMessage;
+        String errMessage = "";
         try {
-            errMessage = ex.toString();
             for (StackTraceElement frame: ex.getStackTrace()) {
-                errMessage += "\n    ";
-                errMessage += frame;
+                errMessage += "\t";
+                errMessage += frame + "\n";
             }
 
             Throwable cause = ex.getCause();
@@ -57,4 +56,9 @@ public static String getStackTraceAsString(Throwable ex) {
         }
         return errMessage;
     }
+
+    @RuntimeCallable
+    public static String getMessage(Throwable ex) {
+        return ex.toString();
+    }
 }