Merge pull request #1399 from NativeScript/trifonov/exception-reporting

refactoring message and stack trace reporting
NativeScript · Jun 21, 2019 · 8b5ca4a · 8b5ca4a
2 parents b1f87d0 + cc95ba5
commit 8b5ca4a
Show file tree

Hide file tree

Showing 10 changed files with 141 additions and 76 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,15 @@
 - [java.lang.NullPointerException in Metadata generator(#13795)](https://github.com/NativeScript/android-runtime/issues/1379)
 - [Buffer() is deprecated(#1392)](https://github.com/NativeScript/android-runtime/pull/1392)
 - [Warnings when building android(#1396)](https://github.com/NativeScript/android-runtime/issues/1396)
+- [No JS stack on discardedError and unhandledError(#1354)](https://github.com/NativeScript/android-runtime/issues/1354)
+
+## Breaking Changes
+
+- Exception information in onDiscarderError and onUnhandledError is changed so that `message` contains the exception message and `stackTrace` contains only the stackTrace. In the previous implementation `stackTrace` contained some additional details (including the exception message) and the `message` was something like:
+
+    ```
+    The application crashed because of an uncaught exception. You can look at "stackTrace" or "nativeException" for more detailed information about the exception.
+    ```
 
 5.4.0
 ==

diff --git a/test-app/app/src/main/assets/app/tests/discardedExceptionsTest.js b/test-app/app/src/main/assets/app/tests/discardedExceptionsTest.js
@@ -18,7 +18,8 @@ describe("Tests discarded exception ", function () {
         expect(reportedException).not.toBe(null);
         expect(reportedException.nativeException).not.toBe(null);
         expect(reportedException.nativeException.getMessage()).toBe('Exception to suppress');
-        expect(reportedException.stackTrace).toContain('Error on "main" thread for reportSupressedException');
+        expect(reportedException.message).toBe('Exception to suppress');
+        expect(reportedException.stackTrace).toContain('com.tns.tests.DiscardedExceptionTest.reportSupressedException');
     });
 
     afterEach(function() {

diff --git a/test-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.java b/test-app/app/src/main/java/com/tns/NativeScriptUncaughtExceptionHandler.java
@@ -1,7 +1,6 @@
 package com.tns;
 
 import java.lang.Thread.UncaughtExceptionHandler;
-
 import android.content.Context;
 
 public class NativeScriptUncaughtExceptionHandler implements UncaughtExceptionHandler {
@@ -19,18 +18,19 @@ public NativeScriptUncaughtExceptionHandler(Logger logger, Context context) {
 
     @Override
     public void uncaughtException(Thread thread, Throwable ex) {
-        String currentThreadMessage = "An uncaught Exception occurred on \"" + thread.getName() + "\" thread.\n";
-
-        String errorMessage = currentThreadMessage + Runtime.getStackTraceErrorMessage(ex);
+        String currentThreadMessage = String.format("An uncaught Exception occurred on \"%s\" thread.\n%s\n", thread.getName(), ex.getMessage());
+        String stackTraceErrorMessage = Runtime.getStackTraceErrorMessage(ex);
+        String errorMessage = currentThreadMessage + stackTraceErrorMessage;
 
         if (Runtime.isInitialized()) {
             try {
-                ex.printStackTrace();
+                // print this only in debug
+                System.err.println(errorMessage);
 
                 Runtime runtime = Runtime.getCurrentRuntime();
 
                 if (runtime != null) {
-                    runtime.passUncaughtExceptionToJs(ex, errorMessage);
+                    runtime.passUncaughtExceptionToJs(ex, ex.getMessage(), stackTraceErrorMessage);
                 }
             } catch (Throwable t) {
                 t.printStackTrace();

diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp
@@ -23,14 +23,20 @@ NativeScriptException::NativeScriptException(const string& message)
     m_javascriptException(nullptr), m_javaException(JniLocalRef()), m_message(message) {
 }
 
+NativeScriptException::NativeScriptException(const string& message, const string& stackTrace)
+    :
+    m_javascriptException(nullptr), m_javaException(JniLocalRef()), m_message(message), m_stackTrace(stackTrace) {
+}
+
 NativeScriptException::NativeScriptException(TryCatch& tc, const string& message)
     :
     m_javaException(JniLocalRef()) {
     auto isolate = Isolate::GetCurrent();
     m_javascriptException = new Persistent<Value>(isolate, tc.Exception());
-    bool isMessageEmpty = tc.Message().IsEmpty();
-    bool isExceptionEmpty = tc.Exception().IsEmpty();
-    m_message = GetFullMessage(tc, isExceptionEmpty, isMessageEmpty, message);
+    auto ex = tc.Exception();
+    m_message = GetErrorMessage(tc.Message(), ex, message);
+    m_stackTrace = GetErrorStackTrace(tc.Message()->GetStackTrace());
+    m_fullMessage = GetFullMessage(tc, m_message);
     tc.Reset();
 }
 
@@ -40,9 +46,15 @@ void NativeScriptException::ReThrowToV8() {
 
     if (m_javascriptException != nullptr) {
         errObj = Local<Value>::New(isolate, *m_javascriptException);
-        if (errObj->IsObject() && !m_message.empty()) {
-            errObj.As<Object>()->Set(ArgConverter::ConvertToV8String(isolate, "fullMessage"), ArgConverter::ConvertToV8String(isolate, m_message));
+        if (errObj->IsObject()) {
+            if (!m_fullMessage.empty()) {
+                errObj.As<Object>()->Set(ArgConverter::ConvertToV8String(isolate, "fullMessage"), ArgConverter::ConvertToV8String(isolate, m_fullMessage));
+            } else if (!m_message.empty()) {
+                errObj.As<Object>()->Set(ArgConverter::ConvertToV8String(isolate, "fullMessage"), ArgConverter::ConvertToV8String(isolate, m_message));
+            }
         }
+    } else if (!m_fullMessage.empty()) {
+        errObj = Exception::Error(ArgConverter::ConvertToV8String(isolate, m_fullMessage));
     } else if (!m_message.empty()) {
         errObj = Exception::Error(ArgConverter::ConvertToV8String(isolate, m_message));
     } else if (!m_javaException.IsNull()) {
@@ -83,22 +95,24 @@ void NativeScriptException::ReThrowToJava() {
             ex = (jthrowable) exObj.Move();
         }
 
+        JniLocalRef msg(env.NewStringUTF(m_message.c_str()));
+        JniLocalRef stackTrace(env.NewStringUTF(m_stackTrace.c_str()));
+
         if (ex == nullptr) {
-            JniLocalRef msg(env.NewStringUTF(m_message.c_str()));
-            ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, reinterpret_cast<jlong>(m_javascriptException)));
+            ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, (jstring)stackTrace, reinterpret_cast<jlong>(m_javascriptException)));
         } else {
             auto excClassName = objectManager->GetClassName(ex);
             if (excClassName != "com/tns/NativeScriptException") {
-                JniLocalRef msg(env.NewStringUTF(m_message.c_str()));
-                ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID, (jstring) msg, ex));
+                ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID, (jstring) msg, (jstring)stackTrace, ex));
             }
         }
     } else if (!m_message.empty()) {
         JniLocalRef msg(env.NewStringUTF(m_message.c_str()));
-        ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, (jlong) 0));
+        JniLocalRef stackTrace(env.NewStringUTF(m_stackTrace.c_str()));
+        ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, (jstring)stackTrace, (jlong) 0));
     } else {
         JniLocalRef msg(env.NewStringUTF("No java exception or message provided."));
-        ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, (jlong) 0));
+        ex = static_cast<jthrowable>(env.NewObject(NATIVESCRIPTEXCEPTION_CLASS, NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID, (jstring) msg, (jstring) nullptr, (jlong) 0));
     }
     env.Throw(ex);
 }
@@ -115,10 +129,10 @@ void NativeScriptException::Init() {
     NATIVESCRIPTEXCEPTION_CLASS = env.FindClass("com/tns/NativeScriptException");
     assert(NATIVESCRIPTEXCEPTION_CLASS != nullptr);
 
-    NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID = env.GetMethodID(NATIVESCRIPTEXCEPTION_CLASS, "<init>", "(Ljava/lang/String;J)V");
+    NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID = env.GetMethodID(NATIVESCRIPTEXCEPTION_CLASS, "<init>", "(Ljava/lang/String;Ljava/lang/String;J)V");
     assert(NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID != nullptr);
 
-    NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID = env.GetMethodID(NATIVESCRIPTEXCEPTION_CLASS, "<init>", "(Ljava/lang/String;Ljava/lang/Throwable;)V");
+    NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID = env.GetMethodID(NATIVESCRIPTEXCEPTION_CLASS, "<init>", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/Throwable;)V");
     assert(NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID != nullptr);
 
     NATIVESCRIPTEXCEPTION_GET_STACK_TRACE_AS_STRING_METHOD_ID = env.GetStaticMethodID(NATIVESCRIPTEXCEPTION_CLASS, "getStackTraceAsString", "(Ljava/lang/Throwable;)Ljava/lang/String;");
@@ -128,8 +142,9 @@ void NativeScriptException::Init() {
 // ON V8 UNCAUGHT EXCEPTION
 void NativeScriptException::OnUncaughtError(Local<Message> message, Local<Value> error) {
     string errorMessage = GetErrorMessage(message, error);
+    string stackTrace = GetErrorStackTrace(message->GetStackTrace());
 
-    NativeScriptException e(errorMessage);
+    NativeScriptException e(errorMessage, stackTrace);
     e.ReThrowToJava();
 }
 
@@ -203,17 +218,31 @@ Local<Value> NativeScriptException::GetJavaExceptionFromEnv(const JniLocalRef& e
     return errObj;
 }
 
-string NativeScriptException::GetFullMessage(const TryCatch& tc, bool isExceptionEmpty, bool isMessageEmpty, const string& prependMessage) {
+string NativeScriptException::GetFullMessage(const TryCatch& tc, const string& jsExceptionMessage) {
     auto ex = tc.Exception();
 
-    string jsExeptionMessage;
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
+    v8::Local<v8::Context> context = isolate->GetEnteredContext();
 
-    if (!isExceptionEmpty && !isMessageEmpty) {
-        jsExeptionMessage = GetErrorMessage(tc.Message(), ex);
-    }
+    auto message = tc.Message();
 
     stringstream ss;
-    ss << endl << prependMessage << jsExeptionMessage;
+    ss << jsExceptionMessage;
+
+    //get script name
+    auto scriptResName = message->GetScriptResourceName();
+
+    //get stack trace
+    string stackTraceMessage = GetErrorStackTrace(message->GetStackTrace());
+
+    if (!scriptResName.IsEmpty() && scriptResName->IsString()) {
+        ss << endl <<"File: \"" << ArgConverter::ConvertToString(scriptResName.As<String>());
+    } else {
+        ss << endl <<"File: \"<unknown>";
+    }
+    ss << ", line: " << message->GetLineNumber(context).ToChecked() << ", column: " << message->GetStartColumn() << endl << endl;
+    ss << "StackTrace: " << endl << stackTraceMessage << endl;
+
     string loggedMessage = ss.str();
 
     PrintErrorMessage(loggedMessage);
@@ -266,46 +295,38 @@ void NativeScriptException::PrintErrorMessage(const string& errorMessage) {
     }
 }
 
-string NativeScriptException::GetErrorMessage(const Local<Message>& message, Local<Value>& error) {
+string NativeScriptException::GetErrorMessage(const Local<Message>& message, Local<Value>& error, const string& prependMessage) {
 
     Local<String> message_text_string = message->Get();
     auto mes = ArgConverter::ConvertToString(message_text_string);
 
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
     v8::Local<v8::Context> context = isolate->GetEnteredContext();
-    int line_number = message->GetLineNumber(context).FromMaybe(0);
 
     //get whole error message from previous stack
+    stringstream ss;
+
+    if (prependMessage != "") {
+        ss << prependMessage << endl;
+    }
+
     string errMessage;
+    bool hasFullErrorMessage = false;
     auto v8FullMessage = ArgConverter::ConvertToV8String(isolate, "fullMessage");
     if (error->IsObject() && error.As<Object>()->Has(context, v8FullMessage).ToChecked()) {
+        hasFullErrorMessage = true;
         errMessage = ArgConverter::ConvertToString(error.As<Object>()->Get(v8FullMessage).As<String>());
+        ss << errMessage;
     }
 
-
-    //get current message
     auto str = error->ToDetailString(context);
-    if (str.IsEmpty()) {
-        str = String::NewFromUtf8(isolate, "");
-    }
-    String::Utf8Value utfError(isolate, str.FromMaybe(Local<String>()));
-
-    //get script name
-    auto scriptResName = message->GetScriptResourceName();
-
-    //get stack trace
-    string stackTraceMessage = GetErrorStackTrace(message->GetStackTrace());
-
-    stringstream ss;
-    ss << endl << errMessage;
-    ss << endl << *utfError << endl;
-    if (!scriptResName.IsEmpty() && scriptResName->IsString()) {
-        ss << "File: \"" << ArgConverter::ConvertToString(scriptResName.As<String>());
-    } else {
-        ss << "File: \"<unknown>";
+    if (!str.IsEmpty()) {
+        String::Utf8Value utfError(isolate, str.FromMaybe(Local<String>()));
+        if(hasFullErrorMessage) {
+            ss << endl;
+        }
+        ss << *utfError;
     }
-    ss << ", line: " << message->GetLineNumber(context).ToChecked() << ", column: " << message->GetStartColumn() << endl << endl;
-    ss << "StackTrace: " << endl << stackTraceMessage << endl;
 
     return ss.str();
 }

diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.h b/test-app/runtime/src/main/cpp/NativeScriptException.h
@@ -20,6 +20,11 @@ class NativeScriptException {
          */
         NativeScriptException(const std::string& message);
 
+        /*
+         * Generates a NativeScriptException with given message and stackTrace
+         */
+        NativeScriptException(const std::string& message, const std::string& stackTrace);
+
         /*
          * Generates a NativeScriptException with javascript error from TryCatch and a prepend message if any
          */
@@ -64,7 +69,7 @@ class NativeScriptException {
         /*
          * Gets all the information from a js message and an js error object and puts it in a string
          */
-        static std::string GetErrorMessage(const v8::Local<v8::Message>& message, v8::Local<v8::Value>& error);
+        static std::string GetErrorMessage(const v8::Local<v8::Message>& message, v8::Local<v8::Value>& error, const std::string& prependMessage = "");
 
         /*
          * Generates string stack trace from js StackTrace
@@ -74,11 +79,13 @@ class NativeScriptException {
         /*
          *	Adds a prepend message to the normal message process
          */
-        std::string GetFullMessage(const v8::TryCatch& tc, bool isExceptionEmpty, bool isMessageEmpty, const std::string& prependMessage = "");
+        std::string GetFullMessage(const v8::TryCatch& tc, const std::string& jsExceptionMessage);
 
         v8::Persistent<v8::Value>* m_javascriptException;
         JniLocalRef m_javaException;
         std::string m_message;
+        std::string m_stackTrace;
+        std::string m_fullMessage;
 
         static jclass RUNTIME_CLASS;
         static jclass THROWABLE_CLASS;

diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp
@@ -370,12 +370,10 @@ bool Runtime::TryCallGC() {
     return success;
 }
 
-void Runtime::PassExceptionToJsNative(JNIEnv* env, jobject obj, jthrowable exception, jstring stackTrace, jboolean isDiscarded) {
+void Runtime::PassExceptionToJsNative(JNIEnv* env, jobject obj, jthrowable exception, jstring message, jstring stackTrace, jboolean isDiscarded) {
     auto isolate = m_isolate;
 
-    //create error message
-    string errMsg = isDiscarded ? "An exception was caught and discarded. You can look at \"stackTrace\" or \"nativeException\" for more detailed information about the exception.":
-                    "The application crashed because of an uncaught exception. You can look at \"stackTrace\" or \"nativeException\" for more detailed information about the exception.";
+    string errMsg = ArgConverter::jstringToString(message);
 
     auto errObj = Exception::Error(ArgConverter::ConvertToV8String(isolate, errMsg)).As<Object>();
 
@@ -392,9 +390,6 @@ void Runtime::PassExceptionToJsNative(JNIEnv* env, jobject obj, jthrowable excep
         }
     }
 
-    string stackTraceText = ArgConverter::jstringToString(stackTrace);
-    errMsg += "\n" + stackTraceText;
-
     //create a JS error object
     errObj->Set(V8StringConstants::GetNativeException(isolate), nativeExceptionObject);
     errObj->Set(V8StringConstants::GetStackTrace(isolate), ArgConverter::jstringToV8String(isolate, stackTrace));

diff --git a/test-app/runtime/src/main/cpp/Runtime.h b/test-app/runtime/src/main/cpp/Runtime.h
@@ -52,7 +52,7 @@ class Runtime {
         void AdjustAmountOfExternalAllocatedMemory();
         bool NotifyGC(JNIEnv* env, jobject obj);
         bool TryCallGC();
-        void PassExceptionToJsNative(JNIEnv* env, jobject obj, jthrowable exception, jstring stackTrace, jboolean isDiscarded);
+        void PassExceptionToJsNative(JNIEnv* env, jobject obj, jthrowable exception, jstring message, jstring stackTrace, jboolean isDiscarded);
         void PassUncaughtExceptionFromWorkerToMainHandler(v8::Local<v8::String> message, v8::Local<v8::String> stackTrace, v8::Local<v8::String> filename, int lineno);
         void ClearStartupData(JNIEnv* env, jobject obj);
         void DestroyRuntime();

diff --git a/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp b/test-app/runtime/src/main/cpp/com_tns_Runtime.cpp
@@ -245,7 +245,7 @@ extern "C" JNIEXPORT void Java_com_tns_Runtime_unlock(JNIEnv* env, jobject obj,
     }
 }
 
-extern "C" JNIEXPORT void Java_com_tns_Runtime_passExceptionToJsNative(JNIEnv* env, jobject obj, jint runtimeId, jthrowable exception, jstring stackTrace, jboolean isDiscarded) {
+extern "C" JNIEXPORT void Java_com_tns_Runtime_passExceptionToJsNative(JNIEnv* env, jobject obj, jint runtimeId, jthrowable exception, jstring message, jstring stackTrace, jboolean isDiscarded) {
     auto runtime = TryGetRuntime(runtimeId);
     if (runtime == nullptr) {
         return;
@@ -256,7 +256,7 @@ extern "C" JNIEXPORT void Java_com_tns_Runtime_passExceptionToJsNative(JNIEnv* e
     v8::HandleScope handleScope(isolate);
 
     try {
-        runtime->PassExceptionToJsNative(env, obj, exception, stackTrace, isDiscarded);
+        runtime->PassExceptionToJsNative(env, obj, exception, message, stackTrace, isDiscarded);
     } catch (NativeScriptException& e) {
         e.ReThrowToJava();
     } catch (std::exception e) {

diff --git a/test-app/runtime/src/main/java/com/tns/NativeScriptException.java b/test-app/runtime/src/main/java/com/tns/NativeScriptException.java
@@ -4,28 +4,39 @@
 public class NativeScriptException extends RuntimeException {
     @SuppressWarnings("unused")
     private long jsValueAddress = 0;
+    private String incomingStackTrace;
 
     public NativeScriptException() {
         super();
     }
 
-    public NativeScriptException(String detailMessage) {
-        super(detailMessage);
+    public NativeScriptException(String message) {
+        super(message);
     }
 
     public NativeScriptException(Throwable throwable) {
         super(throwable);
     }
 
-    public NativeScriptException(String detailMessage, Throwable throwable) {
-        super(detailMessage, throwable);
+    public NativeScriptException(String message, Throwable throwable) {
+        super(message, throwable);
     }
 
-    public NativeScriptException(String detailMessage, long jsValueAddress) {
-        super(detailMessage);
+    public NativeScriptException(String message, String stackTrace, Throwable throwable) {
+        super(message, throwable);
+        this.incomingStackTrace = stackTrace;
+    }
+
+    public NativeScriptException(String message, String stackTrace, long jsValueAddress) {
+        super(message);
+        this.incomingStackTrace = stackTrace;
         this.jsValueAddress = jsValueAddress;
     }
 
+    public String getIncomingStackTrace() {
+        return incomingStackTrace;
+    }
+
     @RuntimeCallable
     public static String getStackTraceAsString(Throwable ex) {
         String errMessage;