fix(runtime): Improve error handling in overriding properties and met…

…hods * Add `deepInstanceProperty` and `deepInstanceMethod` getters in `BaseClassMeta` * Refactor `_protocols` field to use `ProtocolMetas` * Check and throw errors when overriding a native method with a JS property or a native property with a JS function
NativeScript · Aug 26, 2019 · 943948d · 943948d
1 parent ac3e77d
commit 943948d
Show file tree

Hide file tree

Showing 8 changed files with 110 additions and 46 deletions.
diff --git a/src/NativeScript/Metadata/Metadata.h b/src/NativeScript/Metadata/Metadata.h
@@ -823,7 +823,40 @@ struct BaseClassMeta : Meta {
         return this->member(identifier, strlen(identifier), type, includeProtocols, /*onlyIfAvailable*/ true, additionalProtocols);
     }
 
+    const MemberMeta* deepMember(const char* identifier, size_t length, MemberType type, bool includeProtocols, bool onlyIfAvailable, const ProtocolMetas& additionalProtocols) const;
+
+    const MemberMeta* deepMember(StringImpl* identifier, MemberType type, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        // Assign to a separate variable to ensure the lifetime of the string returned by utf8
+        auto identifierUtf8 = identifier->utf8();
+        const char* identif = reinterpret_cast<const char*>(identifierUtf8.data());
+        size_t length = (size_t)identifier->length();
+        return this->deepMember(identif, length, type, includeProtocols, /*onlyIfAvailable*/ true, additionalProtocols);
+    }
+
+    const MemberMeta* deepMember(const char* identifier, MemberType type, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        return this->deepMember(identifier, strlen(identifier), type, includeProtocols, /*onlyIfAvailable*/ true, additionalProtocols);
+    }
+
     /// instance methods
+    const MethodMeta* instanceMethod(const char* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto methodMeta = static_cast<const MethodMeta*>(this->member(identifier, MemberType::InstanceMethod, includeProtocols, additionalProtocols));
+        return methodMeta && methodMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? methodMeta : nullptr;
+    }
+
+    const MethodMeta* instanceMethod(StringImpl* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto methodMeta = static_cast<const MethodMeta*>(this->member(identifier, MemberType::InstanceMethod, includeProtocols, additionalProtocols));
+        return methodMeta && methodMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? methodMeta : nullptr;
+    }
+
+    const MethodMeta* deepInstanceMethod(const char* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto methodMeta = static_cast<const MethodMeta*>(this->deepMember(identifier, MemberType::InstanceMethod, includeProtocols, additionalProtocols));
+        return methodMeta && methodMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? methodMeta : nullptr;
+    }
+
+    const MethodMeta* deepInstanceMethod(StringImpl* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto methodMeta = static_cast<const MethodMeta*>(this->deepMember(identifier, MemberType::InstanceMethod, includeProtocols, additionalProtocols));
+        return methodMeta && methodMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? methodMeta : nullptr;
+    }
 
     // Remove all optional methods/properties which are not implemented in the class
     template <typename TMemberMeta>
@@ -861,6 +894,16 @@ struct BaseClassMeta : Meta {
         return propMeta && propMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? propMeta : nullptr;
     }
 
