fix(bridge): Remove ObjCWrapperObject from cache after dealloc

mbektchiev · mbektchiev · commit b3713cb55693 · 2019-02-07T10:10:50.000+02:00
When `dealloc` is implemented in JS we were caching a wrapper to the
already deallocated instance. This doesn't do any harm until the same
memory address is reused for another Objective-C instance. When this happens
the new object will be returned to the JS world via the cached wrapper,
which however won't increment its retention count, leading to its premature
deallocation and a crash whenever it's attempted to be used afterwards.
diff --git a/src/NativeScript/ObjC/ObjCMethodCallback.mm b/src/NativeScript/ObjC/ObjCMethodCallback.mm
@@ -13,6 +13,7 @@
 #include "Metadata.h"
 #include "ObjCConstructorNative.h"
 #include "ObjCTypes.h"
+#include "ObjCWrapperObject.h"
 #include "ReferenceTypeInstance.h"
 #include "TypeFactory.h"
 
@@ -151,8 +152,8 @@ static bool checkErrorOutParameter(ExecState* execState, const WTF::Vector<Stron
     ExecState* execState = methodCallback->_globalExecState;
 
     id target = *static_cast<id*>(argValues[0]);
-#ifdef DEBUG_OBJC_INVOCATION
     SEL selector = *static_cast<SEL*>(argValues[1]);
+#ifdef DEBUG_OBJC_INVOCATION
     bool isInstance = !class_isMetaClass(object_getClass(target));
     NSLog(@"< %@[%@ %@]", isInstance ? @"-" : @"+", NSStringFromClass(object_getClass(target)), NSStringFromSelector(selector));
 #endif
@@ -172,6 +173,18 @@ static bool checkErrorOutParameter(ExecState* execState, const WTF::Vector<Stron
     JSValue thisValue = toValue(execState, target);
     methodCallback->callFunction(thisValue, arguments, retValue);
 
+    if (sel_isEqual(selector, @selector(dealloc))) {
+        // When `dealloc` is implemented in JS we were caching a wrapper to the
+        // already deallocated instance. This doesn't do any harm until the same
+        // memory address is reused for another Objective-C instance. When this happens
+        // the new object will be returned to the JS world via the cached wrapper,
+        // which however won't increment its retention count, leading to its premature
+        // deallocation and a crash whenever it's attempted to be used afterwards.
+        if (auto wrapperObject = jsCast<ObjCWrapperObject*>(thisValue)) {
+            wrapperObject->removeFromCache();
+        }
+    }
+
     if (methodCallback->_hasErrorOutParameter) {
         //        size_t methodCallbackLength = jsDynamicCast<JSObject*>(vm, methodCallback->function())->get(execState, vm.propertyNames->length).toUInt32(execState);
         if (methodCallbackLength == methodCallback->parametersCount() - 1) {
diff --git a/src/NativeScript/ObjC/ObjCWrapperObject.h b/src/NativeScript/ObjC/ObjCWrapperObject.h
@@ -35,6 +35,8 @@ class ObjCWrapperObject : public JSC::JSDestructibleObject {
 
     void setWrappedObject(id wrappedObject);
 
+    void removeFromCache();
+
     static WTF::String className(const JSObject* object, JSC::VM&);
 
     ~ObjCWrapperObject();
diff --git a/src/NativeScript/ObjC/ObjCWrapperObject.mm b/src/NativeScript/ObjC/ObjCWrapperObject.mm
@@ -48,6 +48,10 @@
     return Base::putByIndex(cell, execState, propertyName, value, shouldThrow);
 }
 
+void ObjCWrapperObject::removeFromCache() {
+    this->_objectMap->remove(this->_wrappedObject.get());
+}
+
 void ObjCWrapperObject::setWrappedObject(id wrappedObject) {
     if (this->_wrappedObject) {
         this->_objectMap->remove(this->_wrappedObject.get());
