chore: pr feedback

Author: NathanWalker

test-app/app/src/main/assets/app/tests/testNativeESClasses.js

@@ -202,6 +202,31 @@ describe("Tests native ES class extensions (class X extends NativeType)", functi
         expect(runCount).toBe(1);
     });
 
+    it("When_two_es_classes_implement_the_same_interface_js_instances_should_stay_distinct", function () {
+        var aCount = 0;
+        var bCount = 0;
+
+        class EsRunnableA extends java.lang.Runnable {
+            run() {
+                aCount++;
+            }
+        }
+
+        class EsRunnableB extends java.lang.Runnable {
+            run() {
+                bCount++;
+            }
+        }
+
+        new java.lang.Thread(new EsRunnableA()).run();
+        new java.lang.Thread(new EsRunnableB()).run();
+
+        expect(aCount).toBe(1);
+        expect(bCount).toBe(1);
+        // DexFactory shares one interface proxy; JS identity is per instance.
+        expect(EsRunnableA.class.equals(EsRunnableB.class)).toBe(true);
+    });
+
     it("When_declaring_static_interfaces_the_proxy_should_implement_them", function () {
         var ran = { value: false };
 
@@ -348,6 +373,30 @@ describe("Tests native ES class extensions (class X extends NativeType)", functi
         expect(ESPrivateAllocObject.class.newInstance().someMethod()).toBe(1);
     });
 
+    it("When_java_instantiates_an_es_class_nested_native_construction_should_not_steal_adopt", function () {
+        class ESNestedAdoptObject extends java.lang.Object {
+            constructor() {
+                // Valid before super(): must not consume the pending adopt id
+                // that belongs to ESNestedAdoptObject.
+                var list = new java.util.ArrayList();
+                list.add("nested");
+                super();
+                this.list = list;
+            }
+        }
+
+        var allocated = ESNestedAdoptObject.class.newInstance();
+        expect(allocated instanceof ESNestedAdoptObject).toBe(true);
+        expect(allocated.list instanceof java.util.ArrayList).toBe(true);
+        expect(allocated.list.size()).toBe(1);
+        expect(allocated.list.get(0)).toBe("nested");
+        expect(allocated.getClass().getName()).toContain("ESNestedAdoptObject");
+
+        var constructed = new ESNestedAdoptObject();
+        expect(constructed.list.get(0)).toBe("nested");
+        expect(constructed.getClass().equals(allocated.getClass())).toBe(true);
+    });
+
     it("When_java_instantiates_an_es_class_super_args_should_not_construct_again", function () {
         class ESAdoptOnceObject extends com.tns.tests.DummyClass {
             constructor() {
@@ -438,6 +487,17 @@ describe("Tests native ES class extensions (class X extends NativeType)", functi
         expect(new ESEagerNamed() instanceof ESEagerNamed).toBe(true);
     });
 
+    it("When_NativeClass_sets_an_unqualified_android_name_it_should_throw", function () {
+        expect(function () {
+            global.NativeClass({
+                android: {
+                    name: "UnqualifiedName"
+                }
+            })(class UnqualifiedNativeClass extends java.lang.Object {
+            });
+        }).toThrow();
+    });
+
     it("When_NativeClass_runs_on_a_worker_it_should_be_a_noop", function (done) {
         var worker = new Worker("../shared/Workers/EvalWorker.js");
         worker.onmessage = function (msg) {
@@ -462,16 +522,22 @@ describe("Tests native ES class extensions (class X extends NativeType)", functi
     });
 
     it("When_anonymous_es_classes_extend_native_types_each_should_get_a_distinct_proxy", function () {
-        var First = class extends java.lang.Object {
-            toString() {
-                return "first anonymous";
-            }
-        };
-        var Second = class extends java.lang.Object {
-            toString() {
-                return "second anonymous";
+        // Array-literal class expressions stay anonymous (no inferred name),
+        // so both hash as ESClass and exercise the _2 suffix collision path.
+        var classes = [
+            class extends java.lang.Object {
+                toString() {
+                    return "first anonymous";
+                }
+            },
+            class extends java.lang.Object {
+                toString() {
+                    return "second anonymous";
+                }
             }
-        };
+        ];
+        var First = classes[0];
+        var Second = classes[1];
 
         var firstInstance = new First();
         var secondInstance = new Second();

test-app/app/src/main/assets/internal/ts_helpers.js

@@ -177,9 +177,18 @@
 		var name = android && android.name;
 
 		if (interfaces && interfaces.length > 0) {
-			target.interfaces = (target.interfaces && target.interfaces instanceof Array ? target.interfaces.concat(interfaces) : interfaces.slice());
+			var merged = (target.interfaces && target.interfaces instanceof Array ? target.interfaces.concat(interfaces) : interfaces.slice());
+			target.interfaces = merged;
+			// Legacy `.extend()` reads interfaces from the implementation object
+			// (the prototype). Keep both so downleveled ES5 targets still work.
+			if (target.prototype) {
+				target.prototype.interfaces = merged;
+			}
 		}
 		if (name) {
+			if (name.indexOf(".") === -1) {
+				throw new Error("NativeClass android.name must be a fully qualified Java class name.");
+			}
 			target.nativeClassName = name;
 			// Accessing `.class` lazily registers the proxy under the explicit name.
 			void target.class;

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

@@ -95,7 +95,7 @@ bool CallbackHandlers::RegisterInstance(Isolate *isolate, const Local<Object> &j
     auto objectManager = runtime->GetObjectManager();
 
     int adoptObjectId = -1;
-    if (MetadataNode::TryConsumePendingESAdopt(isolate, adoptObjectId)) {
+    if (MetadataNode::TryConsumePendingESAdopt(isolate, fullClassName, adoptObjectId)) {
         // Adopt path: Java already created this object. Bind it to the ES
         // construct and do not NewObject again (that would be a second
         // instance, or recurse through initInstance).

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

@@ -166,7 +166,7 @@ bool JsArgConverter::ConvertArg(const Local<Value> &arg, int index) {
             // JEnv caches classes as global refs - mark as global so the dtor doesn't delete it
             SetConvertedObject(index, clazz, true /* isGlobal */);
         } else {
-            sprintf(buff, "Cannot convert function to %s at index %d", typeSignature.c_str(), index);
+            snprintf(buff, sizeof(buff), "Cannot convert function to %s at index %d", typeSignature.c_str(), index);
         }
     } else if (arg->IsObject()) {
         auto context = m_isolate->GetCurrentContext();

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

@@ -1178,7 +1178,7 @@ MetadataNode::TypeMetadata* MetadataNode::TryGetTypeMetadata(Isolate* isolate, c
         return nullptr;
     }
 
-    return reinterpret_cast<TypeMetadata*>(hiddenVal.As<External>()->Value());
+    return reinterpret_cast<TypeMetadata*>(hiddenVal.As<External>()->Value(v8::kExternalPointerTypeTagDefault));
 }
 
 std::string MetadataNode::TryResolveClassCtorTypeName(Isolate* isolate, const Local<Function>& func) {
@@ -1221,20 +1221,51 @@ std::string SanitizeESClassNamePart(const std::string& name) {
     std::string result;
     result.reserve(name.size());
     for (char c : name) {
-        bool isValid = isalpha(c) || isdigit(c) || c == '_';
+        auto uc = static_cast<unsigned char>(c);
+        bool isValid = isalnum(uc) || c == '_';
         result += isValid ? c : '_';
     }
     return result;
 }
 
+std::string BuildESClassProxyIdentity(const std::string& scriptName, const std::string& baseClassName, const std::string& className,
+                                      const std::vector<std::string>& methodOverrides, const std::vector<std::string>& implementedInterfaces) {
+    auto sortedMethods = methodOverrides;
+    auto sortedInterfaces = implementedInterfaces;
+    std::sort(sortedMethods.begin(), sortedMethods.end());
+    std::sort(sortedInterfaces.begin(), sortedInterfaces.end());
+
+    std::string identity = scriptName + "|" + baseClassName + "|" + className;
+    for (const auto& method : sortedMethods) {
+        identity += "|m:" + method;
+    }
+    for (const auto& iface : sortedInterfaces) {
+        identity += "|i:" + iface;
+    }
+    return identity;
+}
+
+bool TryGetConstructorPrototype(Isolate* isolate, const Local<Function>& ctorFunc, Local<Object>& out) {
+    auto context = isolate->GetCurrentContext();
+    Local<Value> protoValue;
+    if (!ctorFunc->Get(context, V8StringConstants::GetPrototype(isolate)).ToLocal(&protoValue)
+            || protoValue.IsEmpty() || !protoValue->IsObject()) {
+        return false;
+    }
+    out = protoValue.As<Object>();
+    return true;
+}
+
 // True only for genuine `class` syntax constructors. Function source text is the reliable
 // discriminator: per spec, Function.prototype.toString for a class constructor reproduces the
 // `class` declaration/expression source (possibly behind leading comments/whitespace, which V8
 // does not emit for the class case - the text starts with "class").
 bool IsESClassConstructor(v8::Isolate* isolate, const v8::Local<v8::Function>& func) {
+    v8::TryCatch tc(isolate);
     auto context = isolate->GetCurrentContext();
     v8::Local<v8::String> sourceText;
     if (!func->FunctionProtoToString(context).ToLocal(&sourceText)) {
+        tc.Reset();
         return false;
     }
 
@@ -1431,7 +1462,7 @@ MetadataNode::TypeMetadata* MetadataNode::EnsureExtendedESClass(Isolate* isolate
             scriptName = ArgConverter::ConvertToString(resourceName.As<String>());
         }
 
-        string extendNameAndLocation = "es" + HashESClassId(scriptName + "|" + baseClassName + "|" + className) + "_" + className;
+        string extendNameAndLocation = "es" + HashESClassId(BuildESClassProxyIdentity(scriptName, baseClassName, className, methodOverrides, implementedInterfaces)) + "_" + className;
         string candidate = TNS_PREFIX + CreateFullClassName(baseClassName, extendNameAndLocation);
 
         // collision handling for distinct classes that produce the same deterministic name
@@ -1492,17 +1523,26 @@ bool MetadataNode::TryConstructESDerivedInstance(Isolate* isolate, const string&
         return false;
     }
 
+    // DexFactory collapses every interface extension onto one shared
+    // com.tns.gen.<interface> proxy. That name does not identify a single ES
+    // class, so Java-born instances stay on the legacy wrapper path.
+    if (cacheData.node != nullptr && cacheData.node->IsNodeTypeInterface()) {
+        return false;
+    }
+
     Local<Function> ctor = Local<Function>::New(isolate, *cacheData.extendedCtorFunction);
     auto typeMetadata = TryGetTypeMetadata(isolate, ctor);
     if (typeMetadata == nullptr || !typeMetadata->isESDerived) {
         return false;
     }
 
     cache->PendingESAdoptObjectId = javaObjectID;
+    cache->PendingESAdoptClassName = proxyClassName;
     TryCatch tc(isolate);
     auto context = isolate->GetCurrentContext();
     bool ok = !ctor->CallAsConstructor(context, 0, nullptr).IsEmpty();
     cache->PendingESAdoptObjectId = -1;
+    cache->PendingESAdoptClassName.clear();
     if (!ok) {
         throw NativeScriptException(tc, "Failed to construct ES class for native instance");
     }
@@ -1517,14 +1557,18 @@ bool MetadataNode::TryConstructESDerivedInstance(Isolate* isolate, const string&
     return true;
 }
 
-bool MetadataNode::TryConsumePendingESAdopt(Isolate* isolate, int& javaObjectID) {
+bool MetadataNode::TryConsumePendingESAdopt(Isolate* isolate, const string& fullClassName, int& javaObjectID) {
     auto cache = GetMetadataNodeCache(isolate);
     if (cache->PendingESAdoptObjectId == -1) {
         return false;
     }
+    if (cache->PendingESAdoptClassName != fullClassName) {
+        return false;
+    }
 
     javaObjectID = cache->PendingESAdoptObjectId;
     cache->PendingESAdoptObjectId = -1;
+    cache->PendingESAdoptClassName.clear();
     return true;
 }
 
@@ -1618,7 +1662,10 @@ void MetadataNode::InterfaceConstructorCallback(const v8::FunctionCallbackInfo<v
             }
 
             if (typeMetadata != nullptr && typeMetadata->isESDerived) {
-                auto esImplementationObject = newTargetFunc->Get(context, V8StringConstants::GetPrototype(isolate)).ToLocalChecked().As<Object>();
+                Local<Object> esImplementationObject;
+                if (!TryGetConstructorPrototype(isolate, newTargetFunc, esImplementationObject)) {
+                    throw NativeScriptException(string("Cannot resolve the prototype of the ES class constructor."));
+                }
 
                 SetInstanceMetadata(isolate, thiz, node);
                 thiz->SetInternalField(static_cast<int>(ObjectManager::MetadataNodeKeys::CallSuper), True(isolate));
@@ -1705,8 +1752,10 @@ void MetadataNode::ClassConstructorCallback(const v8::FunctionCallbackInfo<v8::V
             }
 
             if (typeMetadata != nullptr && typeMetadata->isESDerived) {
-                auto context = isolate->GetCurrentContext();
-                auto implementationObject = newTargetFunc->Get(context, V8StringConstants::GetPrototype(isolate)).ToLocalChecked().As<Object>();
+                Local<Object> implementationObject;
+                if (!TryGetConstructorPrototype(isolate, newTargetFunc, implementationObject)) {
+                    throw NativeScriptException(string("Cannot resolve the prototype of the ES class constructor."));
+                }
 
                 SetInstanceMetadata(isolate, thiz, node);
                 thiz->SetInternalField(static_cast<int>(ObjectManager::MetadataNodeKeys::CallSuper), True(isolate));

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

@@ -60,7 +60,7 @@ class MetadataNode {
          */
         static bool TryConstructESDerivedInstance(v8::Isolate* isolate, const std::string& proxyClassName, int javaObjectID, v8::Local<v8::Object>& out);
 
-        static bool TryConsumePendingESAdopt(v8::Isolate* isolate, int& javaObjectID);
+        static bool TryConsumePendingESAdopt(v8::Isolate* isolate, const std::string& fullClassName, int& javaObjectID);
 
         static v8::Local<v8::Object> GetImplementationObject(v8::Isolate* isolate, const v8::Local<v8::Object>& object);
 
@@ -340,9 +340,10 @@ class MetadataNode {
 
             // Java object id being adopted by an in-flight ES construct
             // (CreateJSInstanceNative → CallAsConstructor → super()).
-            // RegisterInstance consumes it so super() binds that id and does
-            // not NewObject again.
+            // RegisterInstance consumes it only when fullClassName matches, so
+            // a nested `new OtherNative()` before super() cannot steal the id.
             int PendingESAdoptObjectId = -1;
+            std::string PendingESAdoptClassName;
 
             ~MetadataNodeCache() {
                 delete MetadataKey;