refactor: replace assert() with NS_CHECK/NS_DCHECK

assert() never runs in a build we ship. assembleRelease maps to the
RelWithDebInfo CMake config, whose stock CMAKE_CXX_FLAGS_RELWITHDEBINFO
carries -DNDEBUG, and CMakeLists appends -O3 to that variable rather than
replacing it, so nothing removes the define. All 123 first-party asserts were
therefore diagnostics that existed only in debug and test runs -- the two
places the invariants were least likely to be violated.

Two macros replace them, both in NativeScriptAssert.h:

  NS_CHECK  evaluates and aborts in every configuration.
  NS_DCHECK evaluates and aborts in debug builds only.

61 sites become NS_CHECK: the JNIEnv/JavaVM handles and the jclass, jmethodID
and jfieldID lookups resolved once during runtime initialisation from fixed
class names, plus the per-isolate V8StringConstants block. Every one of them
is used unconditionally a statement or two later, so a null there is undefined
behaviour today and surfaces as a tombstone pointing at whatever ran next.
JEnv::GetMethodID and friends already call CheckForJavaException, so these
fire only when a lookup returns null with no pending Java exception; they are
backstops, not the primary error path.

The remaining 62 sites keep debug-only semantics as NS_DCHECK. Notably
MethodCache and FieldAccessor check the result of JEnv::FindClass, which
deliberately returns nullptr with a pending Java exception for a class that is
genuinely missing and lets the caller raise a NativeScriptException. Aborting
there would turn a handled, recoverable path into a crash.

A failed NS_CHECK records the expression and source location through
CrashBreadcrumbs::RecordFatal and logs it at ANDROID_LOG_FATAL, which claims
the bionic abort message slot, so the check names itself in the tombstone and
in the breadcrumb file the next launch reports. RecordFatal takes no lock and
writes a buffer the signal handler already knows how to emit, so it is safe on
a thread that is aborting from under one of the runtime's own locks.

NS_DCHECK still compiles its expression when NDEBUG is defined, in a branch
that is never taken, so an expression that stops making sense is a build
failure instead of something only a debug build notices. It follows that the
expression must stay free of side effects, exactly as with assert().

Author: edusperoni

test-app/runtime/CMakeLists.txt

