refactor: replace assert() with NS_CHECK/NS_DCHECK - #2014

Why

assert() never runs in a build we ship.

assembleRelease maps to the RelWithDebInfo CMake config, whose stock CMAKE_CXX_FLAGS_RELWITHDEBINFO carries -DNDEBUG. CMakeLists.txt appends -O3 to that variable (line 113) rather than replacing it, so nothing removes the define. Verified from the generated ninja files: -DNDEBUG appears 82× under .cxx/RelWithDebInfo/ and 0× under .cxx/Debug/.

So all 123 first-party asserts were diagnostics that existed only in debug and test runs — the two environments where the invariants were least likely to be violated in the first place. On a user's device they were absent, and the invariant violation surfaced later as an unattributed SIGSEGV.

The macros are named NS_CHECK/NS_DCHECK rather than CHECK/DCHECK because V8's v8_inspector/src/base/logging.h already defines both bare names and is compiled into debug builds.

What

Two macros in NativeScriptAssert.h (which, despite the filename, previously held only the DEBUG_WRITE family):

NS_DCHECK keeps exactly the semantics assert() had, so its expression must remain free of side effects. It still compiles the expression under NDEBUG in a branch that is never taken, so an expression that stops making sense is a build failure rather than something only a debug build notices.

The split: 73 NS_CHECK, 50 NS_DCHECK

The rule, sharpened during review: NS_CHECK where the value is stored or dereferenced unconditionally and the failure is not reachable from application JavaScript.

That second clause is what keeps the per-call conversion paths debug-only. ArgConverter::ConvertToJavaLong can fail because a JS getter threw, and that has to propagate as an exception rather than abort the process. Init-time failures have no such path — nothing catches them, and the next statement dereferences an empty handle.

Promoted to NS_CHECK — the JNIEnv/JavaVM handles and the jclass/jmethodID/jfieldID lookups resolved once during runtime init from fixed class names, the per-isolate V8StringConstants block, the EventLoop handler object, the require factory and per-directory require functions, StructuredClone::Init, the tracing agent's compile/run/call results, and LRUCache's load callback (a raw function pointer, so a null call is undefined rather than a throw). Each is used unconditionally a statement or two later, so a null is undefined behaviour today.

These are backstops rather than the primary error path: JEnv::GetMethodID and friends already call CheckForJavaException, which throws NativeScriptException. An NS_CHECK here fires only when a lookup returns null with no pending Java exception.

Kept as NS_DCHECK — everything else, including one case worth calling out:

JEnv::FindClass deliberately returns nullptr with a pending Java exception for a genuinely missing class (JEnv.cpp:740-742, 757-761 — "just return null and let the runtime deal with the NativeScriptException").

MethodCache.cpp:63 and FieldAccessor.cpp:217-224 check that result for metadata-driven class names. Promoting those would have converted a designed, recoverable path into a hard abort. They stay debug-only.

Crash reporting

A failed NS_CHECK routes through CrashBreadcrumbs::RecordFatal and logs at ANDROID_LOG_FATAL, which claims the bionic abort-message slot. The check therefore names itself both in the tombstone and in the breadcrumb file the next launch reports, instead of arriving as a bare SIGABRT.

RecordFatal takes no lock and writes into a buffer the signal handler already knows how to emit, so it stays safe on a thread aborting from under one of the runtime's own locks.

Deliberately not included

• clang-tidy bugprone-assert-side-effect. The only mechanical guarantee against a side-effecting NS_DCHECK, but there is no .clang-tidy in the repo, so it is new infrastructure; and with CheckFunctionCalls: true it flags every NS_DCHECK(!x.IsEmpty()) and would need an IgnoredFunctions list.
• An always-evaluating debug-only variant (VERIFY-style). It silently discards failures in production, which is usually the wrong default. If a call must run and its failure matters, that is NS_CHECK.

Three bugs this surfaced

Making the "these never run in production" claim explicit turned up defects that predate the PR:

1. CallbackHandlers never validated DISABLE_VERBOSE_LOGGING_METHOD_ID. Line 60 checked ENABLE_VERBOSE_LOGGING_METHOD_ID a second time, immediately after assigning the disable id — which is passed to CallVoidMethod. Promoting it unchanged would have frozen the wrong check in permanently.

