Prevent Obj-C dealloc accessing Kotlin counterpart

#KT-41811 Fixed.

Also add GC assertion for add ref for reclaimed object.

backend.native/tests/interop/objc/tests/kt41811.h

@@ -0,0 +1,22 @@
+#import <Foundation/NSObject.h>
+
+extern BOOL deallocRetainReleaseDeallocated;
+
+@interface DeallocRetainRelease : NSObject
+@end;
+
+@protocol WeakReference
+@required
+@property (weak) id referent;
+@end;
+
+@interface ObjCWeakReference : NSObject <WeakReference>
+@property (weak) id referent;
+@end;
+
+extern id <WeakReference> weakDeallocLoadWeak;
+extern BOOL deallocLoadWeakDeallocated;
+
+@interface DeallocLoadWeak : NSObject
+-(void)checkWeak;
+@end;

backend.native/tests/interop/objc/tests/kt41811.kt

@@ -0,0 +1,83 @@
+import kotlin.native.ref.WeakReference
+import kotlinx.cinterop.*
+import kotlin.test.*
+import objcTests.*
+
+// Note: these tests rely on GC assertions: without the fix and the assertions it won't actually crash.
+// GC should fire an assertion if it obtains a reference to Kotlin object that is being (or has been) deallocated.
+
+@Test
+fun testKT41811() {
+    // Attempt to make the state predictable:
+    kotlin.native.internal.GC.collect()
+
+    deallocRetainReleaseDeallocated = false
+    assertFalse(deallocRetainReleaseDeallocated)
+
+    createGarbageDeallocRetainRelease()
+
+    // Runs [DeallocRetainRelease dealloc]:
+    kotlin.native.internal.GC.collect()
+
+    assertTrue(deallocRetainReleaseDeallocated)
+
+    // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object:
+    kotlin.native.internal.GC.collect()
+}
+
+private fun createGarbageDeallocRetainRelease() {
+    autoreleasepool {
+        object : DeallocRetainRelease() {}
+    }
+}
+
+@Test
+fun testKT41811LoadWeak() {
+    testKT41811LoadWeak(ObjCWeakReference())
+}
+
+@Test
+fun testKT41811LoadKotlinWeak() {
+    val kotlinWeak = object : NSObject(), WeakReferenceProtocol {
+        lateinit var weakReference: WeakReference<Any>
+
+        override fun referent(): Any? {
+            return weakReference.value
+        }
+
+        override fun setReferent(referent: Any?) {
+            weakReference = WeakReference(referent!!)
+        }
+    }
+
+    testKT41811LoadWeak(kotlinWeak)
+}
+
+private fun testKT41811LoadWeak(weakRef: WeakReferenceProtocol) {
+    // Attempt to make the state predictable:
+    kotlin.native.internal.GC.collect()
+
+    deallocLoadWeakDeallocated = false
+    assertFalse(deallocLoadWeakDeallocated)
+
+    createGarbageDeallocLoadWeak(weakRef)
+
+    // Runs [DeallocLoadWeak dealloc]:
+    kotlin.native.internal.GC.collect()
+
+    assertTrue(deallocLoadWeakDeallocated)
+
+    // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object:
+    kotlin.native.internal.GC.collect()
+
+    weakDeallocLoadWeak = null
+}
+
+private fun createGarbageDeallocLoadWeak(weakRef: WeakReferenceProtocol) {
+    autoreleasepool {
+        val obj = object : DeallocLoadWeak() {}
+        weakDeallocLoadWeak = weakRef.apply { referent = obj }
+        obj.checkWeak()
+        assertSame(obj, weakDeallocLoadWeak!!.referent)
+    }
+}

backend.native/tests/interop/objc/tests/kt41811.m

@@ -0,0 +1,37 @@
+#import "kt41811.h"
+#import "assert.h"
+
+id retainObject = nil;
+BOOL deallocRetainReleaseDeallocated = NO;
+
+@implementation DeallocRetainRelease
+-(void)dealloc {
+    retainObject = self;
+    assert(retainObject == self);
+    retainObject = nil;
+
+    assert(!deallocRetainReleaseDeallocated);
+    deallocRetainReleaseDeallocated = YES;
+}
+@end;
+
+@implementation ObjCWeakReference
+@end;
+
+id <WeakReference> weakDeallocLoadWeak = nil;
+BOOL deallocLoadWeakDeallocated = NO;
+
+@implementation DeallocLoadWeak
+-(void)checkWeak {
+    assert(weakDeallocLoadWeak != nil);
+    assert(weakDeallocLoadWeak.referent == self);
+}
+
+-(void)dealloc {
+    assert(weakDeallocLoadWeak != nil);
+    assert(weakDeallocLoadWeak.referent == nil);
+
+    assert(!deallocLoadWeakDeallocated);
+    deallocLoadWeakDeallocated = YES;
+}
+@end;

backend.native/tests/objcexport/deallocRetain.kt

@@ -0,0 +1,12 @@
+/*
+ * Copyright 2010-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license
+ * that can be found in the LICENSE file.
+ */
+
+package deallocretain
+
+open class DeallocRetainBase
+
+fun garbageCollect() = kotlin.native.internal.GC.collect()
+
+fun createWeakReference(value: Any) = kotlin.native.ref.WeakReference(value)