+    const PropertyMeta* deepInstanceProperty(const char* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto propMeta = static_cast<const PropertyMeta*>(this->deepMember(identifier, MemberType::InstanceProperty, includeProtocols, additionalProtocols));
+        return propMeta && propMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? propMeta : nullptr;
+    }
+
+    const PropertyMeta* deepInstanceProperty(StringImpl* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
+        auto propMeta = static_cast<const PropertyMeta*>(this->deepMember(identifier, MemberType::InstanceProperty, includeProtocols, additionalProtocols));
+        return propMeta && propMeta->isAvailableInClasses(klasses, /*isStatic*/ false) ? propMeta : nullptr;
+    }
+
     /// static properties
     const PropertyMeta* staticProperty(const char* identifier, KnownUnknownClassPair klasses, bool includeProtocols, const ProtocolMetas& additionalProtocols) const {
         auto propMeta = static_cast<const PropertyMeta*>(this->member(identifier, MemberType::StaticProperty, includeProtocols, additionalProtocols));

diff --git a/src/NativeScript/Metadata/Metadata.mm b/src/NativeScript/Metadata/Metadata.mm
@@ -216,6 +216,22 @@ static int compareIdentifiers(const char* nullTerminated, const char* notNullTer
     return members.size() > 0 ? *members.begin() : nullptr;
 }
 
+const MemberMeta* BaseClassMeta::deepMember(const char* identifier, size_t length, MemberType type, bool includeProtocols, bool onlyIfAvailable, const ProtocolMetas& additionalProtocols) const {
+    const MemberMeta* memberMeta = nullptr;
+
+    ProtocolMetas emptyProtocols;
+    const ProtocolMetas* protocols = &additionalProtocols;
+    for (auto currentClass = this; currentClass != nullptr; currentClass = (currentClass->type() == MetaType::Interface) ? static_cast<const InterfaceMeta*>(currentClass)->baseMeta() : nullptr) {
+        if ((memberMeta = currentClass->member(identifier, length, type, includeProtocols, onlyIfAvailable, *protocols))) {
+            break;
+        }
+        // Do not recheck protocols when visiting base classes
+        protocols = &emptyProtocols;
+    }
+
+    return memberMeta;
+}
+
 void collectInheritanceChainMembers(const char* identifier, size_t length, MemberType type, bool onlyIfAvailable, const BaseClassMeta* meta, std::function<void(const MemberMeta*)> collectMember) {
 
     const ArrayOfPtrTo<MemberMeta>* members = nullptr;

diff --git a/src/NativeScript/ObjC/Constructor/ObjCConstructorNative.mm b/src/NativeScript/ObjC/Constructor/ObjCConstructorNative.mm
@@ -75,7 +75,7 @@
                                 constructor->_metadata,
                                 MemberType::StaticMethod,
                                 klass,
-                                nullptr);
+                                ProtocolMetas());
     }
 
     return Base::put(cell, execState, propertyName, value, propertySlot);

diff --git a/src/NativeScript/ObjC/Inheritance/ObjCClassBuilder.h b/src/NativeScript/ObjC/Inheritance/ObjCClassBuilder.h
@@ -54,7 +54,7 @@ class ObjCClassBuilder {
 
     JSC::Strong<ObjCConstructorNative> _baseConstructor;
 
-    std::vector<const Metadata::ProtocolMeta*> _protocols;
+    Metadata::ProtocolMetas _protocols;
 };
 } // namespace NativeScript
 

diff --git a/src/NativeScript/ObjC/Inheritance/ObjCClassBuilder.mm b/src/NativeScript/ObjC/Inheritance/ObjCClassBuilder.mm
@@ -238,7 +238,7 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
 
     ObjCProtocolWrapper* protocolWrapperObject = jsCast<ObjCProtocolWrapper*>(protocolWrapper);
 
