Fix MetalRedrawer being a source of leak (#771)

* Break strong cycle

* Add test proving it

* Remove dead code

* Explain test

* Add remark about two passes

* Add temporary ignore on test

* Use assertTrue instead of assertEquals

* Update test to ignore CI failure on Metal not supported

* Update test

* Refactor to reflect insight about memory model

* Add contract comment.

* Break strong reference from CADisplayLink -> MetalRedrawer

* Bind logical lifetime of MetalRedrawer to caDisplayLink

* Bind logical lifetime of MetalRedrawer to caDisplayLink

* Close skia context on MetalRedrawer dispose.

* Use `check`

* Fix tests on iOS with Metal on GitHub Actions (#783)

* iOS simulator warmup

* comment redundant for now

* sim ID

* iPhone 8

* iPhone 14

* env.IOS_SIM_UUID

* remove redundant

* one line cmd

* one line cmd

* device

* device

* property

* retry 3 times

* retry 3 times

* retry 5 times

* retry 10 times

* testMetalIosX64

* test test fails

* kotlin 1.8.20

* success test

* success test

* temp fail

* success test

* simplify

* simplify

* simplify

* Added iOS Metal tests only on MacOS

* Gradle, check targets before adding iosX64TestWithMetal task

---------
Co-authored-by: dima.avdeev <dima.avdeev@jetbrains.com>

.github/workflows/ci.yml

 # Default Skiko CI
 name: CI
 
+env:
+  IOS_SIM_UUID: '9C58D5E8-9AE8-4595-A262-35CA4871E886' # https://github.com/futureware-tech/simulator-action/wiki/Devices-macos-13
+
 on:
   push:
     branches: [ master ]
@@ -47,7 +50,7 @@ jobs:
           retention-days: 5
 
   ios:
-    runs-on: macos-11
+    runs-on: macos-13
     steps:
       - uses: actions/checkout@v3
         name: 'Check out code'
@@ -59,9 +62,13 @@ jobs:
           java-version: '11'
           cache: 'gradle'
 
-      - shell: bash
-        name: 'Compile and run iOS x64 tests'
-        run: ./gradlew --stacktrace --info -Pskiko.native.enabled=true -Pskiko.test.onci=true :skiko:iosX64Test
+      - name: 'Compile and run iOS x64 tests'
+        uses: nick-fields/retry@v2
+        with:
+          max_attempts: 10
+          timeout_minutes: 60
+          shell: bash
+          command: ./gradlew --stacktrace --info -Pskiko.native.enabled=true -Pskiko.test.onci=true -Pskiko.iosSimulatorUUID="${{ env.IOS_SIM_UUID }}" :skiko:iosX64TestWithMetal
 
       # iosSimulatorArm64Test will build the binary but the tests will be skipped due to X64 host machine
       - shell: bash

skiko/build.gradle.kts

@@ -426,6 +426,28 @@ kotlin {
             }
         }
     }
