From 21616f8760a282b0b01b31413bccec492b394736 Mon Sep 17 00:00:00 2001 From: dabrosch Date: Mon, 8 Dec 2025 14:17:00 -0800 Subject: [PATCH 1/4] [#424] Fix: app.rive.runtime.kotlin.core.errors.RiveException: Accessing disposed C++ object RiveArtboardRenderer. [#424] Fix: app.rive.runtime.kotlin.core.errors.RiveException: Accessing disposed C++ object RiveArtboardRenderer. - The crash is caused by calling resizeArtboard which attempts to get the width from the disposed C++ object, so to protect against this we move the call to inside of the frame lock. - This nesting of the file lock inside of the frame lock looks to be correctly supported by the disposeDependencies function first releasing the file lock and then releasing the frame lock. --- .../runtime/kotlin/renderers/RiveArtboardRenderer.kt | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt index aeb744b6a..96d28225e 100644 --- a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt +++ b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt @@ -51,15 +51,18 @@ open class RiveArtboardRenderer( // Be aware of thread safety! @WorkerThread override fun draw() { - if (controller.requireArtboardResize.getAndSet(false)) { - synchronized(controller.file?.lock ?: this) { resizeArtboard() } - } - // Deref and draw under frameLock synchronized(frameLock) { // Early out for deleted renderer or inactive controller. if (!hasCppObject || !controller.isActive) return + // Handle artboard resize inside frameLock to prevent race condition + if (controller.requireArtboardResize.getAndSet(false)) { + synchronized(controller.file?.lock ?: this) { + resizeArtboard() + } + } + controller.activeArtboard?.draw(cppPointer, fit, alignment, scaleFactor = scaleFactor) } } From 3028f5cddaa62cd263a2bb66fe96570a8e55e747 Mon Sep 17 00:00:00 2001 From: dabrosch Date: Tue, 9 Dec 2025 16:13:56 -0800 Subject: [PATCH 2/4] [#424] Cleanup / Optimizations for race condition fix - Switched both Draw and resizeArtBoard to be under both the file lock and the frame lock. - Removed test for resizeArtboard and made it private because we are already testing that the CppObject cannot be changed during draw. - Updated some comments, I am still wondering if we really need the file lock before disposingDependencies, because the objects it is releasing look like they could be mutated even when the file or even frame lock is held. - Tests were not even able to be ran without the Rive.init, so I added that. --- .../kotlin/core/RiveArtboardRendererTest.kt | 179 +++++++----------- .../kotlin/renderers/RiveArtboardRenderer.kt | 33 ++-- 2 files changed, 92 insertions(+), 120 deletions(-) diff --git a/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt b/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt index 2f7ad48cd..1c328fe1a 100644 --- a/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt +++ b/kotlin/src/androidTest/java/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt @@ -3,10 +3,12 @@ package app.rive.runtime.kotlin.core import android.graphics.SurfaceTexture import android.view.Surface import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry import app.rive.runtime.kotlin.SharedSurface import app.rive.runtime.kotlin.controllers.RiveFileController import app.rive.runtime.kotlin.renderers.RiveArtboardRenderer import org.junit.Assert.assertFalse +import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import java.util.concurrent.CountDownLatch @@ -16,7 +18,11 @@ import java.util.concurrent.locks.ReentrantLock @RunWith(AndroidJUnit4::class) class RiveArtboardRendererTest { - private val testUtils = TestUtils() + + @Before + fun setup() { + Rive.init(InstrumentationRegistry.getInstrumentation().targetContext) + } @Test fun createRenderer() { @@ -55,67 +61,93 @@ class RiveArtboardRendererTest { } /** - * Tests for a race where the renderer is deleted while a frame is being drawn, using a - * controller subclass that blocks the getter for activeArtboard. + * Tests that the renderer cannot be deleted while draw() is executing. + * The delete() call should block until file.lock is released when draw() completes. */ @Test fun deleteRendererDuringFrame() { - // Keep this low, as the test times out during non-critical activeArtboard access. - val timeout = 100L - // Latch to block signal we are passed the `hasCppObject` check in the draw() method - val duringActiveArtboardLatch = CountDownLatch(1) - // Latch to block the activeArtboard getter until we have deleted the renderer - val afterDeleteLatch = CountDownLatch(1) - - // The renderer needs a valid artboard to proceed to accessing the cppPointer + val timeout = 1000L + // Latch to signal we are inside the file.lock synchronized block during draw() + val insideDrawLatch = CountDownLatch(1) + // Latch to allow draw() to complete + val releaseDrawLatch = CountDownLatch(1) + + // The renderer needs a valid artboard val dummyArtboard = object : Artboard(unsafeCppPointer = 1L, lock = ReentrantLock()) { - // Override draw to do nothing, avoiding the thread affinity checks in the real draw(). + // Override draw to add blocking and signal we're inside the synchronized block override fun draw( rendererAddress: Long, fit: Fit, alignment: Alignment, scaleFactor: Float ) { + // Signal that we're inside draw (holding file.lock) + insideDrawLatch.countDown() + // Block to hold the file.lock + releaseDrawLatch.await(timeout, TimeUnit.MILLISECONDS) } } - val latchedController = object : RiveFileController() { - // Called by the renderer's `draw()` method *before* accessing the cppPointer but *after* - // checking `hasCppObject` and `isActive`. - override var activeArtboard: Artboard? - get() { - // Signal that we are in the getter - duringActiveArtboardLatch.countDown() - // Wait until we have deleted the renderer - afterDeleteLatch.await(timeout, TimeUnit.MILLISECONDS) - return dummyArtboard - } - set(value) { - super.activeArtboard = value - } - } - - latchedController.isActive = true - latchedController.activeArtboard = dummyArtboard - val renderer = RiveArtboardRenderer(controller = latchedController) + val controller = RiveFileController(activeArtboard = dummyArtboard) + controller.isActive = true + val renderer = RiveArtboardRenderer(controller = controller) renderer.make() - // Start draw() in a background thread, simulating the worker thread. - // It will block in activeArtboard getter. + // Capture any exception thrown in the background threads + val exceptionRef = AtomicReference() + + // Start draw() in a background thread - this will hold file.lock val drawThread = Thread { - renderer.draw() + try { + renderer.draw() + } catch (e: Throwable) { + exceptionRef.set(e) + } } drawThread.start() - // Wait for the getter to be entered - duringActiveArtboardLatch.await(timeout, TimeUnit.MILLISECONDS) + // Wait until we're inside draw() (holding file.lock) + insideDrawLatch.await(timeout, TimeUnit.MILLISECONDS) - // Simulate deletion during draw(), nulling the cppPointer. - renderer.delete() - // Let the getter return and draw() proceed - afterDeleteLatch.countDown() + // Start delete() in a separate thread - it should block trying to acquire file.lock + val deleteThread = Thread { + try { + renderer.delete() + } catch (e: Throwable) { + exceptionRef.set(e) + } + } + deleteThread.start() + + // Give delete a chance to try acquiring the lock + Thread.sleep(200) + // Verify delete is still blocked waiting for file.lock (thread should still be alive) + assert(deleteThread.isAlive) { + "Expected delete to be blocked waiting for file.lock while draw is executing" + } + + // Now allow draw() to complete and release the lock + releaseDrawLatch.countDown() + + // Wait for draw thread to finish (releases file.lock) drawThread.join(timeout) + + // Now delete should be able to complete + deleteThread.join(timeout) + + // Verify delete completed successfully (thread is no longer alive) + assertFalse( + "Expected delete to complete after file.lock was released", + deleteThread.isAlive + ) + + // Verify no exception was thrown + val exception = exceptionRef.get() + assert(exception == null) { + "Expected no exception with proper locking. " + + "Got: ${exception?.javaClass?.simpleName}: ${exception?.message}" + } } /** @@ -257,71 +289,4 @@ class RiveArtboardRendererTest { drawThread.join(timeout) } - /** - * Tests that the renderer can be safely deleted while resizeArtboard() is executing. - * The fix adds a hasCppObject check at the start of resizeArtboard() to prevent - * accessing disposed C++ objects when accessing width/height properties. - */ - @Test - fun deleteRendererDuringResizeArtboard() { - val timeout = 1000L - // Latch to signal we've entered resizeArtboard() - val duringResizeLatch = CountDownLatch(1) - // Latch to block until we have deleted the renderer - val afterDeleteLatch = CountDownLatch(1) - - val controller = RiveFileController() - controller.fit = Fit.LAYOUT // This sets requireArtboardResize to true - controller.isActive = true - - // Create a custom renderer that overrides resizeArtboard() to add blocking, - // simulating the race condition where the renderer is deleted during resize - val latchingRenderer = object : RiveArtboardRenderer(controller = controller) { - override fun resizeArtboard() { - // Signal we've entered resizeArtboard() - duringResizeLatch.countDown() - - // Block until the renderer is deleted - this simulates the race where - // the renderer could be deleted while resizeArtboard() is executing - afterDeleteLatch.await(timeout, TimeUnit.MILLISECONDS) - - // Call the parent's resizeArtboard() which will check hasCppObject - // (the fix) and return early if disposed, preventing a crash - super.resizeArtboard() - } - } - - latchingRenderer.make() - - // Capture any exception thrown in the background thread - val exceptionRef = AtomicReference() - - // Start draw() in a background thread - val drawThread = Thread { - try { - latchingRenderer.draw() - } catch (e: Throwable) { - exceptionRef.set(e) - } - } - drawThread.start() - - // Wait for resizeArtboard() to be entered - duringResizeLatch.await(timeout, TimeUnit.MILLISECONDS) - - // Delete the renderer while resizeArtboard() is blocked - latchingRenderer.delete() - - // Let resizeArtboard() continue - the hasCppObject check should prevent a crash - afterDeleteLatch.countDown() - - drawThread.join(timeout) - - // Verify no exception was thrown - the fix should prevent the crash - val exception = exceptionRef.get() - assert(exception == null) { - "Expected no exception when renderer is deleted during resizeArtboard(). " + - "Got: ${exception?.javaClass?.simpleName}: ${exception?.message}" - } - } } diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt index 96d28225e..25af52549 100644 --- a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt +++ b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt @@ -1,6 +1,5 @@ package app.rive.runtime.kotlin.renderers -import androidx.annotation.VisibleForTesting import androidx.annotation.WorkerThread import app.rive.runtime.kotlin.controllers.RiveFileController import app.rive.runtime.kotlin.core.Fit @@ -32,10 +31,7 @@ open class RiveArtboardRenderer( } @WorkerThread - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - open fun resizeArtboard() { - if (!hasCppObject) return - + private fun resizeArtboard() { if (fit == Fit.LAYOUT) { val newWidth = width / scaleFactor val newHeight = height / scaleFactor @@ -54,16 +50,24 @@ open class RiveArtboardRenderer( // Deref and draw under frameLock synchronized(frameLock) { // Early out for deleted renderer or inactive controller. + // Note: controller.isActive can change at any time, but + // hasCppObject can only change while holding frameLock if (!hasCppObject || !controller.isActive) return - // Handle artboard resize inside frameLock to prevent race condition - if (controller.requireArtboardResize.getAndSet(false)) { - synchronized(controller.file?.lock ?: this) { - resizeArtboard() + // Protect both resize and draw operations with file.lock to prevent race conditions + // with file/artboard changes on the UI thread. This matches the locking strategy + // used in controller.advance() + synchronized(controller.file?.lock ?: this) { + if (controller.requireArtboardResize.getAndSet(false)) { + resizeArtboard() } + controller.activeArtboard?.draw( + cppPointer, + fit, + alignment, + scaleFactor = scaleFactor + ) } - - controller.activeArtboard?.draw(cppPointer, fit, alignment, scaleFactor = scaleFactor) } } @@ -95,7 +99,10 @@ open class RiveArtboardRenderer( } override fun disposeDependencies() { - // Lock to make sure things are disposed in an orderly manner. - synchronized(controller.file?.lock ?: this) { super.disposeDependencies() } + // Lock to ensure dependencies are disposed in an orderly manner and to coordinate + // with any ongoing file/artboard operations that might be using these dependencies. + synchronized(controller.file?.lock ?: this) { + super.disposeDependencies() + } } } From d078c1186cf206fae4c8011132aa4b37e692a1bc Mon Sep 17 00:00:00 2001 From: dabrosch Date: Tue, 9 Dec 2025 17:36:18 -0800 Subject: [PATCH 3/4] [#424] Added another check for isActive at the innermost lock context. --- .../app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt index 25af52549..b68d5c255 100644 --- a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt +++ b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt @@ -58,6 +58,8 @@ open class RiveArtboardRenderer( // with file/artboard changes on the UI thread. This matches the locking strategy // used in controller.advance() synchronized(controller.file?.lock ?: this) { + // No need to check hasCppObject again here. + if (!controller.isActive) return if (controller.requireArtboardResize.getAndSet(false)) { resizeArtboard() } From 106a089901061dcec764075b95f86d5310484289 Mon Sep 17 00:00:00 2001 From: dabrosch Date: Thu, 11 Dec 2025 10:57:02 -0800 Subject: [PATCH 4/4] [#424] Remove override of disposeDependencies since it should not also be synchronized on the file lock. If left as is we would get a deadlock with the above draw function which gets the framelock and then gets the file lock. --- .../rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt | 7 ------- 1 file changed, 7 deletions(-) diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt index b68d5c255..5e0b51322 100644 --- a/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt +++ b/kotlin/src/main/java/app/rive/runtime/kotlin/renderers/RiveArtboardRenderer.kt @@ -100,11 +100,4 @@ open class RiveArtboardRenderer( start() } - override fun disposeDependencies() { - // Lock to ensure dependencies are disposed in an orderly manner and to coordinate - // with any ongoing file/artboard operations that might be using these dependencies. - synchronized(controller.file?.lock ?: this) { - super.disposeDependencies() - } - } }