From 21616f8760a282b0b01b31413bccec492b394736 Mon Sep 17 00:00:00 2001 From: dabrosch Date: Mon, 8 Dec 2025 14:17:00 -0800 Subject: [PATCH 1/5] [#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/5] [#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/5] [#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/5] [#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() - } - } } From ac4eeb2cc9940b1e1a92b5faab0402fdd41f9bef Mon Sep 17 00:00:00 2001 From: dabrosch Date: Sun, 21 Jun 2026 23:41:45 +0200 Subject: [PATCH 5/5] fix(Android): Rewrite #424 renderer lock tests and mark isActive volatile - The prior dispose-during-draw tests used RiveFileController without a File, so draw() and activeArtboard synchronized on different fallback locks and would pass even when fileLock was removed from draw(). - Rewrote the suite against a real empty .riv file with shared fixtures, added a #424 resize/delete regression test, and deduped the file-lock mutation scenarios. - Marked controller.isActive @Volatile so draw()'s cross-thread early-out checks are reliable. --- .../kotlin/core/RiveArtboardRendererTest.kt | 471 ++++++++++++------ .../kotlin/controllers/RiveFileController.kt | 2 + .../kotlin/renderers/RiveArtboardRenderer.kt | 17 +- 3 files changed, 325 insertions(+), 165 deletions(-) diff --git a/kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt b/kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt index fb931e05d..7431a48f1 100644 --- a/kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt +++ b/kotlin/src/androidTest/kotlin/app/rive/runtime/kotlin/core/RiveArtboardRendererTest.kt @@ -4,6 +4,7 @@ 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.test.R import app.rive.runtime.kotlin.SharedSurface import app.rive.runtime.kotlin.controllers.RiveFileController import app.rive.runtime.kotlin.renderers.RiveArtboardRenderer @@ -20,9 +21,15 @@ import java.util.concurrent.locks.ReentrantLock @RunWith(AndroidJUnit4::class) class RiveArtboardRendererTest { + companion object { + private const val TIMEOUT_MS = 1000L + private const val POLL_INTERVAL_MS = 10L + } + @Before fun setup() { - Rive.init(InstrumentationRegistry.getInstrumentation().targetContext) + // Ensure native libraries are loaded for JNI calls. + TestUtils().context } @Test @@ -62,31 +69,39 @@ class RiveArtboardRendererTest { } /** - * Tests that the renderer cannot be deleted while draw() is executing. - * The delete() call should block until frameLock is released when draw() completes. + * Verifies delete() cannot run concurrently with draw(). + * + * Blocks inside [Artboard.draw] under [Renderer.frameLock], starts [RiveArtboardRenderer.delete] + * on another thread, and asserts delete blocks until draw releases the lock. + * + * Intentionally uses [RiveFileController] **without** a [File]: this test targets + * [Renderer.frameLock] only, not [File.fileLock]. */ @Test fun deleteRendererDuringFrame() { - val timeout = 1000L - // Latch to signal we are inside the fileLock synchronized block during draw() + // Latch: draw has entered the blocking artboard override (draw() holds frameLock). val insideDrawLatch = CountDownLatch(1) - // Latch to allow draw() to complete + // Latch: test releases this to let draw() finish and exit frameLock. val releaseDrawLatch = CountDownLatch(1) - // The renderer needs a valid artboard + // The renderer needs a valid artboard to proceed with accessing the cppPointer val dummyArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = ReentrantLock()) { - // Override draw to add blocking and signal we're inside the synchronized block + // Override draw to add blocking and signal we're inside the synchronized block; + // this avoids the thread affinity checks in the real draw() override fun draw( rendererAddress: Long, fit: Fit, alignment: Alignment, scaleFactor: Float ) { - // Signal that we're inside draw (holding fileLock) + // Signal that draw reached Artboard.draw (frameLock still held by draw()). insideDrawLatch.countDown() - // Block to hold the fileLock - releaseDrawLatch.await(timeout, TimeUnit.MILLISECONDS) + // Hold frameLock until the test releases releaseDrawLatch. + releaseDrawLatch.awaitOrFail("Timed out waiting for test to release draw latch") } + + // Prevent any delete on fake pointer (1L). + override fun cppDelete(pointer: Long) {} } val controller = RiveFileController(activeArtboard = dummyArtboard) @@ -97,7 +112,7 @@ class RiveArtboardRendererTest { // Capture any exception thrown in the background threads val exceptionRef = AtomicReference() - // Start draw() in a background thread - this will hold frameLock and fileLock + // Start draw() in a background thread - this will hold frameLock for the duration of draw() val drawThread = Thread { try { renderer.draw() @@ -107,12 +122,15 @@ class RiveArtboardRendererTest { } drawThread.start() - // Wait until we're inside draw() (holding fileLock) - insideDrawLatch.await(timeout, TimeUnit.MILLISECONDS) + insideDrawLatch.awaitOrFail("draw() did not reach Artboard.draw in time") + // Counted down at the start of delete() so waitUntil knows the thread entered delete + // (and is blocked on frameLock, not still starting up). + val deleteInvoked = CountDownLatch(1) // Start delete() in a separate thread - it should block trying to acquire frameLock val deleteThread = Thread { try { + deleteInvoked.countDown() renderer.delete() } catch (e: Throwable) { exceptionRef.set(e) @@ -120,22 +138,20 @@ class RiveArtboardRendererTest { } deleteThread.start() - // Give delete a chance to try acquiring the lock - Thread.sleep(200) - - // Verify delete is still blocked waiting for frameLock (thread should still be alive) - assert(deleteThread.isAlive) { - "Expected delete to be blocked waiting for frameLock while draw is executing" + waitUntil( + message = "Expected delete to be blocked waiting for frameLock while draw is executing" + ) { + deleteInvoked.count == 0L && deleteThread.state == Thread.State.BLOCKED } // Now allow draw() to complete and release the lock releaseDrawLatch.countDown() - // Wait for draw thread to finish (releases frameLock and fileLock) - drawThread.join(timeout) + // Wait for draw thread to finish (releases frameLock) + drawThread.join(TIMEOUT_MS) // Now delete should be able to complete - deleteThread.join(timeout) + deleteThread.join(TIMEOUT_MS) // Verify delete completed successfully (thread is no longer alive) assertFalse( @@ -152,153 +168,50 @@ class RiveArtboardRendererTest { } /** - * Tests for a race where an artboard is disposed while being drawn, specifically after it has - * entered the synchronized block in draw() but before dereferencing the cppPointer. + * Verifies draw() holds [File.lock] so UI-thread artboard mutations block until the frame + * completes. + * + * Uses [loadEmptyFile] so [RiveFileController.file] is set — see that helper for why a real + * [File] is required (without it, draw and [RiveFileController.activeArtboard] synchronize on + * different fallback objects and file-lock regressions do not fail). */ @Test - fun disposeArtboardDuringFrameAfterEnteringSyncBlock() { - // Keep this low, as we expect it to timeout. - val timeout = 1000L - // Signals that the artboard has entered draw() and is about to dereference the cppPointer. - val readyForRelease = CountDownLatch(1) - // Signals that the main thread has released the artboard, and draw() can continue. - val afterRelease = CountDownLatch(1) - - // A lock we can reference, unlike the default private Artboard file lock. - val artboardLock = ReentrantLock() - - // An artboard that synchronizes with the main thread to simulate being released while - // being drawn. - val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = artboardLock) { - override fun draw( - rendererAddress: Long, - fit: Fit, - alignment: Alignment, - scaleFactor: Float - ) { - synchronized(artboardLock) { - readyForRelease.countDown() - // Wait for the test to dispose the artboard. - // We expect this to timeout as the artboard's locking prevents the main thread - // from disposing, so afterRelease cannot be counted down. - afterRelease.await(timeout, TimeUnit.MILLISECONDS) - // Simulate the critical part of draw(): dereference the artboard pointer. - cppPointer - } - } - - // Need to override to avoid calling the real native delete on address 1L - override fun cppDelete(pointer: Long) {} - } - - val controller = RiveFileController(activeArtboard = latchingArtboard) - controller.isActive = true - - val renderer = RiveArtboardRenderer(controller = controller) - renderer.make() - - // Simulate the worker thread calling draw() - val drawThread = Thread { - // Calls the controller's active artboard to draw - renderer.draw() - } - drawThread.start() - - // Wait until renderer.draw() has entered Artboard.draw(). - readyForRelease.await(timeout, TimeUnit.MILLISECONDS) - - // Dispose the artboard by releasing the one place that references it. - controller.activeArtboard = null - assertFalse( - "Expected latchingArtboard to have been disposed", - latchingArtboard.hasCppObject - ) - - // Let draw() continue - afterRelease.countDown() + fun drawHoldsFileLockBlocksActiveArtboardMutation() { + assertActiveArtboardMutationBlocksOnFileLockDuringDraw() + } - drawThread.join(timeout) + /** + * While draw() is inside [Artboard.draw], [RiveFileController.activeArtboard] mutations must + * block on [File.fileLock] until the frame completes. + * + * Backed by [loadEmptyFile] so draw and the setter share one lock — see [loadEmptyFile]. + */ + @Test + fun disposeArtboardDuringFrameAfterEnteringSyncBlock() { + assertActiveArtboardMutationBlocksOnFileLockDuringDraw(assertArtboardDisposal = true) } /** - * Tests for a race where an artboard is disposed while being drawn, specifically before it has - * entered the synchronized block in draw(). + * Same [File.fileLock] requirement as [disposeArtboardDuringFrameAfterEnteringSyncBlock], + * with the latching override resuming into the real [Artboard.draw] after the latch. */ @Test fun disposeArtboardDuringFrameBeforeEnteringSyncBlock() { - val timeout = 1000L - // Signals that the artboard has entered draw() and is about to dereference the cppPointer. - val readyForRelease = CountDownLatch(1) - // Signals that the main thread has released the artboard, and draw() can continue. - val afterRelease = CountDownLatch(1) - - // A lock we can reference, unlike the default private Artboard file lock. - val artboardLock = ReentrantLock() - - // An artboard that synchronizes with the main thread to simulate being released right - // before being drawn. - val latchingArtboard = object : Artboard(unsafeCppPointer = 1L, fileLock = artboardLock) { - override fun draw( - rendererAddress: Long, - fit: Fit, - alignment: Alignment, - scaleFactor: Float - ) { - readyForRelease.countDown() - afterRelease.await() - super.draw(rendererAddress, fit, alignment, scaleFactor) - } - - // Do nothing, avoiding the thread affinity checks in the real draw(). - override fun cppDrawAligned( - cppPointer: Long, rendererPointer: Long, - fit: Fit, alignment: Alignment, - scaleFactor: Float - ) { - } - - // Need to override to avoid calling the real native delete on address 1L - override fun cppDelete(pointer: Long) {} - } - - val controller = RiveFileController(activeArtboard = latchingArtboard) - controller.isActive = true - - val renderer = RiveArtboardRenderer(controller = controller) - renderer.make() - - // Simulate the worker thread calling draw() - val drawThread = Thread { - // Calls the controller's active artboard to draw - renderer.draw() - } - drawThread.start() - - // Wait until renderer.draw() has entered Artboard.draw(). - readyForRelease.await(timeout, TimeUnit.MILLISECONDS) - - // Dispose the artboard by releasing the one place that references it. - controller.activeArtboard = null - assertFalse( - "Expected latchingArtboard to have been disposed", - latchingArtboard.hasCppObject + assertActiveArtboardMutationBlocksOnFileLockDuringDraw( + callSuperDrawAfterLatch = true, + assertArtboardDisposal = true, ) - - // Let draw() continue - afterRelease.countDown() - - drawThread.join(timeout) } /** - * Regression test for a race where resizeArtboard() reads renderer dimensions after the renderer - * has been deleted. + * Regression test for #424: resize reads renderer width/height while draw() holds frameLock. * - * The fit getter is used as a deterministic pause point during resize inside draw(). + * Pauses inside [RiveFileController.fit] (read during resize) so another thread can call + * [RiveArtboardRenderer.delete]. Delete blocks on frameLock; resize must not touch a disposed + * renderer pointer when the pause ends. */ @Test fun deleteRendererBetweenCppObjectCheckAndDimensionRead_doesNotCrash() { - val timeoutMs = 1000L val readyForDelete = CountDownLatch(1) val resumeAfterDelete = CountDownLatch(1) val exceptionRef = AtomicReference(null) @@ -307,7 +220,8 @@ class RiveArtboardRendererTest { override var fit: Fit get() { readyForDelete.countDown() - resumeAfterDelete.await(timeoutMs, TimeUnit.MILLISECONDS) + // Deterministic pause during resizeArtboard() (fit is read for layout sizing). + resumeAfterDelete.awaitOrFail("Timed out waiting to resume draw after delete") return super.fit } set(value) { @@ -315,6 +229,7 @@ class RiveArtboardRendererTest { } } controller.fit = Fit.LAYOUT // Sets requireArtboardResize = true + controller.isActive = true val renderer = RiveArtboardRenderer(controller = controller) renderer.make() @@ -328,15 +243,26 @@ class RiveArtboardRendererTest { } drawThread.start() - assertTrue( - "draw() did not reach the fit getter in time; race was not induced.", - readyForDelete.await(timeoutMs, TimeUnit.MILLISECONDS) + readyForDelete.awaitOrFail( + "draw() did not reach the fit getter in time; race was not induced." ) - renderer.delete() + // delete() blocks on frameLock while draw is paused in the fit getter (still inside draw()). + // Start delete on a background thread, then countDown resume so resize can finish while + // delete waits — same pattern as deleteRendererDuringFrame. + val deleteThread = Thread { + try { + renderer.delete() + } catch (e: Throwable) { + exceptionRef.set(e) + } + } + deleteThread.start() + resumeAfterDelete.countDown() - drawThread.join(timeoutMs) + drawThread.join(TIMEOUT_MS) + deleteThread.join(TIMEOUT_MS) val exception = exceptionRef.get() assertTrue( @@ -345,4 +271,231 @@ class RiveArtboardRendererTest { exception == null ) } + + /** + * Loads [R.raw.empty] for tests that assert [File.fileLock] behaviour. + * + * [RiveArtboardRenderer.draw] and [RiveFileController.activeArtboard] both use + * `synchronized(file?.fileLock ?: this)`. When [RiveFileController.file] is null those fallbacks + * are **different** objects (the renderer instance vs the controller instance), so the old + * dispose tests could pass even with `fileLock` removed from `draw()`. Wiring a real [File] + * forces both code paths onto [File.fileLock], matching production [RiveAnimationView] usage. + */ + private fun loadEmptyFile(): File { + val bytes = InstrumentationRegistry.getInstrumentation().targetContext.resources + .openRawResource(R.raw.empty) + .readBytes() + return File(bytes) + } + + /** + * Controller, renderer, and latching artboard for [File.fileLock] tests — see [loadEmptyFile]. + * + * [RiveArtboardRenderer.draw] blocks in [Artboard.draw] until [releaseDrawLatch] is counted + * down, while holding the same [File.lock] used by [RiveFileController.activeArtboard] + * mutations. + */ + private data class FileLockDrawFixture( + val latchingArtboard: Artboard, + val controller: RiveFileController, + val renderer: RiveArtboardRenderer, + ) + + /** + * Builds a [FileLockDrawFixture]: real [File] from [R.raw.empty], artboard whose [Artboard.draw] + * pauses on [releaseDrawLatch], and an active controller/renderer pair. + * + * @param timeout Passed to [awaitOrFail] while [Artboard.draw] is blocked; defaults to + * [TIMEOUT_MS]. + * @param insideDrawLatch Counted down when [Artboard.draw] is entered. + * @param releaseDrawLatch Unblocks [Artboard.draw] when the test is ready to finish the frame. + * @param callSuperDrawAfterLatch When true, resumes into [Artboard.draw] after the latch and + * stubs [Artboard.cppDrawAligned] to avoid native draw / thread-affinity checks. + */ + private fun fileLockDrawFixture( + insideDrawLatch: CountDownLatch, + releaseDrawLatch: CountDownLatch, + timeout: Long = TIMEOUT_MS, + callSuperDrawAfterLatch: Boolean = false, + ): FileLockDrawFixture { + val file = loadEmptyFile() + val latchingArtboard = if (callSuperDrawAfterLatch) { + object : Artboard(unsafeCppPointer = 1L, fileLock = file.lock) { + override fun draw( + rendererAddress: Long, + fit: Fit, + alignment: Alignment, + scaleFactor: Float + ) { + insideDrawLatch.countDown() + releaseDrawLatch.awaitOrFail( + "Timed out waiting for test to release draw latch", + timeoutMs = timeout, + ) + super.draw(rendererAddress, fit, alignment, scaleFactor) + } + + // Do nothing, avoiding the thread affinity checks in the real draw(). + override fun cppDrawAligned( + cppPointer: Long, rendererPointer: Long, + fit: Fit, alignment: Alignment, + scaleFactor: Float + ) { + } + + override fun cppDelete(pointer: Long) {} + } + } else { + object : Artboard(unsafeCppPointer = 1L, fileLock = file.lock) { + override fun draw( + rendererAddress: Long, + fit: Fit, + alignment: Alignment, + scaleFactor: Float + ) { + insideDrawLatch.countDown() + releaseDrawLatch.awaitOrFail( + "Timed out waiting for test to release draw latch", + timeoutMs = timeout, + ) + } + + override fun cppDelete(pointer: Long) {} + } + } + + val controller = RiveFileController(file = file, activeArtboard = latchingArtboard) + controller.isActive = true + val renderer = RiveArtboardRenderer(controller = controller) + renderer.make() + return FileLockDrawFixture(latchingArtboard, controller, renderer) + } + + /** + * Runs draw() on a background thread while blocked in [Artboard.draw], starts an + * [RiveFileController.activeArtboard] = null mutation, and asserts the mutation blocks on + * [File.fileLock] until draw finishes. + * + * @param callSuperDrawAfterLatch Passed to [fileLockDrawFixture]; when true, the latching + * artboard resumes into [Artboard.draw] after the latch. + * @param assertArtboardDisposal When true, asserts the latching artboard is not disposed while + * blocked and is disposed after the frame completes. + */ + private fun assertActiveArtboardMutationBlocksOnFileLockDuringDraw( + callSuperDrawAfterLatch: Boolean = false, + assertArtboardDisposal: Boolean = false, + ) { + val insideDrawLatch = CountDownLatch(1) + val releaseDrawLatch = CountDownLatch(1) + val mutationComplete = CountDownLatch(1) + val exceptionRef = AtomicReference() + + val fixture = fileLockDrawFixture( + insideDrawLatch = insideDrawLatch, + releaseDrawLatch = releaseDrawLatch, + callSuperDrawAfterLatch = callSuperDrawAfterLatch, + ) + + val drawThread = Thread { + try { + fixture.renderer.draw() + } catch (e: Throwable) { + exceptionRef.set(e) + } + } + drawThread.start() + + insideDrawLatch.awaitOrFail("draw() did not reach Artboard.draw in time") + + val mutationThread = Thread { + try { + fixture.controller.activeArtboard = null + mutationComplete.countDown() + } catch (e: Throwable) { + exceptionRef.set(e) + } + } + mutationThread.start() + + waitUntil( + message = "Expected activeArtboard mutation to block while draw holds fileLock" + ) { + mutationThread.state == Thread.State.BLOCKED + } + + if (assertArtboardDisposal) { + assertTrue( + "Artboard should not be disposed while draw holds fileLock", + fixture.latchingArtboard.hasCppObject + ) + } + + releaseDrawLatch.countDown() + + drawThread.join(TIMEOUT_MS) + mutationThread.join(TIMEOUT_MS) + + mutationComplete.awaitOrFail( + "activeArtboard mutation should complete after draw finishes" + ) + + if (assertArtboardDisposal) { + assertFalse( + "Expected latchingArtboard to have been disposed after draw completed", + fixture.latchingArtboard.hasCppObject + ) + } + + val exception = exceptionRef.get() + assert(exception == null) { + "Expected no exception with proper fileLock. " + + "Got: ${exception?.javaClass?.simpleName}: ${exception?.message}" + } + + fixture.controller.isActive = false + fixture.controller.release() + } + + /** + * Polls until [condition] is true or [timeoutMs] elapses. + * + * Prefer this over fixed [Thread.sleep] when waiting for concurrent threads to reach an + * expected state: fast on healthy runners, tolerant of slow CI emulators. Lock tests should + * pass a [condition] that detects blocking (e.g. [Thread.State.BLOCKED]), not merely + * [Thread.isAlive]. + * + * @param timeoutMs Maximum time to poll before throwing [AssertionError] with [message]. + * @param message Failure message when [condition] never becomes true. + * @param condition Expected concurrent state; polled every [POLL_INTERVAL_MS]. + */ + private fun waitUntil( + timeoutMs: Long = TIMEOUT_MS, + message: String, + condition: () -> Boolean, + ) { + val deadline = System.currentTimeMillis() + timeoutMs + while (!condition()) { + if (System.currentTimeMillis() >= deadline) { + throw AssertionError(message) + } + Thread.sleep(POLL_INTERVAL_MS) + } + } + + /** + * Awaits this latch within [timeoutMs], failing the test with [message] if it times out. + * + * @param message Failure message when the latch does not reach zero in time. + * @param timeoutMs Maximum time to wait; defaults to [TIMEOUT_MS]. + * @param timeUnit Unit for [timeoutMs]; defaults to milliseconds. + */ + private fun CountDownLatch.awaitOrFail( + message: String, + timeoutMs: Long = TIMEOUT_MS, + timeUnit: TimeUnit = TimeUnit.MILLISECONDS, + ) { + if (!await(timeoutMs, timeUnit)) { + throw AssertionError(message) + } + } } diff --git a/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt b/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt index 3fbfe966d..25a6adff6 100644 --- a/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt +++ b/kotlin/src/main/java/app/rive/runtime/kotlin/controllers/RiveFileController.kt @@ -100,7 +100,9 @@ class RiveFileController internal constructor( /** * Whether this controller is active or not. If this is false, it will prevent advancing or * drawing. + * Marked as `@Volatile` to ensure immediate visibility across worker & UI threads */ + @Volatile var isActive = false /** 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 06edcb69d..756f1ef1e 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 @@ -36,6 +36,12 @@ open class RiveArtboardRenderer( } } + /** + * Resizes the active artboard to match the renderer surface. + * + * Must be called with [frameLock] held, then `controller.file?.fileLock` (in that order). + * [draw] is the only call site; locks are not taken here so ordering stays centralized. + */ @WorkerThread private fun resizeArtboard() { if (fit == Fit.LAYOUT) { @@ -57,17 +63,16 @@ open class RiveArtboardRenderer( // Be aware of thread safety! @WorkerThread override fun draw() { - // Deref and draw under frameLock + // Resize 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 + // hasCppObject is only mutated under frameLock; isActive may change on other threads. if (!hasCppObject || !controller.isActive) return - // Protect both resize and draw operations with fileLock to prevent race conditions - // with file/artboard changes on the UI thread. This matches the locking strategy - // used in controller.advance(). Always acquire frameLock before fileLock. + // Protect both resize and draw with fileLock (frameLock first, always). Matches + // controller.advance() and prevents UI-thread file/artboard mutations mid-frame. synchronized(controller.file?.fileLock ?: this) { + // Re-check isActive only; hasCppObject remains stable while frameLock is held. if (!controller.isActive) return if (controller.requireArtboardResize.getAndSet(false)) { resizeArtboard()