fix: serialize extended-class allocate/register to prevent duplicate class names (#421)

Author: edusperoni

NativeScript/runtime/ClassBuilder.cpp

@@ -1,10 +1,54 @@
 #include "ClassBuilder.h"
 
+#include <mutex>
+
+#include "UnfairLock.h"
+
 namespace tns {
 
+namespace {
+// objc_allocateClassPair only rejects names that are already *registered*, so
+// two threads extending the same class name concurrently can both allocate it
+// and register duplicate same-named classes (objc keeps both and name lookups
+// become ambiguous). Serialize the whole allocate -> register window.
+//
+// This lock is a leaf — the section never acquires a v8::Locker — so it cannot
+// form a cycle with the isolate locks.
+UnfairMutex extendedClassRegistrationMutex;
+
+// Registered objc class names are never reclaimed and the ladder below
+// allocates suffixes densely under the lock, so a name's taken suffixes form a
+// contiguous prefix. Gallop + binary-search with objc_getClass probes to find
+// its end, so heavy same-name reuse (worker tests, HMR re-extends of one
+// class) stays O(log N) per extend with no side state to grow.
+int FirstFreeSuffix(const std::string& initialName) {
+  auto taken = [&initialName](int i) {
+    return objc_getClass((initialName + std::to_string(i)).c_str()) != nil;
+  };
+  int hi = 1;
+  while (taken(hi)) {
+    hi *= 2;
+  }
+  int lo = hi / 2;
+  while (lo + 1 < hi) {
+    int mid = lo + (hi - lo) / 2;
+    if (taken(mid)) {
+      lo = mid;
+    } else {
+      hi = mid;
+    }
+  }
+  return hi;
+}
+
+// Bounds only *consecutive* failures past the probed free position, i.e.
+// allocation failing for reasons other than an ordinary name collision (which
+// would otherwise loop forever holding the mutex).
+constexpr int kMaxConsecutiveAllocFailures = 100;
+}  // namespace
+
 // Moved this method in a separate .cpp file because ARC destroys the class
 // created with objc_allocateClassPair when the control leaves this method scope
-// TODO: revist this. Maybe a lock is needed regardless
 Class ClassBuilder::GetExtendedClass(std::string baseClassName,
                                      std::string staticClassName,
                                      std::string suffix) {
@@ -14,19 +58,23 @@ Class ClassBuilder::GetExtendedClass(std::string baseClassName,
           ? staticClassName
           : baseClassName + suffix + "_" +
                 std::to_string(++ClassBuilder::classNameCounter_);
-  // here we could either call objc_getClass with the name to see if the class
-  // already exists or we can just try allocating it, which will return nil if
-  // the class already exists so we try allocating it every time to avoid race
-  // conditions in case this method is being executed by multiple threads
+  // Allocation failure is the collision signal (objc_getClass beforehand
+  // would race), but that only detects *registered* names — hence the lock
+  // spanning allocate -> register.
+  std::lock_guard<UnfairMutex> lock(extendedClassRegistrationMutex);
   Class clazz = objc_allocateClassPair(baseClass, name.c_str(), 0);
 
   if (clazz == nil) {
-    int i = 1;
     std::string initialName = name;
-    while (clazz == nil) {
-      name = initialName + std::to_string(i++);
+    int next = FirstFreeSuffix(initialName);
+    for (int attempts = 0;
+         clazz == nil && attempts < kMaxConsecutiveAllocFailures; attempts++) {
+      name = initialName + std::to_string(next++);
       clazz = objc_allocateClassPair(baseClass, name.c_str(), 0);
     }
+    if (clazz == nil) {
+      return nil;
+    }
   }
 
   objc_registerClassPair(clazz);

NativeScript/runtime/ClassBuilder.mm

@@ -63,6 +63,7 @@
 
     Class extendedClass = ClassBuilder::GetExtendedClass(baseClassName, staticClassName,
                                                          std::to_string(isolateId) + "_");
+    tns::Assert(extendedClass != nil, isolate);
     class_addProtocol(extendedClass, @protocol(TNSDerivedClass));
     class_addProtocol(object_getClass(extendedClass), @protocol(TNSDerivedClass));
 
@@ -222,6 +223,7 @@
         auto isolateId = cache->getIsolateId();
         __block Class extendedClass = ClassBuilder::GetExtendedClass(
             baseClassName, extendedClassName, std::to_string(isolateId) + "_");
+        tns::Assert(extendedClass != nil, isolate);
         class_addProtocol(extendedClass, @protocol(TNSDerivedClass));
         class_addProtocol(object_getClass(extendedClass), @protocol(TNSDerivedClass));
 

NativeScript/runtime/SpinLock.h

@@ -1,15 +1,19 @@
 #ifndef SpinLock_h
 #define SpinLock_h
 
-
 /**
  WARNING:
  Do NOT use this.
- More ofthen than not a normal mutex is better and this spinlock is really unfair in multi threading
- This is only supposed to be used in places where the function is very fast and the expected concurrency is very low
- If any of those things are false, this WILL be slower and worse than a mutex or a read write mutex
- 
- The only place this is currently used is for caching selectors, which take ns to run and are not locked to a specific isolate.
+ More ofthen than not a normal mutex is better and this spinlock is really
+ unfair in multi threading This is only supposed to be used in places where the
+ function is very fast and the expected concurrency is very low If any of those
+ things are false, this WILL be slower and worse than a mutex or a read write
+ mutex
+
+ The only place this is currently used is for caching selectors, which take ns
+ to run and are not locked to a specific isolate.
+
+ For sections that can block or see bursty contention, use UnfairLock.h instead.
  */
 
 struct SpinMutex {
@@ -43,14 +47,9 @@ struct SpinMutex {
 };
 
 struct SpinLock {
-    SpinMutex& _mutex;
-    SpinLock(SpinMutex& m) : _mutex(m) {
-        _mutex.lock();
-    }
-    ~SpinLock() {
-        _mutex.unlock();
-    }
+  SpinMutex& _mutex;
+  SpinLock(SpinMutex& m) : _mutex(m) { _mutex.lock(); }
+  ~SpinLock() { _mutex.unlock(); }
 };
 
-
 #endif /* SpinLock_h */

NativeScript/runtime/UnfairLock.h

@@ -0,0 +1,60 @@
+#ifndef UnfairLock_h
+#define UnfairLock_h
+
+#include <os/lock.h>
+
+/**
+ BasicLockable wrapper over os_unfair_lock — the fastest blocking lock on
+ Darwin. Drive it with std::lock_guard<UnfairMutex>.
+
+ Prefer this over SpinLock for any critical section that can block (syscalls,
+ objc runtime calls, allocation) or that can see bursty contention: waiters
+ sleep in the kernel and donate their priority to the holder, while SpinLock
+ busy-waits in userspace, invisible to the scheduler. Benchmarked on an
+ M-series host: under an 8-thread burst on a short section os_unfair_lock is
+ ~30x faster than SpinLock at ~33x less CPU, and against a holder that sleeps
+ mid-section waiters cost ~100x less CPU. SpinLock stays marginally faster
+ (~0.2 ns/op) only for uncontended nanosecond-scale sections — its documented
+ niche (selector caching).
+
+ NATIVESCRIPT_UNFAIR_LOCK_ADAPTIVE_SPIN opts lock() into
+ os_unfair_lock_lock_with_options() with kernel-informed adaptive spinning —
+ the approach Firefox adopted for its nanosecond-hot allocator locks:
+ https://hacks.mozilla.org/2022/10/improving-firefox-responsiveness-on-macos/
+
+ Experiments only, never ship it:
+ - PRIVATE API (os/lock_private.h); App Review flags the symbol.
+ - It only wins for nanosecond-scale sections at low contention with the
+   holder on-core (measured ~2x over plain os_unfair_lock at 2 threads).
+   Under bursty contention it degenerates to spinlock behavior (~30x slower,
+   ~30x more CPU at 8 threads), it is the worst option under QoS inversion,
+   and it changes nothing when the holder sleeps.
+ */
+
+#ifdef NATIVESCRIPT_UNFAIR_LOCK_ADAPTIVE_SPIN
+extern "C" void os_unfair_lock_lock_with_options(os_unfair_lock_t lock,
+                                                 uint32_t options);
+// Values from os/lock_private.h (ADAPTIVE_SPIN requires iOS 13+).
+#define NATIVESCRIPT_OS_UNFAIR_LOCK_DATA_SYNCHRONIZATION 0x00010000u
+#define NATIVESCRIPT_OS_UNFAIR_LOCK_ADAPTIVE_SPIN 0x00040000u
+#endif
+
+struct UnfairMutex {
+  os_unfair_lock lock_ = OS_UNFAIR_LOCK_INIT;
+
+  inline void lock() noexcept {
+#ifdef NATIVESCRIPT_UNFAIR_LOCK_ADAPTIVE_SPIN
+    os_unfair_lock_lock_with_options(
+        &lock_, NATIVESCRIPT_OS_UNFAIR_LOCK_DATA_SYNCHRONIZATION |
+                    NATIVESCRIPT_OS_UNFAIR_LOCK_ADAPTIVE_SPIN);
+#else
+    os_unfair_lock_lock(&lock_);
+#endif
+  }
+
+  inline bool try_lock() noexcept { return os_unfair_lock_trylock(&lock_); }
+
+  inline void unlock() noexcept { os_unfair_lock_unlock(&lock_); }
+};
+
+#endif /* UnfairLock_h */