-    this->_protocols.push_back(protocolWrapperObject->metadata());
+    this->_protocols.append(protocolWrapperObject->metadata());
 
     if (Protocol* aProtocol = protocolWrapperObject->protocol()) {
         Class klass = this->klass();
@@ -288,7 +288,7 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
                             this->_constructor->metadata(),
                             MemberType::InstanceMethod,
                             this->klass(),
-                            &this->_protocols);
+                            this->_protocols);
 }
 
 void ObjCClassBuilder::addInstanceMethod(ExecState* execState, const Identifier& jsName, JSCell* method, JSC::JSValue& typeEncoding) {
@@ -297,32 +297,29 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
 }
 
 void ObjCClassBuilder::addProperty(ExecState* execState, const Identifier& name, const PropertyDescriptor& propertyDescriptor) {
-    if (!propertyDescriptor.isAccessorDescriptor()) {
-        WTFCrash();
-    }
+    RELEASE_ASSERT(propertyDescriptor.isAccessorDescriptor());
 
     WTF::StringImpl* propertyName = name.impl();
-    const InterfaceMeta* currentClass = this->_baseConstructor->metadata();
-    const PropertyMeta* propertyMeta = nullptr;
-    do {
-        // Do not pass a `Class` instance, we need to override the property and must not check whether it's implemented or not
-        propertyMeta = currentClass->instanceProperty(propertyName, KnownUnknownClassPair(), /*includeProtocols*/ true, ProtocolMetas());
-        currentClass = currentClass->baseMeta();
-    } while (!propertyMeta && currentClass);
-
-    if (!propertyMeta && !this->_protocols.empty()) {
-        for (const ProtocolMeta* aProtocol : this->_protocols) {
-            // Do not pass a `Class` instance, we need to override the property and must not check whether it's implemented or not
-            if ((propertyMeta = aProtocol->instanceProperty(propertyName, KnownUnknownClassPair(), /*includeProtocols*/ true, ProtocolMetas()))) {
-                break;
-            }
-        }
-    }
+    const PropertyMeta* propertyMeta = this->_baseConstructor->metadata()->deepInstanceProperty(propertyName, KnownUnknownClassPair(), /*includeProtocols*/ true, this->_protocols);
 
-    if (propertyMeta) {
+    VM& vm = execState->vm();
+    if (!propertyMeta) {
+        JSValue basePrototype = this->_constructor->get(execState, execState->vm().propertyNames->prototype)
+                                    .toObject(execState)
+                                    ->getPrototypeDirect(execState->vm());
+        PropertySlot baseSlot(basePrototype, PropertySlot::InternalMethodType::Get);
+        bool hasBaseSlot = basePrototype.getPropertySlot(execState, name, baseSlot);
+
+        if (hasBaseSlot && !baseSlot.isAccessor()) {
+            auto throwScope = DECLARE_THROW_SCOPE(vm);
+            WTF::String message = WTF::String::format("Cannot override native method \"%s\" with a property, define it as a JS function instead.",
+                                                      propertyName->utf8().data());
+            throwException(execState, throwScope, JSC::createError(execState, message, defaultSourceAppender));
+            return;
+        }
+    } else {
         Class klass = this->klass();
         GlobalObject* globalObject = jsCast<GlobalObject*>(execState->lexicalGlobalObject());
-        VM& vm = globalObject->vm();
         auto scope = DECLARE_THROW_SCOPE(vm);
 
         if (const MethodMeta* getter = propertyMeta->getter()) {
@@ -370,6 +367,9 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
     JSC::VM& vm = execState->vm();
     instanceMethods->methodTable(vm)->getOwnPropertyNames(instanceMethods, execState, prototypeKeys, EnumerationMode());
 
+    JSValue basePrototype = this->_constructor->get(execState, execState->vm().propertyNames->prototype)
+                                .toObject(execState)
+                                ->getPrototypeDirect(execState->vm());
     for (Identifier key : prototypeKeys) {
         PropertySlot propertySlot(instanceMethods, PropertySlot::InternalMethodType::GetOwnProperty);
 
@@ -379,13 +379,32 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
             continue;
         }
 
+        PropertySlot baseSlot(basePrototype, PropertySlot::InternalMethodType::Get);
+        bool hasBaseSlot = basePrototype.getPropertySlot(execState, key, baseSlot);
         if (propertySlot.isAccessor()) {
             PropertyDescriptor propertyDescriptor;
             propertyDescriptor.setAccessorDescriptor(propertySlot.getterSetter(), propertySlot.attributes());
 
             this->addProperty(execState, key, propertyDescriptor);
         } else if (propertySlot.isValue()) {
             JSValue method = propertySlot.getValue(execState, key);
+
+            if (hasBaseSlot) {
+                if (baseSlot.isAccessor()) {
+                    WTF::String message = WTF::String::format("cannot override native property \"%s\", define it as a JS property instead.",
+                                                              key.utf8().data());
+                    throwException(execState, scope, JSC::createError(execState, method, message, defaultSourceAppender));
+                    return;
+                }
+
+                if (!method.isCell()) {
+                    WTF::String message = WTF::String::format("cannot override native method \"%s\".",
+                                                              key.utf8().data());
+                    throwException(execState, scope, JSC::createError(execState, method, message, defaultSourceAppender));
+                    return;
+                }
+            }
+
             if (method.isCell()) {
                 JSValue encodingValue = jsUndefined();
                 /// We check here if we have an exposed method for the current instance method.
@@ -447,7 +466,7 @@ static void addMethodToClass(ExecState* execState, Class klass, JSCell* method,
                             this->_constructor->metadata(),
                             MemberType::StaticMethod,
                             klass,
-                            &this->_protocols);
+                            this->_protocols);
 }
 
 void ObjCClassBuilder::addStaticMethod(ExecState* execState, const Identifier& jsName, JSCell* method, JSC::JSValue& typeEncoding) {

diff --git a/src/NativeScript/ObjC/ObjCMethodCallback.h b/src/NativeScript/ObjC/ObjCMethodCallback.h
@@ -50,7 +50,7 @@ class ObjCMethodCallback : public FFICallback<ObjCMethodCallback> {
 };
 
 void overrideObjcMethodCalls(ExecState* execState, JSObject* object, PropertyName propertyName, JSCell* method, const Metadata::BaseClassMeta* meta, Metadata::MemberType memberType, Class klass,
-                             std::vector<const Metadata::ProtocolMeta*>* protocols);
+                             const Metadata::ProtocolMetas& protocols);
 
 } // namespace NativeScript
 

diff --git a/src/NativeScript/ObjC/ObjCMethodCallback.mm b/src/NativeScript/ObjC/ObjCMethodCallback.mm
@@ -52,22 +52,8 @@ void overrideObjcMethodCall(ExecState* execState, Class klass, JSCell* method, O
 }
 
 void overrideObjcMethodCalls(ExecState* execState, JSObject* object, PropertyName propertyName, JSCell* method, const Metadata::BaseClassMeta* meta, Metadata::MemberType memberType, Class klass,
-                             std::vector<const Metadata::ProtocolMeta*>* protocols) {
-    JSValue methodValue = jsUndefined();
-    PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
-    if (object->getPropertySlot(execState, propertyName, slot)) {
-        if (slot.isAccessor()) {
-            JSC::VM& vm = execState->vm();
-            auto throwScope = DECLARE_THROW_SCOPE(vm);
-            WTF::String message = WTF::String::format("Cannot override native property \"%s\" with a function, define it as a JS property instead.",
-                                                      propertyName.publicName()->utf8().data());
-            throwException(execState, throwScope, JSC::createError(execState, message, defaultSourceAppender));
-            return;
-        }
-
-        methodValue = slot.getValue(execState, propertyName);
-    }
-
+                             const ProtocolMetas& protocols) {
+    JSValue methodValue = object->get(execState, propertyName);
     ObjCMethodWrapper* wrapper = jsDynamicCast<ObjCMethodWrapper*>(execState->vm(), methodValue);
     Strong<ObjCMethodWrapper> strongWrapper;
     if (!wrapper) {
@@ -84,8 +70,8 @@ void overrideObjcMethodCalls(ExecState* execState, JSObject* object, PropertyNam
 
         } while (methodMetas.size() == 0 && currentClass);
 
-        if (methodMetas.size() == 0 && protocols && !protocols->empty()) {
-            for (auto aProtocol : *protocols) {
+        if (methodMetas.size() == 0) {
+            for (auto aProtocol : protocols) {
                 if ((methodMetas = aProtocol->members(propertyName.publicName(), memberType, /*includeProtocols*/ true, ProtocolMetas())).size() > 0) {
                     break;
                 }

diff --git a/src/NativeScript/ObjC/ObjCPrototype.mm b/src/NativeScript/ObjC/ObjCPrototype.mm
@@ -138,7 +138,7 @@ static EncodedJSValue JSC_HOST_CALL getIterator(ExecState* execState) {
                                 prototype->_metadata,
                                 MemberType::InstanceMethod,
                                 klass,
-                                nullptr);
+                                ProtocolMetas());
     }
 
     return Base::put(cell, execState, propertyName, value, propertySlot);