backend.native/tests/objcexport/deallocRetain.swift

@@ -0,0 +1,67 @@
+/*
+ * Copyright 2010-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license
+ * that can be found in the LICENSE file.
+ */
+
+import Kt
+
+// Note: these tests rely on GC assertions: without the fix and the assertions it won't actually crash.
+// GC should fire an assertion if it obtains a reference to Kotlin object that is being (or has been) deallocated.
+
+private func test1() throws {
+    // Attempt to make the state predictable:
+    DeallocRetainKt.garbageCollect()
+
+    DeallocRetain.deallocated = false
+    try assertFalse(DeallocRetain.deallocated)
+
+    try autoreleasepool {
+        let obj = DeallocRetain()
+        try obj.checkWeak()
+    }
+
+    // Runs DeallocRetain.deinit:
+    DeallocRetainKt.garbageCollect()
+
+    try assertTrue(DeallocRetain.deallocated)
+
+    // Might crash due to double-dispose if the dealloc applied addRef/releaseRef to reclaimed Kotlin object:
+    DeallocRetainKt.garbageCollect()
+}
+
+private class DeallocRetain : DeallocRetainBase {
+    static var deallocated = false
+    static var retainObject: DeallocRetain? = nil
+    static weak var weakObject: DeallocRetain? = nil
+    static var kotlinWeakRef: KotlinWeakReference<AnyObject>? = nil
+
+    override init() {
+        super.init()
+        DeallocRetain.weakObject = self
+        DeallocRetain.kotlinWeakRef = DeallocRetainKt.createWeakReference(value: self)
+    }
+
+    func checkWeak() throws {
+        try assertSame(actual: DeallocRetain.weakObject, expected: self)
+        try assertSame(actual: DeallocRetain.kotlinWeakRef!.value, expected: self)
+    }
+
+    deinit {
+        DeallocRetain.retainObject = self
+        DeallocRetain.retainObject = nil
+
+        try! assertNil(DeallocRetain.weakObject)
+        try! assertNil(DeallocRetain.kotlinWeakRef!.value)
+
+        try! assertFalse(DeallocRetain.deallocated)
+        DeallocRetain.deallocated = true
+    }
+}
+
+class DeallocRetainTests : SimpleTestProvider {
+    override init() {
+        super.init()
+
+        test("Test1", test1)
+    }
+}

backend.native/tests/objcexport/expectedLazy.h

@@ -318,6 +318,19 @@ __attribute__((swift_name("CoroutinesKt")))
 + (void)invoke1Block:(id<KtKotlinSuspendFunction1>)block argument:(id _Nullable)argument completionHandler:(void (^)(id _Nullable, NSError * _Nullable))completionHandler __attribute__((swift_name("invoke1(block:argument:completionHandler:)")));
 @end;
 
+__attribute__((swift_name("DeallocRetainBase")))
+@interface KtDeallocRetainBase : KtBase
+- (instancetype)init __attribute__((swift_name("init()"))) __attribute__((objc_designated_initializer));
++ (instancetype)new __attribute__((availability(swift, unavailable, message="use object initializers instead")));
+@end;
+
+__attribute__((objc_subclassing_restricted))
+__attribute__((swift_name("DeallocRetainKt")))
+@interface KtDeallocRetainKt : KtBase
++ (void)garbageCollect __attribute__((swift_name("garbageCollect()")));
++ (KtKotlinWeakReference<id> *)createWeakReferenceValue:(id)value __attribute__((swift_name("createWeakReference(value:)")));
+@end;
+
 __attribute__((swift_name("FHolder")))
 @interface KtFHolder : KtBase
 - (instancetype)init __attribute__((swift_name("init()"))) __attribute__((objc_designated_initializer));

runtime/src/main/cpp/Memory.cpp

@@ -1578,10 +1578,12 @@ inline void addHeapRef(ContainerHeader* container) {
     case CONTAINER_TAG_STACK:
       break;
     case CONTAINER_TAG_LOCAL:
+      RuntimeAssert(container->refCount() > 0, "add ref for reclaimed object");
       incrementRC</* Atomic = */ false>(container);
       break;
     /* case CONTAINER_TAG_FROZEN: case CONTAINER_TAG_SHARED: */
     default:
+      RuntimeAssert(container->refCount() > 0, "add ref for reclaimed object");
       incrementRC</* Atomic = */ true>(container);
       break;
   }

runtime/src/main/cpp/MemorySharedRefs.cpp

