From 25b49c2994c482e6d460911f978d1c238fbc03d3 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 15 Aug 2026 15:12:28 -0300 Subject: [PATCH 1/5] 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(). --- test-app/runtime/CMakeLists.txt | 1 + .../runtime/src/main/cpp/ArgConverter.cpp | 6 +-- .../src/main/cpp/ArrayBufferHelper.cpp | 9 ++-- test-app/runtime/src/main/cpp/ArrayHelper.cpp | 5 ++- .../runtime/src/main/cpp/AssetExtractor.cpp | 8 ++-- .../runtime/src/main/cpp/CallbackHandlers.cpp | 25 ++++++----- .../runtime/src/main/cpp/CrashBreadcrumbs.cpp | 40 +++++++++++++++-- .../runtime/src/main/cpp/CrashBreadcrumbs.h | 7 +++ test-app/runtime/src/main/cpp/EventLoop.cpp | 5 +-- .../runtime/src/main/cpp/FieldAccessor.cpp | 9 ++-- test-app/runtime/src/main/cpp/File.cpp | 1 - .../runtime/src/main/cpp/FrameCallbacks.cpp | 6 +-- test-app/runtime/src/main/cpp/JEnv.cpp | 16 +++---- .../runtime/src/main/cpp/JSONObjectHelper.cpp | 3 +- .../src/main/cpp/JniSignatureParser.cpp | 12 ++--- .../src/main/cpp/JsV8InspectorClient.cpp | 25 +++++------ test-app/runtime/src/main/cpp/LRUCache.h | 12 ++--- .../runtime/src/main/cpp/MetadataNode.cpp | 8 ++-- .../runtime/src/main/cpp/MetadataReader.cpp | 11 ++--- .../runtime/src/main/cpp/MetadataReader.h | 6 +-- test-app/runtime/src/main/cpp/MethodCache.cpp | 8 ++-- .../runtime/src/main/cpp/ModuleInternal.cpp | 14 +++--- .../src/main/cpp/NativeScriptAssert.cpp | 32 ++++++++++++++ .../runtime/src/main/cpp/NativeScriptAssert.h | 44 +++++++++++++++++++ .../src/main/cpp/NativeScriptException.cpp | 14 +++--- .../runtime/src/main/cpp/ObjectManager.cpp | 30 ++++++------- test-app/runtime/src/main/cpp/Runtime.cpp | 4 +- .../runtime/src/main/cpp/StructuredClone.cpp | 8 ++-- .../src/main/cpp/StructuredSerialization.cpp | 6 +-- .../src/main/cpp/V8StringConstants.cpp | 3 +- test-app/runtime/src/main/cpp/WeakRef.cpp | 3 +- .../runtime/src/main/cpp/WorkerWrapper.cpp | 6 +-- .../runtime/src/main/cpp/console/Console.cpp | 4 +- .../src/main/cpp/utils/PageResources.cpp | 9 ++-- 34 files changed, 261 insertions(+), 139 deletions(-) create mode 100644 test-app/runtime/src/main/cpp/NativeScriptAssert.cpp diff --git a/test-app/runtime/CMakeLists.txt b/test-app/runtime/CMakeLists.txt index b9590d6c4..ef8a0d782 100644 --- a/test-app/runtime/CMakeLists.txt +++ b/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 diff --git a/test-app/runtime/src/main/cpp/ArgConverter.cpp b/test-app/runtime/src/main/cpp/ArgConverter.cpp index 6ebcb4a64..93cdf54d9 100644 --- a/test-app/runtime/src/main/cpp/ArgConverter.cpp +++ b/test-app/runtime/src/main/cpp/ArgConverter.cpp @@ -177,16 +177,16 @@ Local ArgConverter::ConvertFromJavaLong(Isolate* isolate, jlong value) { } int64_t ArgConverter::ConvertToJavaLong(Isolate* isolate, const Local& value) { - assert(!value.IsEmpty()); + NS_DCHECK(!value.IsEmpty()); auto obj = Local::Cast(value); - assert(!obj.IsEmpty()); + NS_DCHECK(!obj.IsEmpty()); auto context = isolate->GetCurrentContext(); Local temp; bool success = obj->Get(context, V8StringConstants::GetValue(isolate)).ToLocal(&temp); - assert(success && !temp.IsEmpty()); + NS_DCHECK(success && !temp.IsEmpty()); auto valueProp = temp.As(); string num = ConvertToString(valueProp->ToString(context).ToLocalChecked()); diff --git a/test-app/runtime/src/main/cpp/ArrayBufferHelper.cpp b/test-app/runtime/src/main/cpp/ArrayBufferHelper.cpp index 94aabf59e..9236f733e 100644 --- a/test-app/runtime/src/main/cpp/ArrayBufferHelper.cpp +++ b/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 @@ -65,7 +66,7 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo 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 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 } 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); diff --git a/test-app/runtime/src/main/cpp/ArrayHelper.cpp b/test-app/runtime/src/main/cpp/ArrayHelper.cpp index 795c2fc78..668993dbd 100644 --- a/test-app/runtime/src/main/cpp/ArrayHelper.cpp +++ b/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) { 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(); diff --git a/test-app/runtime/src/main/cpp/AssetExtractor.cpp b/test-app/runtime/src/main/cpp/AssetExtractor.cpp index 258a38305..cbbc66821 100644 --- a/test-app/runtime/src/main/cpp/AssetExtractor.cpp +++ b/test-app/runtime/src/main/cpp/AssetExtractor.cpp @@ -1,6 +1,6 @@ #include "jni.h" #include "zip.h" -#include +#include "NativeScriptAssert.h" #include #include #include @@ -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,7 +53,7 @@ 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"); @@ -61,7 +61,7 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin 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; diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index 800f9a6fe..bfe896457 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/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 &caller, const string break; } default: { - assert(false); + NS_DCHECK(false); break; } } @@ -674,7 +675,7 @@ CallbackHandlers::GetMethodOverrides(JEnv &env, const Local &implementat } void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo &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 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 CallbackHandlers::GetTypeMetadata(const string &name, int index) jsize length = env.GetArrayLength(pubApi); - assert(length > 0); + NS_DCHECK(length > 0); vector result; diff --git a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp index 1e0c4a3b5..16226c2f5 100644 --- a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp +++ b/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 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 g_fatalLength{0}; + thread_local Slot* t_slot = nullptr; int CurrentTid() { return static_cast(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) { diff --git a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.h b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.h index deacfa95a..17569dfc9 100644 --- a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.h +++ b/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 diff --git a/test-app/runtime/src/main/cpp/EventLoop.cpp b/test-app/runtime/src/main/cpp/EventLoop.cpp index af9dd2b46..c1092a028 100644 --- a/test-app/runtime/src/main/cpp/EventLoop.cpp +++ b/test-app/runtime/src/main/cpp/EventLoop.cpp @@ -6,7 +6,6 @@ #include #include -#include #include #include #include @@ -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, "", "(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(this))); - assert(!handler.IsNull()); + NS_DCHECK(!handler.IsNull()); handler_ = env.NewGlobalRef(handler); // flush work buffered before the home thread was known diff --git a/test-app/runtime/src/main/cpp/FieldAccessor.cpp b/test-app/runtime/src/main/cpp/FieldAccessor.cpp index 51b3531dd..a2b7bb274 100644 --- a/test-app/runtime/src/main/cpp/FieldAccessor.cpp +++ b/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& 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); } } diff --git a/test-app/runtime/src/main/cpp/File.cpp b/test-app/runtime/src/main/cpp/File.cpp index e61c66a30..21365e7d5 100644 --- a/test-app/runtime/src/main/cpp/File.cpp +++ b/test-app/runtime/src/main/cpp/File.cpp @@ -9,7 +9,6 @@ #include #include #include -#include using namespace std; diff --git a/test-app/runtime/src/main/cpp/FrameCallbacks.cpp b/test-app/runtime/src/main/cpp/FrameCallbacks.cpp index ff96c3ab7..02dc90460 100644 --- a/test-app/runtime/src/main/cpp/FrameCallbacks.cpp +++ b/test-app/runtime/src/main/cpp/FrameCallbacks.cpp @@ -6,7 +6,7 @@ #include #include -#include +#include "NativeScriptAssert.h" #include #include #include @@ -135,7 +135,7 @@ void ResolveFrameCallbacksClass(JEnv& env) { std::call_once(once, [&env] { // JEnv::FindClass caches a global ref to the class FRAME_CALLBACKS_CLASS = env.FindClass("com/tns/FrameCallbacks"); - assert(FRAME_CALLBACKS_CLASS != nullptr); + NS_CHECK(FRAME_CALLBACKS_CLASS != nullptr); FRAME_CALLBACKS_CTOR = env.GetMethodID(FRAME_CALLBACKS_CLASS, "", "(J)V"); FRAME_CALLBACKS_POST = env.GetMethodID(FRAME_CALLBACKS_CLASS, "post", "(J)V"); FRAME_CALLBACKS_RELEASE = @@ -407,7 +407,7 @@ void FrameCallbacks::PostFrameCallback(const FunctionCallbackInfo& args) std::lock_guard lock(entriesMutex_); auto inserted = entries_.emplace( id, std::make_unique(isolate, func, id)); - assert(inserted.second && "Frame callback ID should not be duplicated"); + NS_DCHECK(inserted.second && "Frame callback ID should not be duplicated"); entry = inserted.first->second.get(); } diff --git a/test-app/runtime/src/main/cpp/JEnv.cpp b/test-app/runtime/src/main/cpp/JEnv.cpp index a172f3dde..7cef05520 100644 --- a/test-app/runtime/src/main/cpp/JEnv.cpp +++ b/test-app/runtime/src/main/cpp/JEnv.cpp @@ -1,7 +1,7 @@ #include "JEnv.h" #include -#include +#include "NativeScriptAssert.h" #include "Util.h" #include "NativeScriptException.h" #include "DesugaredInterfaceCompanionClassNameResolver.h" @@ -26,8 +26,8 @@ JEnv::JEnv() if ((ret != JNI_OK) || (env == nullptr)) { ret = s_jvm->AttachCurrentThread(&env, nullptr); - assert(ret == JNI_OK); - assert(env != nullptr); + NS_CHECK(ret == JNI_OK); + NS_CHECK(env != nullptr); } m_env = env; @@ -38,8 +38,8 @@ JEnv::JEnv(JNIEnv *jniEnv) { if ((ret != JNI_OK) || (jniEnv == nullptr)) { ret = s_jvm->AttachCurrentThread(&jniEnv, nullptr); - assert(ret == JNI_OK); - assert(jniEnv != nullptr); + NS_CHECK(ret == JNI_OK); + NS_CHECK(jniEnv != nullptr); } m_env = jniEnv; @@ -862,15 +862,15 @@ jboolean JEnv::IsAssignableFrom(jclass clazz1, jclass clazz2) { } void JEnv::Init(JavaVM *jvm) { - assert(jvm != nullptr); + NS_CHECK(jvm != nullptr); s_jvm = jvm; JEnv env; RUNTIME_CLASS = env.FindClass("com/tns/Runtime"); - assert(RUNTIME_CLASS != nullptr); + NS_CHECK(RUNTIME_CLASS != nullptr); GET_CACHED_CLASS_METHOD_ID = env.GetStaticMethodID(RUNTIME_CLASS, "getCachedClass", "(Ljava/lang/String;)Ljava/lang/Class;"); - assert(GET_CACHED_CLASS_METHOD_ID != nullptr); + NS_CHECK(GET_CACHED_CLASS_METHOD_ID != nullptr); } jclass JEnv::GetObjectClass(jobject obj) { diff --git a/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp b/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp index b509add49..7d5ef9259 100644 --- a/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp +++ b/test-app/runtime/src/main/cpp/JSONObjectHelper.cpp @@ -1,4 +1,5 @@ #include "NativeScriptException.h" +#include "NativeScriptAssert.h" #include "JSONObjectHelper.h" #include "ArgConverter.h" #include "BuiltinLoader.h" @@ -46,7 +47,7 @@ void JSONObjectHelper::RegisterFromFunction(Isolate *isolate, Local& json Local extData = External::New(isolate, serializeFunc, v8::kExternalPointerTypeTagDefault); Local fromFunc; bool ok = FunctionTemplate::New(isolate, ConvertCallbackStatic, extData)->GetFunction(context).ToLocal(&fromFunc); - assert(ok); + NS_DCHECK(ok); jsonObjectFunc->Set(context, fromKey, fromFunc); } diff --git a/test-app/runtime/src/main/cpp/JniSignatureParser.cpp b/test-app/runtime/src/main/cpp/JniSignatureParser.cpp index 3d6df3559..1d421e570 100644 --- a/test-app/runtime/src/main/cpp/JniSignatureParser.cpp +++ b/test-app/runtime/src/main/cpp/JniSignatureParser.cpp @@ -1,6 +1,6 @@ #include "JniSignatureParser.h" -#include +#include "NativeScriptAssert.h" using namespace std; using namespace tns; @@ -12,11 +12,11 @@ JniSignatureParser::JniSignatureParser(const string& signature) vector JniSignatureParser::Parse() { size_t startIdx = m_signature.find_first_of('('); - assert(startIdx != string::npos); + NS_DCHECK(startIdx != string::npos); size_t endIdx = m_signature.find_first_of(')'); - assert(endIdx != string::npos); + NS_DCHECK(endIdx != string::npos); vector tokens = ParseParams(startIdx + 1, endIdx); @@ -60,7 +60,7 @@ string JniSignatureParser::ReadNextToken(int endIdx) { case 'L': idx = m_signature.find(';', m_pos); - assert(idx != string::npos); + NS_DCHECK(idx != string::npos); token = m_signature.substr(m_pos, idx - m_pos + 1); m_pos = idx + 1; break; @@ -88,13 +88,13 @@ string JniSignatureParser::ReadNextToken(int endIdx) { endFound = currChar == ';'; } } - assert(endFound); + NS_DCHECK(endFound); token = m_signature.substr(m_pos, idx - m_pos); m_pos = idx; break; default: - assert(false); + NS_DCHECK(false); break; } diff --git a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp index 174c4119f..68b30d3b7 100644 --- a/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp +++ b/test-app/runtime/src/main/cpp/JsV8InspectorClient.cpp @@ -1,5 +1,4 @@ #include "JsV8InspectorClient.h" -#include #include #include #include @@ -204,16 +203,16 @@ JsV8InspectorClient::JsV8InspectorClient(v8::Isolate* isolate) JEnv env; inspectorClass_ = env.FindClass("com/tns/AndroidJsV8Inspector"); - assert(inspectorClass_ != nullptr); + NS_CHECK(inspectorClass_ != nullptr); sendMethod_ = env.GetStaticMethodID(inspectorClass_, "send", "(Ljava/lang/Object;Ljava/lang/String;)V"); - assert(sendMethod_ != nullptr); + NS_CHECK(sendMethod_ != nullptr); sendToDevToolsConsoleMethod_ = env.GetStaticMethodID(inspectorClass_, "sendToDevToolsConsole", "(Ljava/lang/Object;Ljava/lang/String;Ljava/lang/String;)V"); - assert(sendToDevToolsConsoleMethod_ != nullptr); + NS_CHECK(sendToDevToolsConsoleMethod_ != nullptr); getInspectorMessageMethod_ = env.GetStaticMethodID(inspectorClass_, "getInspectorMessage", "(Ljava/lang/Object;)Ljava/lang/String;"); - assert(getInspectorMessageMethod_ != nullptr); + NS_CHECK(getInspectorMessageMethod_ != nullptr); } void JsV8InspectorClient::connect(jobject connection) { @@ -971,7 +970,7 @@ void JsV8InspectorClient::registerDomainDispatcherCallback(const FunctionCallbac Local ctorArgs[0]; Local domainInstance; bool success = domainCtorFunc->CallAsConstructor(context, 0, ctorArgs).ToLocal(&domainInstance); - assert(success && domainInstance->IsObject()); + NS_DCHECK(success && domainInstance->IsObject()); Local domainObj = domainInstance.As(); Persistent* poDomainObj = new Persistent(isolate, domainObj); @@ -1003,26 +1002,26 @@ void JsV8InspectorClient::registerModules() { // __inspector success = global->Set(context, ArgConverter::ConvertToV8String(isolate, "__inspector"), inspectorObject).FromMaybe(false); - assert(success); + NS_DCHECK(success); // __registerDomainDispatcher success = v8::Function::New(context, registerDomainDispatcherCallback).ToLocal(&func); - assert(success); + NS_DCHECK(success); success = global->Set(context, ArgConverter::ConvertToV8String(isolate, "__registerDomainDispatcher"), func).FromMaybe(false); - assert(success); + NS_DCHECK(success); // __inspectorSendEvent Local data = External::New(isolate, this, v8::kExternalPointerTypeTagDefault); success = v8::Function::New(context, inspectorSendEventCallback, data).ToLocal(&func); - assert(success); + NS_DCHECK(success); success = global->Set(context, ArgConverter::ConvertToV8String(isolate, "__inspectorSendEvent"), func).FromMaybe(false); - assert(success); + NS_DCHECK(success); // __inspectorTimestamp success = v8::Function::New(context, inspectorTimestampCallback).ToLocal(&func); - assert(success); + NS_DCHECK(success); success = global->Set(context, ArgConverter::ConvertToV8String(isolate, "__inspectorTimestamp"), func).FromMaybe(false); - assert(success); + NS_DCHECK(success); TryCatch tc(isolate); Runtime::GetRuntime(isolate)->RunModule("inspector_modules"); diff --git a/test-app/runtime/src/main/cpp/LRUCache.h b/test-app/runtime/src/main/cpp/LRUCache.h index 624120fee..06b401dc2 100644 --- a/test-app/runtime/src/main/cpp/LRUCache.h +++ b/test-app/runtime/src/main/cpp/LRUCache.h @@ -17,7 +17,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include +#include "NativeScriptAssert.h" #include #include "robin_hood.h" @@ -47,8 +47,8 @@ class LRUCache { // the maximum number of records to be stored LRUCache(value_type (*loadCallback)(const key_type&, void*), void (*evictCallback)(const value_type&, void*), bool (*cacheValidCallback)(const key_type&, const value_type&, void*), size_t capacity, void* state) : m_loadCallback(loadCallback), m_capacity(capacity), m_evictCallback(evictCallback), m_cacheValidCallback(cacheValidCallback), m_state(state) { - assert(m_loadCallback != nullptr); - assert((0 < m_capacity) && (m_capacity < 10000)); + NS_DCHECK(m_loadCallback != nullptr); + NS_DCHECK((0 < m_capacity) && (m_capacity < 10000)); } // Obtain value of the cached function for k @@ -138,7 +138,7 @@ class LRUCache { // Record a fresh key-value pair in the cache void insert(const key_type& k, const value_type& v) { // Method is only called on cache misses - assert(m_key_to_value.find(k) == m_key_to_value.end()); + NS_DCHECK(m_key_to_value.find(k) == m_key_to_value.end()); // Make space if necessary if (m_key_to_value.size() == m_capacity) { @@ -158,11 +158,11 @@ class LRUCache { // Purge the least-recently-used element in the cache void evict() { // Assert method is never called when cache is empty - assert(!m_key_tracker.empty()); + NS_DCHECK(!m_key_tracker.empty()); // Identify least recently used key auto it = m_key_to_value.find(m_key_tracker.front()); - assert(it != m_key_to_value.end()); + NS_DCHECK(it != m_key_to_value.end()); if (m_evictCallback != nullptr) { m_evictCallback((*it).second.first, m_state); diff --git a/test-app/runtime/src/main/cpp/MetadataNode.cpp b/test-app/runtime/src/main/cpp/MetadataNode.cpp index 8f3f9713a..d317fa32c 100644 --- a/test-app/runtime/src/main/cpp/MetadataNode.cpp +++ b/test-app/runtime/src/main/cpp/MetadataNode.cpp @@ -830,7 +830,7 @@ vector MetadataNode::SetInstanceMembersFromRu MetadataTreeNode* treeNode) { SET_PROFILER_FRAME(); - assert(treeNode->metadata != nullptr); + NS_DCHECK(treeNode->metadata != nullptr); std::vector instanceMethodData; @@ -857,7 +857,7 @@ vector MetadataNode::SetInstanceMembersFromRu char chKind = kind[0]; // method or field - assert((chKind == 'M') || (chKind == 'F')); + NS_DCHECK((chKind == 'M') || (chKind == 'F')); MetadataEntry entry(nullptr, NodeType::Field); @@ -1919,7 +1919,7 @@ MetadataNode* MetadataNode::GetNodeFromHandle(const Local& value) { } MetadataEntry MetadataNode::GetChildMetadataForPackage(MetadataNode *node, const std::string &propName) { - assert(node->m_treeNode->children != nullptr); + NS_DCHECK(node->m_treeNode->children != nullptr); MetadataEntry child(nullptr, NodeType::Class); @@ -2002,7 +2002,7 @@ void MetadataNode::BuildMetadata(const string& filesPath) { } fseek(f, 0, SEEK_END); int lenNodes = ftell(f); - assert((lenNodes % sizeof(MetadataTreeNodeRawData)) == 0); + NS_DCHECK((lenNodes % sizeof(MetadataTreeNodeRawData)) == 0); char* nodes = new char[lenNodes]; rewind(f); fread(nodes, 1, lenNodes, f); diff --git a/test-app/runtime/src/main/cpp/MetadataReader.cpp b/test-app/runtime/src/main/cpp/MetadataReader.cpp index 127931941..3dc214260 100644 --- a/test-app/runtime/src/main/cpp/MetadataReader.cpp +++ b/test-app/runtime/src/main/cpp/MetadataReader.cpp @@ -1,4 +1,5 @@ #include "MetadataReader.h" +#include "NativeScriptAssert.h" #include "MetadataMethodInfo.h" #include #include "NativeScriptException.h" @@ -42,7 +43,7 @@ void MetadataReader::StateMutex::Lock() { void MetadataReader::StateMutex::Unlock() { std::lock_guard guard(mutex_); // An unmatched unlock would wrap depth_ and hold the mutex forever. - assert(depth_ > 0 && owner_ == std::this_thread::get_id()); + NS_DCHECK(depth_ > 0 && owner_ == std::this_thread::get_id()); if (--depth_ == 0) { owner_ = std::thread::id(); // Every waiter is blocked on the same `depth_ == 0`, so waking one is @@ -55,7 +56,7 @@ unsigned MetadataReader::StateMutex::ReleaseAll() { std::lock_guard guard(mutex_); // Only the owner may release: doing this from a non-owner would drop // another thread's lock while it is still inside its guarded section. - assert(depth_ > 0 && owner_ == std::this_thread::get_id()); + NS_DCHECK(depth_ > 0 && owner_ == std::this_thread::get_id()); unsigned held = depth_; depth_ = 0; owner_ = std::thread::id(); @@ -233,7 +234,7 @@ uint16_t MetadataReader::GetNodeId(MetadataTreeNode *treeNode) { StateLock lock(m_stateMutex); auto itFound = find(m_v.begin(), m_v.end(), treeNode); - assert(itFound != m_v.end()); + NS_DCHECK(itFound != m_v.end()); uint16_t nodeId = itFound - m_v.begin(); return nodeId; @@ -391,7 +392,7 @@ MetadataTreeNode *MetadataReader::GetOrCreateTreeNodeByName(const string &classN auto cKind = kind[0]; // package, class, interface - assert((cKind == 'P') || (cKind == 'C') || (cKind == 'I')); + NS_DCHECK((cKind == 'P') || (cKind == 'C') || (cKind == 'I')); if ((cKind == 'C') || (cKind == 'I')) { child->metadata = new string(part); @@ -406,7 +407,7 @@ MetadataTreeNode *MetadataReader::GetOrCreateTreeNodeByName(const string &classN baseClassLine >> kind >> name; cKind = kind[0]; - assert(cKind == 'B'); + NS_DCHECK(cKind == 'B'); auto baseClassTreeNode = GetOrCreateTreeNodeByName(name); auto baseClassNodeId = GetNodeId(baseClassTreeNode); diff --git a/test-app/runtime/src/main/cpp/MetadataReader.h b/test-app/runtime/src/main/cpp/MetadataReader.h index 3e20b47f1..76ade1e1c 100644 --- a/test-app/runtime/src/main/cpp/MetadataReader.h +++ b/test-app/runtime/src/main/cpp/MetadataReader.h @@ -8,7 +8,7 @@ #include #include #include -#include +#include "NativeScriptAssert.h" #include "robin_hood.h" namespace tns { @@ -117,7 +117,7 @@ namespace tns { std::string name(ptr, len); - assert(name.length() == len); + NS_DCHECK(name.length() == len); return name; } @@ -214,7 +214,7 @@ namespace tns { : MethodReturnType::Object; break; default: - assert(false); + NS_DCHECK(false); break; } return retType; diff --git a/test-app/runtime/src/main/cpp/MethodCache.cpp b/test-app/runtime/src/main/cpp/MethodCache.cpp index 4b19d9d95..a94196313 100644 --- a/test-app/runtime/src/main/cpp/MethodCache.cpp +++ b/test-app/runtime/src/main/cpp/MethodCache.cpp @@ -32,13 +32,13 @@ void MethodCache::Init() { JEnv env; RUNTIME_CLASS = env.FindClass("com/tns/Runtime"); - assert(RUNTIME_CLASS != nullptr); + NS_CHECK(RUNTIME_CLASS != nullptr); RESOLVE_METHOD_OVERLOAD_METHOD_ID = env.GetMethodID(RUNTIME_CLASS, "resolveMethodOverload", "(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/String;"); - assert(RESOLVE_METHOD_OVERLOAD_METHOD_ID != nullptr); + NS_CHECK(RESOLVE_METHOD_OVERLOAD_METHOD_ID != nullptr); RESOLVE_CONSTRUCTOR_SIGNATURE_ID = env.GetMethodID(RUNTIME_CLASS, "resolveConstructorSignature", "(Ljava/lang/Class;[Ljava/lang/Object;)Ljava/lang/String;"); - assert(RESOLVE_CONSTRUCTOR_SIGNATURE_ID != nullptr); + NS_CHECK(RESOLVE_CONSTRUCTOR_SIGNATURE_ID != nullptr); } MethodCache::CacheMethodInfo MethodCache::ResolveMethodSignature(const string& className, const string& methodName, const FunctionCallbackInfo& args, bool isStatic) { @@ -60,7 +60,7 @@ MethodCache::CacheMethodInfo MethodCache::ResolveMethodSignature(const string& c if (!signature.empty()) { JEnv env; auto clazz = env.FindClass(className); - assert(clazz != nullptr); + NS_DCHECK(clazz != nullptr); method_info.clazz = clazz; method_info.signature = signature; method_info.returnType = MetadataReader::ParseReturnType(method_info.signature); diff --git a/test-app/runtime/src/main/cpp/ModuleInternal.cpp b/test-app/runtime/src/main/cpp/ModuleInternal.cpp index 8b1b3538d..2723820a3 100644 --- a/test-app/runtime/src/main/cpp/ModuleInternal.cpp +++ b/test-app/runtime/src/main/cpp/ModuleInternal.cpp @@ -100,13 +100,13 @@ void ModuleInternal::Init(Isolate* isolate, const string& baseDir) { if (MODULE_CLASS == nullptr) { MODULE_CLASS = env.FindClass("com/tns/Module"); - assert(MODULE_CLASS != nullptr); + NS_CHECK(MODULE_CLASS != nullptr); RESOLVE_PATH_METHOD_ID = env.GetStaticMethodID(MODULE_CLASS, "resolvePath", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"); - assert(RESOLVE_PATH_METHOD_ID != nullptr); + NS_CHECK(RESOLVE_PATH_METHOD_ID != nullptr); GET_APPLICATION_FILES_PATH_METHOD_ID = env.GetStaticMethodID(MODULE_CLASS, "getApplicationFilesPath", "()Ljava/lang/String;"); - assert(GET_APPLICATION_FILES_PATH_METHOD_ID != nullptr); + NS_CHECK(GET_APPLICATION_FILES_PATH_METHOD_ID != nullptr); } m_isolate = isolate; @@ -118,7 +118,7 @@ void ModuleInternal::Init(Isolate* isolate, const string& baseDir) { Local result; auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result); - assert(success && result->IsFunction()); + NS_DCHECK(success && result->IsFunction()); auto requireFactoryFunction = result.As(); @@ -161,7 +161,7 @@ Local ModuleInternal::GetRequireFunction(Isolate* isolate, const strin auto thiz = Object::New(isolate); auto success = requireFuncFactory->Call(context, thiz, 2, args).ToLocal(&result); - assert(success && !result.IsEmpty() && result->IsFunction()); + NS_DCHECK(success && !result.IsEmpty() && result->IsFunction()); requireFunc = result.As(); @@ -242,7 +242,7 @@ void ModuleInternal::RequireCallbackImpl(const v8::FunctionCallbackInfo exportsVal; moduleObj->Get(context, ArgConverter::ConvertToV8String(isolate, "exports")).ToLocal(&exportsVal); - assert(!exportsVal.IsEmpty()); + NS_DCHECK(!exportsVal.IsEmpty()); auto exportsObj = exportsVal.As(); args.GetReturnValue().Set(exportsObj); diff --git a/test-app/runtime/src/main/cpp/NativeScriptAssert.cpp b/test-app/runtime/src/main/cpp/NativeScriptAssert.cpp new file mode 100644 index 000000000..38be05fe0 --- /dev/null +++ b/test-app/runtime/src/main/cpp/NativeScriptAssert.cpp @@ -0,0 +1,32 @@ +#include "NativeScriptAssert.h" + +#include +#include +#include + +#include "CrashBreadcrumbs.h" + +namespace tns { + +void OnCheckFailed(const char* expression, const char* file, int line) { + // The build compiles with absolute paths, and the leading directories are + // shared by every file in the runtime. + const char* separator = strrchr(file, '/'); + if (separator != nullptr) { + file = separator + 1; + } + + char message[256]; + snprintf(message, sizeof(message), "NS_CHECK failed: %s, at %s:%d", + expression, file, line); + + CrashBreadcrumbs::RecordFatal(message); + // ANDROID_LOG_FATAL rather than an error: liblog hands a fatal record to + // android_set_abort_message, which is what puts this line in the tombstone + // beside the abort. Nothing has claimed that slot yet on this path. + DEBUG_WRITE_FATAL("%s", message); + + abort(); +} + +} // namespace tns diff --git a/test-app/runtime/src/main/cpp/NativeScriptAssert.h b/test-app/runtime/src/main/cpp/NativeScriptAssert.h index 3a6196845..7ae313352 100644 --- a/test-app/runtime/src/main/cpp/NativeScriptAssert.h +++ b/test-app/runtime/src/main/cpp/NativeScriptAssert.h @@ -16,6 +16,50 @@ extern bool LogEnabled; #define DEBUG_WRITE(fmt, args...) if (tns::LogEnabled) __android_log_print(ANDROID_LOG_DEBUG, "TNS.Native", fmt, ##args) #define DEBUG_WRITE_FORCE(fmt, args...) __android_log_print(ANDROID_LOG_DEBUG, "TNS.Native", fmt, ##args) #define DEBUG_WRITE_FATAL(fmt, args...) __android_log_print(ANDROID_LOG_FATAL, "TNS.Native", fmt, ##args) + +/* + * Reports a failed NS_CHECK and kills the process. The message is left in the + * crash breadcrumb file and claimed as the abort message, so a check that + * fires on a user's device names itself instead of arriving as an unattributed + * SIGABRT. + */ +[[noreturn]] void OnCheckFailed(const char* expression, const char* file, + int line); } +/* + * Aborts unless the expression holds, in every build configuration. + * + * For invariants whose violation leaves no way to continue -- the alternative + * at these sites is undefined behaviour a few statements later, which surfaces + * as a tombstone pointing at whatever happened to run next. + */ +#define NS_CHECK(...) \ + do { \ + if (!(__VA_ARGS__)) { \ + ::tns::OnCheckFailed(#__VA_ARGS__, __FILE__, __LINE__); \ + } \ + } while (false) + +/* + * NS_CHECK in debug builds, nothing in released ones. + * + * Shipped builds are compiled RelWithDebInfo, which defines NDEBUG, so the + * expression never runs there. It must be free of side effects -- anything the + * program depends on has to be evaluated outside the macro: + * + * bool success = obj->Set(context, key, value).FromMaybe(false); + * NS_DCHECK(success); + * + * The expression is still compiled when NDEBUG is defined, so one that stops + * making sense is a build failure rather than something only a debug build + * notices. + */ +#ifdef NDEBUG +#define NS_DCHECK(...) \ + (true ? static_cast(0) : static_cast((__VA_ARGS__))) +#else +#define NS_DCHECK(...) NS_CHECK(__VA_ARGS__) +#endif + #endif /* NATIVESCRIPTASSERT_H_ */ diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp index 6bf64e173..cb6b4e763 100644 --- a/test-app/runtime/src/main/cpp/NativeScriptException.cpp +++ b/test-app/runtime/src/main/cpp/NativeScriptException.cpp @@ -364,34 +364,34 @@ void NativeScriptException::Init() { JEnv env; RUNTIME_CLASS = env.FindClass("com/tns/Runtime"); - assert(RUNTIME_CLASS != nullptr); + NS_CHECK(RUNTIME_CLASS != nullptr); THROWABLE_CLASS = env.FindClass("java/lang/Throwable"); - assert(THROWABLE_CLASS != nullptr); + NS_CHECK(THROWABLE_CLASS != nullptr); NATIVESCRIPTEXCEPTION_CLASS = env.FindClass("com/tns/NativeScriptException"); - assert(NATIVESCRIPTEXCEPTION_CLASS != nullptr); + NS_CHECK(NATIVESCRIPTEXCEPTION_CLASS != nullptr); NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID = env.GetMethodID(NATIVESCRIPTEXCEPTION_CLASS, "", "(Ljava/lang/String;Ljava/lang/String;J)V"); - assert(NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID != nullptr); + NS_CHECK(NATIVESCRIPTEXCEPTION_JSVALUE_CTOR_ID != nullptr); NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID = env.GetMethodID( NATIVESCRIPTEXCEPTION_CLASS, "", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/Throwable;)V"); - assert(NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID != nullptr); + NS_CHECK(NATIVESCRIPTEXCEPTION_THROWABLE_CTOR_ID != nullptr); 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); + NS_CHECK(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); + NS_CHECK(NATIVESCRIPTEXCEPTION_GET_MESSAGE_METHOD_ID != nullptr); } std::string NativeScriptException::ToString() const { diff --git a/test-app/runtime/src/main/cpp/ObjectManager.cpp b/test-app/runtime/src/main/cpp/ObjectManager.cpp index d900371a1..56c54fdcd 100644 --- a/test-app/runtime/src/main/cpp/ObjectManager.cpp +++ b/test-app/runtime/src/main/cpp/ObjectManager.cpp @@ -28,11 +28,11 @@ ObjectManager::ObjectManager(jobject javaRuntimeObject) InitializeJNI(); auto runtimeClass = env.FindClass("com/tns/Runtime"); - assert(runtimeClass != nullptr); + NS_CHECK(runtimeClass != nullptr); auto useGlobalRefsMethodID = env.GetStaticMethodID(runtimeClass, "useGlobalRefs", "()Z"); - assert(useGlobalRefsMethodID != nullptr); + NS_CHECK(useGlobalRefsMethodID != nullptr); auto useGlobalRefs = env.CallStaticBooleanMethod(runtimeClass, useGlobalRefsMethodID); @@ -51,38 +51,38 @@ void ObjectManager::InitializeJNI() { } JEnv env; auto runtimeClass = env.FindClass("com/tns/Runtime"); - assert(runtimeClass != nullptr); + NS_CHECK(runtimeClass != nullptr); GET_JAVAOBJECT_BY_ID_METHOD_ID = env.GetMethodID( runtimeClass, "getJavaObjectByID", "(I)Ljava/lang/Object;"); - assert(GET_JAVAOBJECT_BY_ID_METHOD_ID != nullptr); + NS_CHECK(GET_JAVAOBJECT_BY_ID_METHOD_ID != nullptr); GET_OR_CREATE_JAVA_OBJECT_ID_METHOD_ID = env.GetMethodID( runtimeClass, "getOrCreateJavaObjectID", "(Ljava/lang/Object;)I"); - assert(GET_OR_CREATE_JAVA_OBJECT_ID_METHOD_ID != nullptr); + NS_CHECK(GET_OR_CREATE_JAVA_OBJECT_ID_METHOD_ID != nullptr); MAKE_INSTANCE_WEAK_BATCH_METHOD_ID = env.GetMethodID( runtimeClass, "makeInstanceWeak", "(Ljava/nio/ByteBuffer;IZ)V"); - assert(MAKE_INSTANCE_WEAK_BATCH_METHOD_ID != nullptr); + NS_CHECK(MAKE_INSTANCE_WEAK_BATCH_METHOD_ID != nullptr); MAKE_INSTANCE_WEAK_AND_CHECK_IF_ALIVE_METHOD_ID = env.GetMethodID(runtimeClass, "makeInstanceWeakAndCheckIfAlive", "(I)Z"); - assert(MAKE_INSTANCE_WEAK_AND_CHECK_IF_ALIVE_METHOD_ID != nullptr); + NS_CHECK(MAKE_INSTANCE_WEAK_AND_CHECK_IF_ALIVE_METHOD_ID != nullptr); RELEASE_NATIVE_INSTANCE_METHOD_ID = env.GetMethodID(runtimeClass, "releaseNativeCounterpart", "(I)V"); - assert(RELEASE_NATIVE_INSTANCE_METHOD_ID != nullptr); + NS_CHECK(RELEASE_NATIVE_INSTANCE_METHOD_ID != nullptr); CHECK_WEAK_OBJECTS_ARE_ALIVE_METHOD_ID = env.GetMethodID(runtimeClass, "checkWeakObjectAreAlive", "(Ljava/nio/ByteBuffer;Ljava/nio/ByteBuffer;I)V"); - assert(CHECK_WEAK_OBJECTS_ARE_ALIVE_METHOD_ID != nullptr); + NS_CHECK(CHECK_WEAK_OBJECTS_ARE_ALIVE_METHOD_ID != nullptr); JAVA_LANG_CLASS = env.FindClass("java/lang/Class"); - assert(JAVA_LANG_CLASS != nullptr); + NS_CHECK(JAVA_LANG_CLASS != nullptr); GET_NAME_METHOD_ID = env.GetMethodID(JAVA_LANG_CLASS, "getName", "()Ljava/lang/String;"); - assert(GET_NAME_METHOD_ID != nullptr); + NS_CHECK(GET_NAME_METHOD_ID != nullptr); } void ObjectManager::SetInstanceIsolate(Isolate* isolate) { @@ -520,7 +520,7 @@ void ObjectManager::ReleaseJSInstance(Persistent* po, throw NativeScriptException(ss.str()); } - assert(po == it->second->target); + NS_DCHECK(po == it->second->target); ObjectWeakCallbackState* callbackState = it->second; m_idToObject.erase(it); @@ -558,7 +558,7 @@ void ObjectManager::ReleaseRegularObjects() { auto obj = Local::New(m_isolate, *po); - assert(!obj.IsEmpty()); + NS_DCHECK(!obj.IsEmpty()); Local gcNum; V8GetPrivateValue(m_isolate, obj, propName, gcNum); @@ -628,14 +628,14 @@ Local ObjectManager::GetEmptyObject(Isolate* isolate) { return Local(); } auto localVal = val.ToLocalChecked(); - assert(localVal->IsObject()); + NS_DCHECK(localVal->IsObject()); auto obj = localVal.As(); return obj; } void ObjectManager::JSWrapperConstructorCallback( const v8::FunctionCallbackInfo& info) { - assert(info.IsConstructCall()); + NS_DCHECK(info.IsConstructCall()); } void ObjectManager::ReleaseNativeCounterpart(v8::Local& object) { diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index ae0b2f611..2057695cd 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -129,11 +129,11 @@ Runtime::Runtime(JNIEnv* env, jobject runtime, int id) if (GET_USED_MEMORY_METHOD_ID == nullptr) { auto RUNTIME_CLASS = env->FindClass("com/tns/Runtime"); - assert(RUNTIME_CLASS != nullptr); + NS_CHECK(RUNTIME_CLASS != nullptr); GET_USED_MEMORY_METHOD_ID = env->GetMethodID(RUNTIME_CLASS, "getUsedMemory", "()J"); - assert(GET_USED_MEMORY_METHOD_ID != nullptr); + NS_CHECK(GET_USED_MEMORY_METHOD_ID != nullptr); } } diff --git a/test-app/runtime/src/main/cpp/StructuredClone.cpp b/test-app/runtime/src/main/cpp/StructuredClone.cpp index 0c4d17feb..cc80b4d45 100644 --- a/test-app/runtime/src/main/cpp/StructuredClone.cpp +++ b/test-app/runtime/src/main/cpp/StructuredClone.cpp @@ -1,6 +1,6 @@ #include "StructuredClone.h" -#include +#include "NativeScriptAssert.h" #include "ArgConverter.h" #include "BuiltinLoader.h" @@ -48,20 +48,20 @@ void StructuredClone::Init(Local context) { Local clone; bool success = v8::Function::New(context, CloneCallback).ToLocal(&clone); - assert(success); + NS_DCHECK(success); Local binding = Object::New(isolate); success = binding->Set(context, ArgConverter::ConvertToV8String(isolate, "clone"), clone) .FromMaybe(false); - assert(success); + NS_DCHECK(success); Local result; success = BuiltinLoader::RunBuiltin(context, BuiltinId::kStructuredClone, binding) .ToLocal(&result); - assert(success); + NS_DCHECK(success); } } // namespace tns diff --git a/test-app/runtime/src/main/cpp/StructuredSerialization.cpp b/test-app/runtime/src/main/cpp/StructuredSerialization.cpp index 74d93594d..b3fb4fac9 100644 --- a/test-app/runtime/src/main/cpp/StructuredSerialization.cpp +++ b/test-app/runtime/src/main/cpp/StructuredSerialization.cpp @@ -1,6 +1,6 @@ #include "StructuredSerialization.h" -#include +#include "NativeScriptAssert.h" #include "ArgConverter.h" @@ -18,7 +18,7 @@ void ThrowDataCloneError(Isolate* isolate, const std::string& message) { ->Set(context, ArgConverter::ConvertToV8String(isolate, "name"), ArgConverter::ConvertToV8String(isolate, "DataCloneError")) .FromMaybe(false); - assert(success); + NS_DCHECK(success); isolate->ThrowException(error); } @@ -168,7 +168,7 @@ Maybe SerializedValue::Serialize(Isolate* isolate, Local context, HostObjectPolicy hostObjectPolicy) { HandleScope handleScope(isolate); Context::Scope contextScope(context); - assert(buffer_ == nullptr); + NS_DCHECK(buffer_ == nullptr); std::vector> transfers; if (!CollectTransferList(isolate, context, transferList, transfers)) { diff --git a/test-app/runtime/src/main/cpp/V8StringConstants.cpp b/test-app/runtime/src/main/cpp/V8StringConstants.cpp index d018cd168..897f68a52 100644 --- a/test-app/runtime/src/main/cpp/V8StringConstants.cpp +++ b/test-app/runtime/src/main/cpp/V8StringConstants.cpp @@ -1,4 +1,5 @@ #include "V8StringConstants.h" +#include "NativeScriptAssert.h" #include "Runtime.h" using namespace v8; @@ -10,7 +11,7 @@ V8StringConstants::PerIsolateV8Constants* V8StringConstants::GetConstantsForIsol auto consts = reinterpret_cast(data); // assert that the structure which contains the constants is not null for the current Isolate - assert(consts != nullptr); + NS_CHECK(consts != nullptr); return consts; } diff --git a/test-app/runtime/src/main/cpp/WeakRef.cpp b/test-app/runtime/src/main/cpp/WeakRef.cpp index d6b429108..68c85512f 100644 --- a/test-app/runtime/src/main/cpp/WeakRef.cpp +++ b/test-app/runtime/src/main/cpp/WeakRef.cpp @@ -1,4 +1,5 @@ #include "WeakRef.h" +#include "NativeScriptAssert.h" #include "ArgConverter.h" #include "BuiltinLoader.h" #include "V8StringConstants.h" @@ -15,5 +16,5 @@ WeakRef::WeakRef() { void WeakRef::Init(v8::Isolate* isolate, Local context) { bool success = !BuiltinLoader::RunBuiltin(context, BuiltinId::kWeakRef).IsEmpty(); - assert(success); + NS_DCHECK(success); } diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index a8ac7bb6f..503da71c5 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -573,7 +573,7 @@ void WorkerWrapper::EnsureJniCached() { JEnv env; RUNTIME_CLASS = env.FindClass("com/tns/Runtime"); - assert(RUNTIME_CLASS != nullptr); + NS_CHECK(RUNTIME_CLASS != nullptr); INIT_WORKER_RUNTIME_METHOD_ID = env.GetStaticMethodID(RUNTIME_CLASS, "initWorkerRuntime", "(ILjava/lang/String;)I"); RUN_WORKER_LOOP_METHOD_ID = env.GetStaticMethodID(RUNTIME_CLASS, "runWorkerLoop", "()V"); @@ -581,13 +581,13 @@ void WorkerWrapper::EnsureJniCached() { env.GetStaticMethodID(RUNTIME_CLASS, "detachWorkerRuntime", "(I)V"); LOOPER_CLASS = env.FindClass("android/os/Looper"); - assert(LOOPER_CLASS != nullptr); + NS_CHECK(LOOPER_CLASS != nullptr); MY_LOOPER_METHOD_ID = env.GetStaticMethodID(LOOPER_CLASS, "myLooper", "()Landroid/os/Looper;"); LOOPER_QUIT_METHOD_ID = env.GetMethodID(LOOPER_CLASS, "quit", "()V"); PROCESS_CLASS = env.FindClass("android/os/Process"); - assert(PROCESS_CLASS != nullptr); + NS_CHECK(PROCESS_CLASS != nullptr); SET_THREAD_PRIORITY_METHOD_ID = env.GetStaticMethodID(PROCESS_CLASS, "setThreadPriority", "(I)V"); } diff --git a/test-app/runtime/src/main/cpp/console/Console.cpp b/test-app/runtime/src/main/cpp/console/Console.cpp index ae57213bb..b28e821f0 100644 --- a/test-app/runtime/src/main/cpp/console/Console.cpp +++ b/test-app/runtime/src/main/cpp/console/Console.cpp @@ -3,7 +3,7 @@ // #include -#include +#include "NativeScriptAssert.h" #include #include #include @@ -60,7 +60,7 @@ v8::Local Console::createConsole(v8::Local context, Con v8::Local console = v8::Object::New(isolate); bool success = console->SetPrototype(context, v8::Object::New(isolate)).FromMaybe(false); - assert(success); + NS_DCHECK(success); bindFunctionProperty(context, console, "assert", assertCallback); diff --git a/test-app/runtime/src/main/cpp/utils/PageResources.cpp b/test-app/runtime/src/main/cpp/utils/PageResources.cpp index 165a9dc1f..6b1fa10f7 100644 --- a/test-app/runtime/src/main/cpp/utils/PageResources.cpp +++ b/test-app/runtime/src/main/cpp/utils/PageResources.cpp @@ -4,7 +4,6 @@ #include #include -#include #include #include #include @@ -29,16 +28,16 @@ std::map PageResource::getPageResources() { auto result = std::map(); tns::JEnv env; jclass inspectorClass = env.FindClass("com/tns/AndroidJsV8Inspector"); - assert(inspectorClass != nullptr); + NS_CHECK(inspectorClass != nullptr); jclass pairClass = env.FindClass("android/util/Pair"); - assert(pairClass != nullptr); + NS_CHECK(pairClass != nullptr); jfieldID pairFirst = env.GetFieldID(pairClass, "first", "Ljava/lang/Object;"); - assert(pairFirst != nullptr); + NS_CHECK(pairFirst != nullptr); jfieldID pairSecond = env.GetFieldID(pairClass, "second", "Ljava/lang/Object;"); - assert(pairSecond != nullptr); + NS_CHECK(pairSecond != nullptr); jmethodID getResourcesMethod = env.GetStaticMethodID(inspectorClass, "getPageResources", "()[Landroid/util/Pair;"); jobject arrayOfPairs = env.CallStaticObjectMethod(inspectorClass, getResourcesMethod); From f0251e1e333bb20ac8e6ad42914a29cd9f2defe9 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 15 Aug 2026 15:50:17 -0300 Subject: [PATCH 2/5] refactor: convert the tracing agent's asserts too ns-v8-tracing-agent-impl.cpp is a first-party source -- CMakeLists builds it alongside the rest -- but it sits under v8_inspector/, which the previous commit skipped as vendored. It called assert() while picking up transitively from MetadataReader.h, so replacing that include broke it in both configurations. Its three checks follow ToLocal() on a MaybeLocal, which is the group that keeps debug-only semantics. --- .../src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp b/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp index 1d4c4c52e..0f4b6c54d 100644 --- a/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp +++ b/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp @@ -10,6 +10,7 @@ //#include //#include #include +#include "NativeScriptAssert.h" #include "ns-v8-tracing-agent-impl.h" #include "Runtime.h" @@ -151,17 +152,17 @@ namespace tns { v8::Local script; bool success = v8::Script::Compile(context, tns::ArgConverter::ToV8String(isolate, source)).ToLocal(&script); - assert(success && !script.IsEmpty()); + NS_DCHECK(success && !script.IsEmpty()); v8::Local result; success = script->Run(context).ToLocal(&result); - assert(success); + NS_DCHECK(success); v8::Local processTraceData = result.As(); v8::Local args[1] = { tns::ArgConverter::ToV8String(isolate, jsonData) }; success = processTraceData->Call(context, processTraceData, 1, args).ToLocal(&result); - assert(success); + NS_DCHECK(success); } From 036fe53d221c56c25e50b2690ab9594942b7ef7d Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 15 Aug 2026 16:27:09 -0300 Subject: [PATCH 3/5] refactor: address review of the check macros CallbackHandlers validated ENABLE_VERBOSE_LOGGING_METHOD_ID twice and never DISABLE_VERBOSE_LOGGING_METHOD_ID, which CallJavaMethod passes to CallVoidMethod. Pre-existing, and the previous commit would have frozen the wrong check in permanently. CrashBreadcrumbs: OpenStore sized its reader for the header and the runtime state but not for the fatal message the handler now writes between them, so a full record lost its tail. RecordFatal also published a length without claiming the buffer, letting a second thread failing a check at the same moment overlap the copy the handler reads; the first caller now wins, as with g_recorded. Promoted to NS_CHECK, all values that are stored or dereferenced unconditionally and cannot be reached from application JavaScript: the EventLoop handler object, the require factory and per-directory require functions, StructuredClone's init sequence, the tracing agent's compile and call results, and LRUCache's load callback -- which is a raw function pointer, not a std::function, so calling it null is undefined rather than a throw. Per-call paths that application JavaScript can reach stay NS_DCHECK, because a throwing getter has to propagate as an exception rather than kill the process. ArgConverter::ConvertToJavaLong is the clearest example. AssetExtractor now handles libzip failures rather than checking them: a null zip_fopen_index skips the entry instead of reaching zip_fread and zip_fclose, and a zip_fread result of 0 or -1 ends the copy loop instead of spinning or handing fwrite a negative length widened to size_t. BuildMetadata throws NativeScriptException when treeNodeStream.dat is not a whole number of records, matching how the same function already reports a file it cannot open. --- .../runtime/src/main/cpp/AssetExtractor.cpp | 27 ++++++++++++++++--- .../runtime/src/main/cpp/CallbackHandlers.cpp | 2 +- .../runtime/src/main/cpp/CrashBreadcrumbs.cpp | 9 ++++++- test-app/runtime/src/main/cpp/EventLoop.cpp | 2 +- test-app/runtime/src/main/cpp/LRUCache.h | 4 +-- .../runtime/src/main/cpp/MetadataNode.cpp | 11 +++++++- .../runtime/src/main/cpp/ModuleInternal.cpp | 4 +-- .../runtime/src/main/cpp/StructuredClone.cpp | 6 ++--- .../v8_inspector/ns-v8-tracing-agent-impl.cpp | 6 ++--- 9 files changed, 53 insertions(+), 18 deletions(-) diff --git a/test-app/runtime/src/main/cpp/AssetExtractor.cpp b/test-app/runtime/src/main/cpp/AssetExtractor.cpp index cbbc66821..864c1e45e 100644 --- a/test-app/runtime/src/main/cpp/AssetExtractor.cpp +++ b/test-app/runtime/src/main/cpp/AssetExtractor.cpp @@ -22,7 +22,9 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin int err = 0; auto z = zip_open(strApk.c_str(), 0, &err); - NS_DCHECK(z != nullptr); + // Nothing downstream can run without the app's assets, and every call + // below dereferences the archive. + NS_CHECK(z != nullptr); zip_int64_t num = zip_get_num_entries(z, 0); struct zip_stat sb; struct zip_file* zf; @@ -53,15 +55,32 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin mkdir_rec(dirFullname.c_str()); zf = zip_fopen_index(z, i, 0); - NS_DCHECK(zf != nullptr); + if (zf == nullptr) { + DEBUG_WRITE_FORCE( + "AssetExtractor: skipping '%s', it could not be opened " + "inside the apk (%s)", + sb.name, zip_strerror(z)); + continue; + } auto fd = fopen(assetFullname.c_str(), "w"); if (fd != nullptr) { zip_int64_t sum = 0; - while (sum != sb.size) { + while (sum != (zip_int64_t)sb.size) { zip_int64_t len = zip_fread(zf, buf, sizeof(buf)); - NS_DCHECK(len > 0); + // 0 means the entry ended early, -1 a read error. Both + // leave the loop unable to advance, and -1 would reach + // fwrite as a huge size_t. + if (len <= 0) { + DEBUG_WRITE_FORCE( + "AssetExtractor: '%s' ended after %lld of %llu " + "bytes (%s)", + sb.name, (long long)sum, + (unsigned long long)sb.size, + zip_file_strerror(zf)); + break; + } fwrite(buf, 1, len, fd); sum += len; diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index bfe896457..cf8b5188d 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -57,7 +57,7 @@ void CallbackHandlers::Init(Isolate *isolate) { DISABLE_VERBOSE_LOGGING_METHOD_ID = env.GetMethodID(RUNTIME_CLASS, "disableVerboseLogging", "()V"); - NS_CHECK(ENABLE_VERBOSE_LOGGING_METHOD_ID != nullptr); + NS_CHECK(DISABLE_VERBOSE_LOGGING_METHOD_ID != nullptr); MetadataNode::Init(isolate); diff --git a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp index 16226c2f5..234355748 100644 --- a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp +++ b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp @@ -51,6 +51,7 @@ struct sigaction g_previous[NSIG]; */ char g_fatalMessage[kFatalMax]; std::atomic g_fatalLength{0}; +std::atomic_flag g_fatalClaimed = ATOMIC_FLAG_INIT; thread_local Slot* t_slot = nullptr; @@ -228,7 +229,7 @@ void CrashBreadcrumbs::OpenStore(const std::string& filesRoot) { return; } - char previous[kBufferMax + kHeaderMax]; + char previous[kHeaderMax + kFatalMax + kBufferMax]; ssize_t length = read(fd, previous, sizeof(previous) - 1); if (length > 0) { previous[length] = '\0'; @@ -304,6 +305,12 @@ void CrashBreadcrumbs::RecordFatal(const char* message) { if (message == nullptr) { return; } + // Only the first caller writes. A second thread failing a check at the same + // moment would otherwise overlap this copy, including while the signal + // handler is reading the buffer out. + if (g_fatalClaimed.test_and_set(std::memory_order_acq_rel)) { + return; + } // Room is reserved for the newline and the terminator. size_t length = strnlen(message, kFatalMax - 2); memcpy(g_fatalMessage, message, length); diff --git a/test-app/runtime/src/main/cpp/EventLoop.cpp b/test-app/runtime/src/main/cpp/EventLoop.cpp index c1092a028..13d1df306 100644 --- a/test-app/runtime/src/main/cpp/EventLoop.cpp +++ b/test-app/runtime/src/main/cpp/EventLoop.cpp @@ -127,7 +127,7 @@ void EventLoop::BindToCurrentThread() { } JniLocalRef handler(env.NewObject(EVENT_LOOP_HANDLER_CLASS, EVENT_LOOP_HANDLER_CTOR, reinterpret_cast(this))); - NS_DCHECK(!handler.IsNull()); + NS_CHECK(!handler.IsNull()); handler_ = env.NewGlobalRef(handler); // flush work buffered before the home thread was known diff --git a/test-app/runtime/src/main/cpp/LRUCache.h b/test-app/runtime/src/main/cpp/LRUCache.h index 06b401dc2..87ed14cba 100644 --- a/test-app/runtime/src/main/cpp/LRUCache.h +++ b/test-app/runtime/src/main/cpp/LRUCache.h @@ -47,8 +47,8 @@ class LRUCache { // the maximum number of records to be stored LRUCache(value_type (*loadCallback)(const key_type&, void*), void (*evictCallback)(const value_type&, void*), bool (*cacheValidCallback)(const key_type&, const value_type&, void*), size_t capacity, void* state) : m_loadCallback(loadCallback), m_capacity(capacity), m_evictCallback(evictCallback), m_cacheValidCallback(cacheValidCallback), m_state(state) { - NS_DCHECK(m_loadCallback != nullptr); - NS_DCHECK((0 < m_capacity) && (m_capacity < 10000)); + NS_CHECK(m_loadCallback != nullptr); + NS_CHECK((0 < m_capacity) && (m_capacity < 10000)); } // Obtain value of the cached function for k diff --git a/test-app/runtime/src/main/cpp/MetadataNode.cpp b/test-app/runtime/src/main/cpp/MetadataNode.cpp index d317fa32c..7b4ce5d1b 100644 --- a/test-app/runtime/src/main/cpp/MetadataNode.cpp +++ b/test-app/runtime/src/main/cpp/MetadataNode.cpp @@ -2002,7 +2002,16 @@ void MetadataNode::BuildMetadata(const string& filesPath) { } fseek(f, 0, SEEK_END); int lenNodes = ftell(f); - NS_DCHECK((lenNodes % sizeof(MetadataTreeNodeRawData)) == 0); + if (lenNodes < 0 || + (static_cast(lenNodes) % sizeof(MetadataTreeNodeRawData)) != 0) { + fclose(f); + stringstream ss; + ss << "Metadata file " << nodesFile << " is " << lenNodes + << " bytes, which is not a whole number of " + << sizeof(MetadataTreeNodeRawData) + << "-byte records. The metadata is truncated or corrupt."; + throw NativeScriptException(ss.str()); + } char* nodes = new char[lenNodes]; rewind(f); fread(nodes, 1, lenNodes, f); diff --git a/test-app/runtime/src/main/cpp/ModuleInternal.cpp b/test-app/runtime/src/main/cpp/ModuleInternal.cpp index 2723820a3..79f7aa0f6 100644 --- a/test-app/runtime/src/main/cpp/ModuleInternal.cpp +++ b/test-app/runtime/src/main/cpp/ModuleInternal.cpp @@ -118,7 +118,7 @@ void ModuleInternal::Init(Isolate* isolate, const string& baseDir) { Local result; auto success = BuiltinLoader::RunBuiltin(context, BuiltinId::kRequireFactory).ToLocal(&result); - NS_DCHECK(success && result->IsFunction()); + NS_CHECK(success && result->IsFunction()); auto requireFactoryFunction = result.As(); @@ -161,7 +161,7 @@ Local ModuleInternal::GetRequireFunction(Isolate* isolate, const strin auto thiz = Object::New(isolate); auto success = requireFuncFactory->Call(context, thiz, 2, args).ToLocal(&result); - NS_DCHECK(success && !result.IsEmpty() && result->IsFunction()); + NS_CHECK(success && !result.IsEmpty() && result->IsFunction()); requireFunc = result.As(); diff --git a/test-app/runtime/src/main/cpp/StructuredClone.cpp b/test-app/runtime/src/main/cpp/StructuredClone.cpp index cc80b4d45..8f906c2dc 100644 --- a/test-app/runtime/src/main/cpp/StructuredClone.cpp +++ b/test-app/runtime/src/main/cpp/StructuredClone.cpp @@ -48,20 +48,20 @@ void StructuredClone::Init(Local context) { Local clone; bool success = v8::Function::New(context, CloneCallback).ToLocal(&clone); - NS_DCHECK(success); + NS_CHECK(success); Local binding = Object::New(isolate); success = binding->Set(context, ArgConverter::ConvertToV8String(isolate, "clone"), clone) .FromMaybe(false); - NS_DCHECK(success); + NS_CHECK(success); Local result; success = BuiltinLoader::RunBuiltin(context, BuiltinId::kStructuredClone, binding) .ToLocal(&result); - NS_DCHECK(success); + NS_CHECK(success); } } // namespace tns diff --git a/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp b/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp index 0f4b6c54d..551bc7963 100644 --- a/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp +++ b/test-app/runtime/src/main/cpp/v8_inspector/ns-v8-tracing-agent-impl.cpp @@ -152,17 +152,17 @@ namespace tns { v8::Local script; bool success = v8::Script::Compile(context, tns::ArgConverter::ToV8String(isolate, source)).ToLocal(&script); - NS_DCHECK(success && !script.IsEmpty()); + NS_CHECK(success && !script.IsEmpty()); v8::Local result; success = script->Run(context).ToLocal(&result); - NS_DCHECK(success); + NS_CHECK(success); v8::Local processTraceData = result.As(); v8::Local args[1] = { tns::ArgConverter::ToV8String(isolate, jsonData) }; success = processTraceData->Call(context, processTraceData, 1, args).ToLocal(&result); - NS_DCHECK(success); + NS_CHECK(success); } From 2c7149edaf08d3240dcf7d7d0f0dbf42be577604 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 15 Aug 2026 16:37:25 -0300 Subject: [PATCH 4/5] refactor: extract assets atomically, and never drop a claimed check Extraction wrote straight into the destination, which fopen truncates before the first read. A read that failed partway therefore replaced a good asset with its opening bytes, and the surrounding code then stamped the apk's mtime onto it. Since an entry is only re-extracted when the apk's copy is strictly newer than what is on disk, the truncated file would have survived every later launch. Removing the remains instead is not enough either: a partial file left with the current time is newer still, so it would also have stuck. Entries now go through a pid-suffixed temporary that is renamed only once the whole entry has been read and written and the stream closed cleanly, so the destination either keeps its previous contents or gains complete ones. A short read, a failed write and a failed close are all reported and discard the temporary. The crash handler could also emit a record without the check that caused it: a thread that had claimed the fatal slot but not yet filled it was indistinguishable from no message at all, so a second thread aborting in that window published a breadcrumb missing the message. The slot now carries a three-state marker, and the handler waits a bounded number of spins for a claim to resolve before deciding. --- .../runtime/src/main/cpp/AssetExtractor.cpp | 43 ++++++++++++++++--- .../runtime/src/main/cpp/CrashBreadcrumbs.cpp | 35 ++++++++++++--- 2 files changed, 65 insertions(+), 13 deletions(-) diff --git a/test-app/runtime/src/main/cpp/AssetExtractor.cpp b/test-app/runtime/src/main/cpp/AssetExtractor.cpp index 864c1e45e..512e9ec97 100644 --- a/test-app/runtime/src/main/cpp/AssetExtractor.cpp +++ b/test-app/runtime/src/main/cpp/AssetExtractor.cpp @@ -3,7 +3,11 @@ #include "NativeScriptAssert.h" #include #include +#include #include +#include +#include +#include #include "AssetExtractor.h" using namespace tns; @@ -63,10 +67,23 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin continue; } - auto fd = fopen(assetFullname.c_str(), "w"); + /* + * Extracted through a temporary and renamed once complete. + * Writing in place would truncate a good asset up front, and a + * read that fails partway would leave the remains behind: the + * mtime comparison above keeps whatever is already on disk + * unless the apk's copy is strictly newer, so a partial file + * would never be replaced on a later launch. + */ + std::string tempFullname(assetFullname); + tempFullname.append(".ns-partial."); + tempFullname.append(std::to_string(getpid())); + + auto fd = fopen(tempFullname.c_str(), "w"); if (fd != nullptr) { zip_int64_t sum = 0; + bool complete = true; while (sum != (zip_int64_t)sb.size) { zip_int64_t len = zip_fread(zf, buf, sizeof(buf)); // 0 means the entry ended early, -1 a read error. Both @@ -79,16 +96,30 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin sb.name, (long long)sum, (unsigned long long)sb.size, zip_file_strerror(zf)); + complete = false; break; } - fwrite(buf, 1, len, fd); + if (fwrite(buf, 1, len, fd) != (size_t)len) { + DEBUG_WRITE_FORCE( + "AssetExtractor: could not write '%s' (errno %d)", + assetFullname.c_str(), errno); + complete = false; + break; + } sum += len; } - fclose(fd); - utimbuf t; - t.modtime = sb.mtime; - utime(assetFullname.c_str(), &t); + + complete = (fclose(fd) == 0) && complete; + + if (complete && rename(tempFullname.c_str(), + assetFullname.c_str()) == 0) { + utimbuf t; + t.modtime = sb.mtime; + utime(assetFullname.c_str(), &t); + } else { + remove(tempFullname.c_str()); + } } zip_fclose(zf); diff --git a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp index 234355748..9fbe6039e 100644 --- a/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp +++ b/test-app/runtime/src/main/cpp/CrashBreadcrumbs.cpp @@ -49,9 +49,18 @@ struct sigaction g_previous[NSIG]; * handler. Kept out of the rendered buffers so that recording it needs no * lock -- the thread may be aborting from under one. */ +enum FatalState { kFatalFree = 0, kFatalClaiming = 1, kFatalPublished = 2 }; + char g_fatalMessage[kFatalMax]; -std::atomic g_fatalLength{0}; -std::atomic_flag g_fatalClaimed = ATOMIC_FLAG_INIT; +size_t g_fatalLength = 0; +std::atomic g_fatalState{kFatalFree}; + +/* + * How long the handler waits on a thread that has claimed the slot but not + * filled it yet. Bounded, so a thread that never gets to publish cannot stall + * the crash path. + */ +constexpr int kFatalSpinLimit = 200000; thread_local Slot* t_slot = nullptr; @@ -167,10 +176,18 @@ void Handler(int signalNumber, siginfo_t* info, void* context) { if (written > 0) { off_t offset = written; - size_t fatalLength = g_fatalLength.load(std::memory_order_acquire); - if (fatalLength > 0) { + // A thread that has claimed the slot is a memcpy away from filling + // it. Waiting for it beats emitting a record that omits the very + // check that brought the process here. + for (int spin = 0; + spin < kFatalSpinLimit && + g_fatalState.load(std::memory_order_acquire) == kFatalClaiming; + ++spin) { + } + + if (g_fatalState.load(std::memory_order_acquire) == kFatalPublished) { ssize_t fatalWritten = - pwrite(fd, g_fatalMessage, fatalLength, offset); + pwrite(fd, g_fatalMessage, g_fatalLength, offset); if (fatalWritten > 0) { offset += fatalWritten; } @@ -308,7 +325,9 @@ void CrashBreadcrumbs::RecordFatal(const char* message) { // Only the first caller writes. A second thread failing a check at the same // moment would otherwise overlap this copy, including while the signal // handler is reading the buffer out. - if (g_fatalClaimed.test_and_set(std::memory_order_acq_rel)) { + int expected = kFatalFree; + if (!g_fatalState.compare_exchange_strong(expected, kFatalClaiming, + std::memory_order_acq_rel)) { return; } // Room is reserved for the newline and the terminator. @@ -316,7 +335,9 @@ void CrashBreadcrumbs::RecordFatal(const char* message) { memcpy(g_fatalMessage, message, length); g_fatalMessage[length] = '\n'; g_fatalMessage[length + 1] = '\0'; - g_fatalLength.store(length + 1, std::memory_order_release); + g_fatalLength = length + 1; + // Publishes the buffer and the length together. + g_fatalState.store(kFatalPublished, std::memory_order_release); } CrashBreadcrumbs::ModuleScope::ModuleScope(const char* modulePath) { From b1686eaf9c4bebcf80d8c8a7869f08090250fb3a Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Sat, 15 Aug 2026 16:45:37 -0300 Subject: [PATCH 5/5] refactor: create the extraction temporary exclusively A pid suffix only separates processes. Two extractions of the same entry within one process would have shared a temporary, truncating each other's output and then racing over the rename and the remove. mkstemp creates the file exclusively and hands back the name it settled on, which is what the rename and the cleanup now use. --- .../runtime/src/main/cpp/AssetExtractor.cpp | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/test-app/runtime/src/main/cpp/AssetExtractor.cpp b/test-app/runtime/src/main/cpp/AssetExtractor.cpp index 512e9ec97..a4460a04b 100644 --- a/test-app/runtime/src/main/cpp/AssetExtractor.cpp +++ b/test-app/runtime/src/main/cpp/AssetExtractor.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include "AssetExtractor.h" @@ -76,10 +77,29 @@ void AssetExtractor::ExtractAssets(JNIEnv* env, jobject obj, jstring apk, jstrin * would never be replaced on a later launch. */ std::string tempFullname(assetFullname); - tempFullname.append(".ns-partial."); - tempFullname.append(std::to_string(getpid())); - - auto fd = fopen(tempFullname.c_str(), "w"); + tempFullname.append(".ns-XXXXXX"); + + // Exclusive creation, so two extractions of the same entry + // cannot land on one temporary. mkstemp writes the name it + // settled on back into the buffer. + FILE* fd = nullptr; + int tempFd = mkstemp(tempFullname.data()); + if (tempFd < 0) { + DEBUG_WRITE_FORCE( + "AssetExtractor: could not create a temporary for '%s' " + "(errno %d)", + assetFullname.c_str(), errno); + } else { + fd = fdopen(tempFd, "w"); + if (fd == nullptr) { + DEBUG_WRITE_FORCE( + "AssetExtractor: could not open the temporary for " + "'%s' (errno %d)", + assetFullname.c_str(), errno); + close(tempFd); + remove(tempFullname.c_str()); + } + } if (fd != nullptr) { zip_int64_t sum = 0;