@@ -195,6 +195,7 @@ add_library(
         src/main/cpp/ModuleBinding.cpp
         src/main/cpp/ModuleInternal.cpp
         src/main/cpp/ModuleInternalCallbacks.cpp
+        src/main/cpp/NativeScriptAssert.cpp
         src/main/cpp/NativeScriptException.cpp
         src/main/cpp/NativeScriptPlatform.cpp
         src/main/cpp/NsBuiltinModules.cpp

test-app/runtime/src/main/cpp/ArgConverter.cpp

@@ -177,16 +177,16 @@ Local<Value> ArgConverter::ConvertFromJavaLong(Isolate* isolate, jlong value) {
 }
 
 int64_t ArgConverter::ConvertToJavaLong(Isolate* isolate, const Local<Value>& value) {
-    assert(!value.IsEmpty());
+    NS_DCHECK(!value.IsEmpty());
 
     auto obj = Local<Object>::Cast(value);
 
-    assert(!obj.IsEmpty());
+    NS_DCHECK(!obj.IsEmpty());
 
     auto context = isolate->GetCurrentContext();
     Local<Value> temp;
     bool success = obj->Get(context, V8StringConstants::GetValue(isolate)).ToLocal(&temp);
-    assert(success && !temp.IsEmpty());
+    NS_DCHECK(success && !temp.IsEmpty());
     auto valueProp = temp.As<Object>();
 
     string num = ConvertToString(valueProp->ToString(context).ToLocalChecked());

test-app/runtime/src/main/cpp/ArrayBufferHelper.cpp

@@ -1,4 +1,5 @@
 #include "ArrayBufferHelper.h"
+#include "NativeScriptAssert.h"
 #include "ArgConverter.h"
 #include "NativeScriptException.h"
 #include <sstream>
@@ -65,7 +66,7 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
 
     if (m_ByteBufferClass == nullptr) {
         m_ByteBufferClass = env.FindClass("java/nio/ByteBuffer");
-        assert(m_ByteBufferClass != nullptr);
+        NS_CHECK(m_ByteBufferClass != nullptr);
     }
 
     auto isByteBuffer = env.IsInstanceOf(obj, m_ByteBufferClass);
@@ -76,7 +77,7 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
 
     if (m_isDirectMethodID == nullptr) {
         m_isDirectMethodID = env.GetMethodID(m_ByteBufferClass, "isDirect", "()Z");
-        assert(m_isDirectMethodID != nullptr);
+        NS_CHECK(m_isDirectMethodID != nullptr);
     }
 
     auto ret = env.CallBooleanMethod(obj, m_isDirectMethodID);
@@ -99,14 +100,14 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
     } else {
         if (m_remainingMethodID == nullptr) {
             m_remainingMethodID = env.GetMethodID(m_ByteBufferClass, "remaining", "()I");
-            assert(m_remainingMethodID != nullptr);
+            NS_CHECK(m_remainingMethodID != nullptr);
         }
 
         int bufferRemainingSize = env.CallIntMethod(obj, m_remainingMethodID);
 
         if (m_getMethodID == nullptr) {
             m_getMethodID = env.GetMethodID(m_ByteBufferClass, "get", "([BII)Ljava/nio/ByteBuffer;");
-            assert(m_getMethodID != nullptr);
+            NS_CHECK(m_getMethodID != nullptr);
         }
 
         jbyteArray byteArray = env.NewByteArray(bufferRemainingSize);

test-app/runtime/src/main/cpp/ArrayHelper.cpp

@@ -1,4 +1,5 @@
 #include "ArrayHelper.h"
+#include "NativeScriptAssert.h"
 #include "ArgConverter.h"
 #include "NativeScriptException.h"
 #include "Runtime.h"
@@ -15,10 +16,10 @@ void ArrayHelper::Init(const Local<Context>& context) {
     JEnv env;
 
     RUNTIME_CLASS = env.FindClass("com/tns/Runtime");
-    assert(RUNTIME_CLASS != nullptr);
+    NS_CHECK(RUNTIME_CLASS != nullptr);
 
     CREATE_ARRAY_HELPER = env.GetStaticMethodID(RUNTIME_CLASS, "createArrayHelper", "(Ljava/lang/String;I)Ljava/lang/Object;");
-    assert(CREATE_ARRAY_HELPER != nullptr);
+    NS_CHECK(CREATE_ARRAY_HELPER != nullptr);
 
     auto isolate = v8::Isolate::GetCurrent();
     auto global = context->Global();

test-app/runtime/src/main/cpp/AssetExtractor.cpp

@@ -1,6 +1,6 @@
 #include "jni.h"
 #include "zip.h"
-#include <assert.h>
+#include "NativeScriptAssert.h"
 #include <libgen.h>
 #include <utime.h>
 #include <sys/stat.h>
@@ -22,7 +22,7 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin
     int err = 0;
     auto z = zip_open(strApk.c_str(), 0, &err);
 
-    assert(z != nullptr);
+    NS_DCHECK(z != nullptr);
     zip_int64_t num = zip_get_num_entries(z, 0);
     struct zip_stat sb;
     struct zip_file* zf;
@@ -53,15 +53,15 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin
                 mkdir_rec(dirFullname.c_str());
 
                 zf = zip_fopen_index(z, i, 0);
-                assert(zf != nullptr);
+                NS_DCHECK(zf != nullptr);
 
                 auto fd = fopen(assetFullname.c_str(), "w");
 
                 if (fd != nullptr) {
                     zip_int64_t sum = 0;
                     while (sum != sb.size) {
                         zip_int64_t len = zip_fread(zf, buf, sizeof(buf));
-                        assert(len > 0);
+                        NS_DCHECK(len > 0);
 
                         fwrite(buf, 1, len, fd);
                         sum += len;

test-app/runtime/src/main/cpp/CallbackHandlers.cpp

@@ -1,4 +1,5 @@
 #include "CallbackHandlers.h"
+#include "NativeScriptAssert.h"
 #include "MetadataNode.h"
 #include "Util.h"
 #include "V8GlobalHelpers.h"
@@ -30,33 +31,33 @@ void CallbackHandlers::Init(Isolate *isolate) {
     JEnv env;
 
     JAVA_LANG_STRING = env.FindClass("java/lang/String");
-    assert(JAVA_LANG_STRING != nullptr);
+    NS_CHECK(JAVA_LANG_STRING != nullptr);
 
     RUNTIME_CLASS = env.FindClass("com/tns/Runtime");
-    assert(RUNTIME_CLASS != nullptr);
+    NS_CHECK(RUNTIME_CLASS != nullptr);
 
     RESOLVE_CLASS_METHOD_ID = env.GetMethodID(RUNTIME_CLASS, "resolveClass",
                                               "(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;[Ljava/lang/String;Z)Ljava/lang/Class;");
-    assert(RESOLVE_CLASS_METHOD_ID != nullptr);
+    NS_CHECK(RESOLVE_CLASS_METHOD_ID != nullptr);
 
     CURRENT_OBJECTID_FIELD_ID = env.GetFieldID(RUNTIME_CLASS, "currentObjectId", "I");
-    assert(CURRENT_OBJECTID_FIELD_ID != nullptr);
+    NS_CHECK(CURRENT_OBJECTID_FIELD_ID != nullptr);
 
     MAKE_INSTANCE_STRONG_ID = env.GetMethodID(RUNTIME_CLASS, "makeInstanceStrong",
                                               "(Ljava/lang/Object;I)V");
-    assert(MAKE_INSTANCE_STRONG_ID != nullptr);
+    NS_CHECK(MAKE_INSTANCE_STRONG_ID != nullptr);
 
     GET_TYPE_METADATA = env.GetStaticMethodID(RUNTIME_CLASS, "getTypeMetadata",
                                               "(Ljava/lang/String;I)[Ljava/lang/String;");
-    assert(GET_TYPE_METADATA != nullptr);
+    NS_CHECK(GET_TYPE_METADATA != nullptr);
 
     ENABLE_VERBOSE_LOGGING_METHOD_ID = env.GetMethodID(RUNTIME_CLASS, "enableVerboseLogging",
                                                        "()V");
-    assert(ENABLE_VERBOSE_LOGGING_METHOD_ID != nullptr);
+    NS_CHECK(ENABLE_VERBOSE_LOGGING_METHOD_ID != nullptr);
 
     DISABLE_VERBOSE_LOGGING_METHOD_ID = env.GetMethodID(RUNTIME_CLASS, "disableVerboseLogging",
                                                         "()V");
-    assert(ENABLE_VERBOSE_LOGGING_METHOD_ID != nullptr);
+    NS_CHECK(ENABLE_VERBOSE_LOGGING_METHOD_ID != nullptr);
 
     MetadataNode::Init(isolate);
 
@@ -559,7 +560,7 @@ void CallbackHandlers::CallJavaMethod(const Local<Object> &caller, const string
             break;
         }
         default: {
-            assert(false);
+            NS_DCHECK(false);
             break;
         }
     }
@@ -674,7 +675,7 @@ CallbackHandlers::GetMethodOverrides(JEnv &env, const Local<Object> &implementat
 }
 
 void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo<v8::Value> &args) {
-    assert(args[0]->IsFunction());
+    NS_DCHECK(args[0]->IsFunction());
     Isolate *isolate = args.GetIsolate();
 
     v8::Locker locker(isolate);
@@ -695,7 +696,7 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo<v8::Va
         std::lock_guard<std::mutex> lock(cacheMutex_);
         bool inserted;
         std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback);
-        assert(inserted && "Main thread callback ID should not be duplicated");
+        NS_DCHECK(inserted && "Main thread callback ID should not be duplicated");
     }
 
     // bare entry: the closure locks the CALLER's isolate (possibly a
@@ -957,7 +958,7 @@ vector<string> CallbackHandlers::GetTypeMetadata(const string &name, int index)
 
     jsize length = env.GetArrayLength(pubApi);
 
-    assert(length > 0);
+    NS_DCHECK(length > 0);
 
     vector<string> result;
 

test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp

@@ -18,6 +18,7 @@ constexpr size_t kMaxRuntimes = 16;
 constexpr size_t kFieldMax = 160;
 constexpr size_t kBufferMax = 8192;
 constexpr size_t kHeaderMax = 128;
+constexpr size_t kFatalMax = 256;
 
 struct Slot {
   bool used;
@@ -43,6 +44,14 @@ std::atomic<int> g_storeFd{-1};
 std::atomic_flag g_recorded = ATOMIC_FLAG_INIT;
 struct sigaction g_previous[NSIG];
 
+/*
+ * Written by whichever thread is on its way to abort(), read by the signal
+ * handler. Kept out of the rendered buffers so that recording it needs no
+ * lock -- the thread may be aborting from under one.
+ */
+char g_fatalMessage[kFatalMax];
+std::atomic<size_t> g_fatalLength{0};
+
 thread_local Slot* t_slot = nullptr;
 
 int CurrentTid() { return static_cast<int>(syscall(__NR_gettid)); }
@@ -154,9 +163,22 @@ void Handler(int signalNumber, siginfo_t* info, void* context) {
       AppendRaw(header, sizeof(header), length, "\n");
 
       ssize_t written = pwrite(fd, header, length, 0);
-      int active = g_active.load(std::memory_order_acquire);
-      if (written > 0 && active >= 0) {
-        pwrite(fd, g_rendered[active], g_renderedLength[active], written);
+      if (written > 0) {
+        off_t offset = written;
+
+        size_t fatalLength = g_fatalLength.load(std::memory_order_acquire);
+        if (fatalLength > 0) {
+          ssize_t fatalWritten =
+              pwrite(fd, g_fatalMessage, fatalLength, offset);
+          if (fatalWritten > 0) {
+            offset += fatalWritten;
+          }
+        }
+
+        int active = g_active.load(std::memory_order_acquire);
+        if (active >= 0) {
+          pwrite(fd, g_rendered[active], g_renderedLength[active], offset);
+        }
       }
     }
   }
@@ -278,6 +300,18 @@ void CrashBreadcrumbs::SetWorkerScript(int runtimeId, const char* script) {
   RenderLocked();
 }
 
+void CrashBreadcrumbs::RecordFatal(const char* message) {
+  if (message == nullptr) {
+    return;
+  }
+  // Room is reserved for the newline and the terminator.
+  size_t length = strnlen(message, kFatalMax - 2);
+  memcpy(g_fatalMessage, message, length);
+  g_fatalMessage[length] = '\n';
+  g_fatalMessage[length + 1] = '\0';
+  g_fatalLength.store(length + 1, std::memory_order_release);
+}
+
 CrashBreadcrumbs::ModuleScope::ModuleScope(const char* modulePath) {
   Slot* slot = t_slot;
   if (slot == nullptr) {

test-app/runtime/src/main/cpp/CrashBreadcrumbs.h

@@ -36,6 +36,13 @@ class CrashBreadcrumbs {
   /* Marks a registered runtime as a worker started from `script`. */
   static void SetWorkerScript(int runtimeId, const char* script);
 
+  /*
+   * Records a line to be written ahead of the runtime state should the process
+   * die. Takes no lock, so it stays usable from a thread that is about to
+   * abort and may already hold any of the runtime's own locks.
+   */
+  static void RecordFatal(const char* message);
+
   /*
    * Records the module the calling runtime is executing for the lifetime of
    * the scope. Module loads nest (`require` inside a module body), so the

test-app/runtime/src/main/cpp/EventLoop.cpp

@@ -6,7 +6,6 @@
 #include <unistd.h>
 
 #include <algorithm>
-#include <cassert>
 #include <cerrno>
 #include <cmath>
 #include <cstring>
@@ -98,7 +97,7 @@ void EventLoop::BindToCurrentThread() {
     if (EVENT_LOOP_HANDLER_CLASS == nullptr) {
         // JEnv::FindClass caches a global ref to the class
         EVENT_LOOP_HANDLER_CLASS = env.FindClass("com/tns/EventLoopHandler");
-        assert(EVENT_LOOP_HANDLER_CLASS != nullptr);
+        NS_CHECK(EVENT_LOOP_HANDLER_CLASS != nullptr);
         EVENT_LOOP_HANDLER_CTOR = env.GetMethodID(EVENT_LOOP_HANDLER_CLASS, "<init>", "(J)V");
         EVENT_LOOP_HANDLER_POST = env.GetMethodID(EVENT_LOOP_HANDLER_CLASS, "post", "(J)V");
         EVENT_LOOP_HANDLER_POST_TOKEN =
@@ -128,7 +127,7 @@ void EventLoop::BindToCurrentThread() {
     }
     JniLocalRef handler(env.NewObject(EVENT_LOOP_HANDLER_CLASS, EVENT_LOOP_HANDLER_CTOR,
                                       reinterpret_cast<jlong>(this)));
-    assert(!handler.IsNull());
+    NS_DCHECK(!handler.IsNull());
     handler_ = env.NewGlobalRef(handler);
 
     // flush work buffered before the home thread was known

test-app/runtime/src/main/cpp/FieldAccessor.cpp

@@ -1,4 +1,5 @@
 #include "FieldAccessor.h"
+#include "NativeScriptAssert.h"
 #include "ArgConverter.h"
 #include "NativeScriptException.h"
 #include "Runtime.h"
@@ -214,14 +215,14 @@ void FieldAccessor::SetJavaField(Isolate* isolate, const Local<Object>& target,
 
         if (isStatic) {
             fieldData->clazz = env.FindClass(fieldMetadata.getDeclaringType());
-            assert(fieldData->clazz != nullptr);
+            NS_DCHECK(fieldData->clazz != nullptr);
             fieldData->fid = env.GetStaticFieldID(fieldData->clazz, fieldMetadata.name, fieldJniSig);
-            assert(fieldData->fid != nullptr);
+            NS_DCHECK(fieldData->fid != nullptr);
         } else {
             fieldData->clazz = env.FindClass(fieldMetadata.getDeclaringType());
-            assert(fieldData->clazz != nullptr);
+            NS_DCHECK(fieldData->clazz != nullptr);
             fieldData->fid = env.GetFieldID(fieldData->clazz, fieldMetadata.name, fieldJniSig);
-            assert(fieldData->fid != nullptr);
+            NS_DCHECK(fieldData->fid != nullptr);
         }
     }