2. AssetExtractor mishandled every libzip failure. A null zip_fopen_index reached zip_fread and zip_fclose; a zip_fread of 0 stalled the copy loop; -1 reached fwrite widened to size_t. Now handled rather than checked.

   Extraction is also atomic now. It wrote straight into the destination, which fopen(..., "w") truncates before the first read, so a failed extraction replaced a good asset with its opening bytes and then stamped the apk's mtime onto it. An entry is only re-extracted when the apk's copy is strictly newer than what is on disk, so that truncated file would have survived every later launch. (Deleting the remains instead would not have helped: a partial file left with the current time is newer still, so it sticks too.) Entries now go through an mkstemp temporary, renamed only once the entry has been fully read, written and closed cleanly. utime runs after the rename, so the apk mtime is still what the up-to-date shortcut compares against.

3. BuildMetadata parsed a truncated treeNodeStream.dat. It now throws NativeScriptException with the actual length and record size, matching how the same function already reports a file it cannot open, and rejects a negative ftell before it reaches new char[lenNodes].

A first-party file under v8_inspector/

The first push broke both CI jobs, in one place. v8_inspector/ns-v8-tracing-agent-impl.cpp is built by CMakeLists like any other source, but it lives under v8_inspector/, which the sweep skipped as vendored. It called assert() while receiving <assert.h> transitively from MetadataReader.h, so replacing that include left it with no declaration — in release for the Build job and in debug for the Test job.

Its three checks follow ToLocal() on a MaybeLocal, so they are NS_DCHECK. Second commit.

Swept for the same signature afterwards: only three other files call assert( without including it directly, and all three are false positives — two are comments (absl/base/options.h, include/v8-fast-api-calls.h) and crdtp/protocol_core.h picks it up from its own status.h. All vendored and untouched.

Testing

Full local run on an arm64 emulator (Pixel_9_Pro_API_35):

• Build, the CI Build job's command (./gradlew --stacktrace): exit 0, 0 compile errors across 7 nested gradle invocations and all four ABIs.
• Debug build + install (:app:installDebug -PuseKotlin=true): exit 0, 0 errors, all four ABIs — this is the configuration the Test job failed in.
• Specs: 879 across 124 suites, 0 failures, 0 errors. Logcat shows 0 NS_CHECK failed, 0 native crashes and 0 breadcrumb reports from a prior process. The one FATAL EXCEPTION: native-crash-on-alien-thread is the intentional UncaughtErrorPolicyTest fixture, and the run continued past it.

Note that a debug build has all 123 checks live — the 50 NS_DCHECK sites as well as the 73 NS_CHECK ones — so a full green suite exercises every converted predicate. Re-run after the review fixes with app data cleared first, so every asset was extracted through the rewritten path: same 879 specs, 0 failures, 0 errors, 0 NS_CHECK fired, no AssetExtractor: diagnostics, and no .ns-partial.* temporaries left behind.

Also verified:

• Both macros compile clean under the repo's own flags (-std=c++20 -Werror) with and without -DNDEBUG, across the statement shapes in the tree: if/else without braces, switch cases, loop bodies, x && "message" predicates, and expressions containing a top-level comma (the macros are variadic, so NS_DCHECK(map.find(k) == map.end()) is one argument).
• Execution semantics confirmed by running a host binary: under NDEBUG, NS_DCHECK(SideEffect()) ran 0 times and NS_CHECK(SideEffect()) ran 1; a failing NS_CHECK aborted in both configurations.
• Every one of the 123 removed lines was an assert( or a now-dead <assert.h>/<cassert> include — checked mechanically against the diff. static_assert, __android_log_assert, assertCallback and the "assert" string literal in Console.cpp are untouched.
• -Wall/-Wextra are not in the flag set, so the discarded-expression form cannot break the -Werror release build on unused-variable warnings.

Summary by CodeRabbit

• Bug Fixes
  • Improved crash diagnostics by recording failed runtime checks in fatal breadcrumbs and logs.
  • Strengthened validation of runtime and JNI initialization failures.
  • Improved asset extraction reliability by detecting incomplete reads/writes and cleaning up temporary files.
  • Added clearer handling for truncated or corrupt metadata files.
• Reliability
  • Standardized runtime assertions and failure handling across native components.
  • Improved detection and reporting of critical runtime failures.