+
+    val metalTestTargets = listOf("iosX64", "iosSimulatorArm64")
+    metalTestTargets.forEach { target: String ->
+        if (kotlin.targets.names.contains(target)) {
+            val testBinary = kotlin.targets.getByName<KotlinNativeTarget>(target).binaries.getTest("DEBUG")
+            project.tasks.create(target + "TestWithMetal") {
+                dependsOn(testBinary.linkTask)
+                doLast {
+                    val simulatorIdPropertyKey = "skiko.iosSimulatorUUID"
+                    val simulatorId = findProperty(simulatorIdPropertyKey)?.toString()
+                        ?: error("Property '$simulatorIdPropertyKey' not found. Pass it with -P$simulatorIdPropertyKey=...")
+
+                    project.exec { commandLine("xcrun", "simctl", "boot", simulatorId) }
+                    try {
+                        project.exec { commandLine("xcrun", "simctl", "spawn", simulatorId, testBinary.outputFile) }
+                    } finally {
+                        project.exec { commandLine("xcrun", "simctl", "shutdown", simulatorId) }
+                    }
+                }
+            }
+        }
+    }
 }
 
 fun configureCinterop(

skiko/src/iosMain/kotlin/org/jetbrains/skiko/SkikoUIView.kt

@@ -15,6 +15,7 @@ import platform.UIKit.*
 import platform.darwin.NSInteger
 import kotlin.math.max
 import kotlin.math.min
+import kotlin.native.ref.WeakReference
 
 /*
  TODO: remove org.jetbrains.skiko.objc.UIViewExtensionProtocol after Kotlin 1.8.20
@@ -94,9 +95,14 @@ class SkikoUIView : UIView, UIKeyInputProtocol, UITextInputProtocol {
         _pointInside = pointInside
         _skikoUITextInputTrains = skikoUITextInputTrains
 
-        _redrawer = MetalRedrawer(_metalLayer) { surface ->
-            skiaLayer.draw(surface)
-        }
+        val weakSkiaLayer = WeakReference(skiaLayer)
+
+        _redrawer = MetalRedrawer(
+            _metalLayer,
+            drawCallback = { surface ->
+                weakSkiaLayer.get()?.draw(surface)
+            }
+        )
 
         skiaLayer.needRedrawCallback = _redrawer::needRedraw
         skiaLayer.view = this

skiko/src/iosMain/kotlin/org/jetbrains/skiko/redrawer/MetalRedrawer.ios.kt

@@ -8,13 +8,16 @@ import platform.Foundation.NSSelectorFromString
 import platform.QuartzCore.*
 import platform.darwin.*
 import kotlin.math.roundToInt
+import kotlin.native.ref.WeakReference
 
 internal class MetalRedrawer(
     private val metalLayer: CAMetalLayer,
-    private val drawIntoSurfaceCallback: (Surface) -> Unit
-) {
-    private var isDisposed = false
+    private val drawCallback: (Surface) -> Unit,
 
+    // Used for tests, access to NSRunLoop crashes in test environment
+    addDisplayLinkToRunLoop: ((CADisplayLink) -> Unit)? = null,
+    private val disposeCallback: (MetalRedrawer) -> Unit = { }
+) {
     // Workaround for KN compiler bug
     // Type mismatch: inferred type is objcnames.protocols.MTLDeviceProtocol but platform.Metal.MTLDeviceProtocol was expected
     @Suppress("USELESS_CAST")
@@ -26,10 +29,10 @@ internal class MetalRedrawer(
     // Semaphore for preventing command buffers count more than swapchain size to be scheduled/executed at the same time
     private val inflightSemaphore = dispatch_semaphore_create(metalLayer.maximumDrawableCount.toLong())
 
-    var maximumFramesPerSecond: NSInteger
-        get() = caDisplayLink.preferredFramesPerSecond
+    internal var maximumFramesPerSecond: NSInteger
+        get() = caDisplayLink?.preferredFramesPerSecond ?: 0
         set(value) {
-            caDisplayLink.preferredFramesPerSecond = value
+            caDisplayLink?.preferredFramesPerSecond = value
         }
 
     /*
@@ -46,53 +49,58 @@ internal class MetalRedrawer(
             field = value
 
             if (value) {
-                caDisplayLink.setPaused(false)
+                caDisplayLink?.setPaused(false)
             }
         }
 
-    private val frameListener: NSObject = FrameTickListener {
-        if (hasScheduledDrawOnNextVSync) {
-            hasScheduledDrawOnNextVSync = false
-
-            draw()
-        }
-
-        if (!needsProactiveDisplayLink) {
-            caDisplayLink.setPaused(true)
-        }
-    }
-
-    private val caDisplayLink = CADisplayLink.displayLinkWithTarget(
-        target = frameListener,
-        selector = NSSelectorFromString(FrameTickListener::onDisplayLinkTick.name)
+    internal var caDisplayLink: CADisplayLink? = CADisplayLink.displayLinkWithTarget(
+        target = DisplayLinkProxy {
+            this.handleDisplayLinkTick()
+        },
+        selector = NSSelectorFromString(DisplayLinkProxy::handleDisplayLinkTick.name)
     )
 
     init {
+        val caDisplayLink = caDisplayLink ?: throw IllegalStateException("caDisplayLink is null during redrawer init")
         caDisplayLink.setPaused(true)
-        caDisplayLink.addToRunLoop(NSRunLoop.mainRunLoop, NSRunLoop.mainRunLoop.currentMode)
+        if (addDisplayLinkToRunLoop == null) {
+            caDisplayLink.addToRunLoop(NSRunLoop.mainRunLoop, NSRunLoop.mainRunLoop.currentMode)
+        } else {
+            addDisplayLinkToRunLoop.invoke(caDisplayLink)
+        }
     }
 
     internal fun dispose() {
-        if (!isDisposed) {
-            caDisplayLink.invalidate()
-            isDisposed = true
-        }
+        check(caDisplayLink != null, { "MetalRedrawer.dispose() was called more than once" } )
+
+        disposeCallback(this)
+        caDisplayLink?.invalidate()
+        caDisplayLink = null
+
+        context.flush()
+        context.close()
     }
 
     internal fun needRedraw() {
-        check(!isDisposed) { "MetalRedrawer is disposed" }
-
         hasScheduledDrawOnNextVSync = true
 
         // If caDisplayLink is proactive (touches tracking), this does nothing (already unpaused)
-        caDisplayLink.setPaused(false)
+        caDisplayLink?.setPaused(false)
     }
 
-    private fun draw() {
-        if (isDisposed) {
-            return
+    private fun handleDisplayLinkTick() {
+        if (hasScheduledDrawOnNextVSync) {
+            hasScheduledDrawOnNextVSync = false
+
+            draw()
+        }
+
+        if (!needsProactiveDisplayLink) {
+            caDisplayLink?.setPaused(true)
         }
+    }
 
+    private fun draw() {
         autoreleasepool {
             val (width, height) = metalLayer.drawableSize.useContents {
                 width.roundToInt() to height.roundToInt()
@@ -132,7 +140,7 @@ internal class MetalRedrawer(
             }
 
             surface.canvas.clear(Color.WHITE)
-            drawIntoSurfaceCallback(surface)
+            drawCallback(surface)
             surface.flushAndSubmit()
 
             val commandBuffer = queue.commandBuffer()!!
@@ -151,9 +159,11 @@ internal class MetalRedrawer(
     }
 }
 
-private class FrameTickListener(val onFrameTick: () -> Unit) : NSObject() {
+private class DisplayLinkProxy(
+    private val callback: () -> Unit
+) : NSObject() {
     @ObjCAction
-    fun onDisplayLinkTick() {
-        onFrameTick()
+    fun handleDisplayLinkTick() {
+        callback()
     }
 }

skiko/src/iosTest/kotlin/org/jetbrains/skiko/MetalRedrawerTest.kt

+package org.jetbrains.skiko
+
+import org.jetbrains.skia.Surface
+import org.jetbrains.skiko.redrawer.MetalRedrawer
+import platform.QuartzCore.CADisplayLink
+import platform.QuartzCore.CAMetalLayer
+import kotlin.native.internal.createCleaner
+import kotlin.native.ref.WeakReference
+import kotlin.test.Ignore
+import kotlin.test.Test
+import kotlin.test.assertTrue
+
+class MockNSRunLoop {
+    val displayLinks = mutableListOf<CADisplayLink>()
+}
+
+class MetalRedrawerTest {
+    @Suppress("UNUSED", "UNUSED_PARAMETER")
+    private class MetalRedrawerOwner(
+        mockNSRunLoop: MockNSRunLoop
+    ) {
+        private val redrawer: MetalRedrawer
+
+        init {
+            val weakThis = WeakReference(this)
+
+            redrawer = MetalRedrawer(
+                CAMetalLayer(),
+                drawCallback = { surface ->
+                    weakThis.get()?.draw(surface)
+                },
+                addDisplayLinkToRunLoop = {
+                    mockNSRunLoop.displayLinks.add(it)
+                },
+                disposeCallback = {
+                    assertTrue(mockNSRunLoop.displayLinks.isNotEmpty(), "mockNSRunLoop.displayLinks must contain a displayLink")
+                    assertTrue(mockNSRunLoop.displayLinks.remove(it.caDisplayLink))
+                }
+            )
+        }
+
+        @OptIn(ExperimentalStdlibApi::class)
+        private val redrawerCleaner = createCleaner(redrawer) {
+            it.dispose()
+        }
+
+        private fun draw(surface: Surface) = Unit
+    }
+
+    @Suppress("UNUSED_VARIABLE", "UNUSED")
+    private fun createAndForgetMetalRedrawerOwner(mockNSRunLoop: MockNSRunLoop) {
+        val owner = MetalRedrawerOwner(mockNSRunLoop)
+    }
+
+    @Test
+    fun `check metal redrawer is disposed`() {
+        val mockNSRunLoop = MockNSRunLoop()
+
+        createAndForgetMetalRedrawerOwner(mockNSRunLoop)
+
+        // GC can't sweep Objc-Kotlin objects in one pass due to different lifetime models
+        // Two passes do not guarantee it either, this test can break in future
+        kotlin.native.internal.GC.collect()
+        kotlin.native.internal.GC.collect()
+
+        assertTrue(mockNSRunLoop.displayLinks.isEmpty(), "displayLinks must be empty after MetalRedrawer is disposed. This test can be flaky and depends on assumptions about GC implementation.")
+    }
+}
\ No newline at end of file