@@ -127,6 +127,8 @@ void BackRefFromAssociatedObject::addRef() {
   static_assert(errorPolicy != ErrorPolicy::kDefaultValue, "Cannot use default return value here");
 
   if (atomicAdd(&refCount, 1) == 1) {
+    if (obj_ == nullptr) return; // E.g. after [detach].
+
     // There are no references to the associated object itself, so Kotlin object is being passed from Kotlin,
     // and it is owned therefore.
     ensureRefAccessible<errorPolicy>(obj_, context_); // TODO: consider removing explicit verification.
@@ -144,6 +146,8 @@ template <ErrorPolicy errorPolicy>
 bool BackRefFromAssociatedObject::tryAddRef() {
   static_assert(errorPolicy != ErrorPolicy::kDefaultValue, "Cannot use default return value here");
 
+  if (obj_ == nullptr) return false; // E.g. after [detach].
+
   // Suboptimal but simple:
   ensureRefAccessible<errorPolicy>(obj_, context_);
 
@@ -165,6 +169,8 @@ template bool BackRefFromAssociatedObject::tryAddRef<ErrorPolicy::kTerminate>();
 void BackRefFromAssociatedObject::releaseRef() {
   ForeignRefContext context = context_;
   if (atomicAdd(&refCount, -1) == 0) {
+    if (obj_ == nullptr) return; // E.g. after [detach].
+
     // Note: by this moment "subsequent" addRef may have already happened and patched context_.
     // So use the value loaded before refCount update:
     DeinitForeignRef(obj_, context);
@@ -173,8 +179,15 @@ void BackRefFromAssociatedObject::releaseRef() {
   }
 }
 
+void BackRefFromAssociatedObject::detach() {
+  RuntimeAssert(atomicGet(&refCount) == 0, "unexpected refCount")
+  obj_ = nullptr; // Handled in addRef/tryAddRef/releaseRef/ref.
+}
+
 template <ErrorPolicy errorPolicy>
 ObjHeader* BackRefFromAssociatedObject::ref() const {
+  RuntimeAssert(obj_ != nullptr, "no valid Kotlin object found");
+
   if (!ensureRefAccessible<errorPolicy>(obj_, context_)) {
     return nullptr;
   }

runtime/src/main/cpp/MemorySharedRefs.hpp

@@ -54,12 +54,14 @@ class BackRefFromAssociatedObject {
 
   void releaseRef();
 
+  void detach();
+
   // Error if called from the wrong worker with non-frozen obj_.
   template <ErrorPolicy errorPolicy>
   ObjHeader* ref() const;
 
  private:
-  ObjHeader* obj_;
+  ObjHeader* obj_; // May be null before [initAndAddRef] or after [detach].
   ForeignRefContext context_;
   volatile int refCount;
 };

runtime/src/main/cpp/ObjCInterop.mm

@@ -118,6 +118,24 @@ void releaseImp(id self, SEL _cmd) {
 }
 
 void releaseAsAssociatedObjectImp(id self, SEL _cmd) {
+  // This function is called by the GC. It made a decision to reclaim Kotlin object, and runs
+  // deallocation hooks at the moment, including deallocation of the "associated object" ([self])
+  // using the [super release] call below.
+
+  // The deallocation involves running [self dealloc] which can contain arbitrary code.
+  // In particular, this code can retain and release [self]. Obj-C and Swift runtimes handle this
+  // gracefully (unless the object gets accessed after the deallocation of course), but Kotlin doesn't.
+  // For example, this happens in https://youtrack.jetbrains.com/issue/KT-41811, provoked by
+  // UIViewController.dealloc (which retains-releases self._view._viewDelegate == self) and UIView.dealloc.
+  // Generally retaining and releasing Kotlin object that is being deallocated would lead to
+  // use-after-dispose and double-dispose problems (with unpredictable consequences) or to an assertion failure.
+  // To workaround this, detach the back ref from the Kotlin object:
+  getBackRef(self)->detach();
+  // So retain/release/etc. on [self] won't affect the Kotlin object, and an attempt to get
+  // the reference to it (e.g. when calling Kotlin method on [self]) would crash.
+  // The latter is generally ok, because by the time superclass dealloc gets launched, subclass state
+  // should already be deinitialized, and Kotlin methods operate on the subclass.
+
   // [super release]
   Class clazz = object_getClass(self);
   struct objc_super s = {self, clazz};

runtime/src/objc/cpp/ObjCExportClasses.mm

@@ -133,6 +133,22 @@ static void injectToRuntime();
 }
 
 -(void)releaseAsAssociatedObject {
+  // This function is called by the GC. It made a decision to reclaim Kotlin object, and runs
+  // deallocation hooks at the moment, including deallocation of the "associated object" ([self])
+  // using the [super release] call below.
+
+  // The deallocation involves running [self dealloc] which can contain arbitrary code.
+  // In particular, this code can retain and release [self]. Obj-C and Swift runtimes handle this
+  // gracefully (unless the object gets accessed after the deallocation of course), but Kotlin doesn't.
+  // Generally retaining and releasing Kotlin object that is being deallocated would lead to
+  // use-after-dispose and double-dispose problems (with unpredictable consequences) or to an assertion failure.
+  // To workaround this, detach the back ref from the Kotlin object:
+  refHolder.detach();
+  // So retain/release/etc. on [self] won't affect the Kotlin object, and an attempt to get
+  // the reference to it (e.g. when calling Kotlin method on [self]) would crash.
+  // The latter is generally ok because can be triggered only by user-defined Swift/Obj-C
+  // subclasses of Kotlin classes.
+
   [super release];
 }