fix: unique JS error handle ids, and diagnose bad metadata node ids

Two review findings on this branch.

The JS error handle id was minted from a per-runtime counter, so every runtime
produced 1, 2, 3... A throwable converted back to JS on a runtime other than the
one that created it would then find an unrelated entry under the same id and
consume it, instead of missing and falling back to rebuilding the error from the
Java throwable. Ids are now unique process-wide, which is what makes the table
lookup itself the ownership check.

GetNodeById's new bounds check turned an out-of-range read into a nullptr its
callers still dereferenced. It now logs the offending id, ReadTypeName and the
array-element lookup in GetNodeType throw a NativeScriptException naming the
problem, and GetBaseClassNode returns null -- which every caller already treats
as "no base class". The assert it relied on was a no-op in release, where the
bounds check was missing entirely.

Also asserts the ownership precondition in StateMutex::Unlock and ReleaseAll: an
unmatched unlock would wrap depth_ and hold the mutex forever, and a non-owner
ReleaseAll would drop another thread's lock mid-section.

Author: edusperoni

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

@@ -1,6 +1,7 @@
 #include "MetadataReader.h"
 #include "MetadataMethodInfo.h"
 #include <android/log.h>
+#include "NativeScriptException.h"
 #include "Util.h"
 #include <sstream>
 
@@ -40,6 +41,8 @@ void MetadataReader::StateMutex::Lock() {
 
 void MetadataReader::StateMutex::Unlock() {
     std::lock_guard<std::mutex> guard(mutex_);
+    // An unmatched unlock would wrap depth_ and hold the mutex forever.
+    assert(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
@@ -50,6 +53,9 @@ void MetadataReader::StateMutex::Unlock() {
 
 unsigned MetadataReader::StateMutex::ReleaseAll() {
     std::lock_guard<std::mutex> 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());
     unsigned held = depth_;
     depth_ = 0;
     owner_ = std::thread::id();
@@ -144,6 +150,9 @@ MetadataTreeNode *MetadataReader::GetNodeById(uint16_t nodeId) {
     StateLock lock(m_stateMutex);
 
     if (nodeId >= m_v.size()) {
+        __android_log_print(ANDROID_LOG_ERROR, "TNS.error",
+                            "Metadata node id %u is out of range (%zu nodes). The metadata is inconsistent.",
+                            nodeId, m_v.size());
         return nullptr;
     }
 
@@ -152,6 +161,12 @@ MetadataTreeNode *MetadataReader::GetNodeById(uint16_t nodeId) {
 
 
 string MetadataReader::ReadTypeName(MetadataTreeNode *treeNode) {
+    // Guards the whole family: the uint16_t overload and ReadTypeNameInternal's
+    // array-element forward both reach here with a GetNodeById result.
+    if (treeNode == nullptr) {
+        throw NativeScriptException("Cannot read a type name: the metadata refers to a node that does not exist.");
+    }
+
     StateLock lock(m_stateMutex);
 
     string name;
@@ -245,6 +260,9 @@ uint8_t MetadataReader::GetNodeType(MetadataTreeNode *treeNode) {
         } else {
             uint16_t nodeId = offsetValue - ARRAY_OFFSET;
             MetadataTreeNode * arrElemNode = GetNodeById(nodeId);
+            if (arrElemNode == nullptr) {
+                throw NativeScriptException("Cannot resolve an array element type: the metadata refers to a node that does not exist.");
+            }
             nodeType = *(m_valueData + arrElemNode->offsetValue);
         }
 
@@ -425,10 +443,8 @@ MetadataTreeNode *MetadataReader::GetBaseClassNode(MetadataTreeNode *treeNode) {
         uint16_t baseClassNodeId = *reinterpret_cast<uint16_t *>(m_valueData +
                                                                  treeNode->offsetValue + 1);
 
-        size_t nodeCount = m_v.size();
-
-        assert(baseClassNodeId < nodeCount);
-
+        // Null for an id the metadata got wrong, which GetNodeById logs; every
+        // caller already treats a missing base class as "no base class".
         baseClassNode = GetNodeById(baseClassNodeId);
     }
 

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

@@ -1,6 +1,7 @@
 #include "NativeScriptException.h"
 
 #include <algorithm>
+#include <atomic>
 #include <sstream>
 
 #include "ArgConverter.h"
@@ -76,14 +77,25 @@ struct JsErrorHandles {
         pruneAt = std::max<size_t>(16, entries.size() * 2);
     }
 
-    int64_t nextId = 1;
     size_t pruneAt = 16;
     robin_hood::unordered_map<int64_t, Entry> entries;
 };
 
 /*
- * Files `error` under a fresh id for this runtime. Returns 0 when the runtime
- * is tearing down, which the Java side reads as "no JS value".
+ * Ids are unique process-wide rather than per runtime. A throwable can be
+ * converted back to JS on a runtime other than the one that created it (it
+ * outlives its runtime, or crosses threads), and a per-runtime counter would
+ * make that lookup hit an unrelated entry with the same id instead of missing.
+ * Being globally unique is what makes `entries.find(id)` the ownership check:
+ * a foreign id is simply absent, and the caller falls back to rebuilding the
+ * error from the Java throwable.
+ */
+std::atomic<int64_t> g_nextJsErrorId{1};
+
+/*
+ * Files `error` in this runtime's table under a process-wide unique id.
+ * Returns 0 when the runtime is tearing down, which the Java side reads as
+ * "no JS value".
  */
 int64_t StoreJsError(Isolate* isolate, Local<Value> error) {
     auto* handles = RuntimeState::For<JsErrorHandles>(isolate);
@@ -94,7 +106,7 @@ int64_t StoreJsError(Isolate* isolate, Local<Value> error) {
     JEnv env;
     handles->PruneIfDue(env);
 
-    int64_t id = handles->nextId++;
+    int64_t id = g_nextJsErrorId.fetch_add(1, std::memory_order_relaxed);
     JsErrorHandles::Entry entry;
     entry.value.Reset(isolate, error);
     handles->entries.emplace(id, std::move(entry));