From 90c5fa6175e08e51f5df0f4badb3307ec7b26c4d Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Tue, 27 May 2025 10:29:34 -0400 Subject: [PATCH 1/8] [Android] Fix ConcurrentModificationException in InteropUIBlockListene --- .../interop/InteropUiBlockListener.kt | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index 5435a35dbe1346..b361b613a79863 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -42,24 +42,38 @@ internal class InteropUIBlockListener : UIManagerListener { if (beforeUIBlocks.isEmpty()) { return } - beforeUIBlocks.forEach { - if (uiManager is UIBlockViewResolver) { - it.execute(uiManager) + // avoid ConcurrentModificationException by iterating over a copy + try { + val snapshot = ArrayList(beforeUIBlocks) + snapshot.forEach { block -> + if (uiManager is UIBlockViewResolver) { + block.execute(uiManager) + } } + } catch (e: ConcurrentModificationException) { + // ignore any mid-iteration mutations + } finally { + beforeUIBlocks.clear() } - beforeUIBlocks.clear() } override fun didMountItems(uiManager: UIManager) { if (afterUIBlocks.isEmpty()) { return } - afterUIBlocks.forEach { - if (uiManager is UIBlockViewResolver) { - it.execute(uiManager) + // avoid ConcurrentModificationException by iterating over a copy + try { + val snapshot = ArrayList(afterUIBlocks) + snapshot.forEach { block -> + if (uiManager is UIBlockViewResolver) { + block.execute(uiManager) + } } + } catch (e: ConcurrentModificationException) { + // ignore any mid-iteration mutations + } finally { + afterUIBlocks.clear() } - afterUIBlocks.clear() } override fun didDispatchMountItems(uiManager: UIManager) = didMountItems(uiManager) From 3d5094a126d628a415ab77324adf68ad3609626b Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Tue, 27 May 2025 14:49:12 -0400 Subject: [PATCH 2/8] cleanup --- .../interop/InteropUiBlockListener.kt | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index b361b613a79863..0fef990145585a 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -39,39 +39,29 @@ internal class InteropUIBlockListener : UIManagerListener { } override fun willMountItems(uiManager: UIManager) { - if (beforeUIBlocks.isEmpty()) { - return - } // avoid ConcurrentModificationException by iterating over a copy try { - val snapshot = ArrayList(beforeUIBlocks) - snapshot.forEach { block -> - if (uiManager is UIBlockViewResolver) { + if (uiManager is UIBlockViewResolver) { + val snapshot = ArrayList(beforeUIBlocks) + snapshot.forEach { block -> block.execute(uiManager) } } - } catch (e: ConcurrentModificationException) { - // ignore any mid-iteration mutations - } finally { + } finally { beforeUIBlocks.clear() } } override fun didMountItems(uiManager: UIManager) { - if (afterUIBlocks.isEmpty()) { - return - } // avoid ConcurrentModificationException by iterating over a copy try { - val snapshot = ArrayList(afterUIBlocks) - snapshot.forEach { block -> - if (uiManager is UIBlockViewResolver) { + if (uiManager is UIBlockViewResolver) { + val snapshot = ArrayList(afterUIBlocks) + snapshot.forEach { block -> block.execute(uiManager) } } - } catch (e: ConcurrentModificationException) { - // ignore any mid-iteration mutations - } finally { + } finally { afterUIBlocks.clear() } } From 529dc5c7aa5de912d29ac09c438845da40bd2150 Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Tue, 27 May 2025 16:39:34 -0400 Subject: [PATCH 3/8] Avoid possible clear() raise condition --- .../react/fabric/internal/interop/InteropUiBlockListener.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index 0fef990145585a..2e6e4b05b81fef 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -43,12 +43,11 @@ internal class InteropUIBlockListener : UIManagerListener { try { if (uiManager is UIBlockViewResolver) { val snapshot = ArrayList(beforeUIBlocks) + beforeUIBlocks.clear() snapshot.forEach { block -> block.execute(uiManager) } } - } finally { - beforeUIBlocks.clear() } } @@ -57,12 +56,11 @@ internal class InteropUIBlockListener : UIManagerListener { try { if (uiManager is UIBlockViewResolver) { val snapshot = ArrayList(afterUIBlocks) + afterUIBlocks.clear() snapshot.forEach { block -> block.execute(uiManager) } } - } finally { - afterUIBlocks.clear() } } From b2ad8e827899782883d8c1e07ddae0db07ec6716 Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Tue, 27 May 2025 17:09:52 -0400 Subject: [PATCH 4/8] further cleanup --- .../interop/InteropUiBlockListener.kt | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index 2e6e4b05b81fef..d288692b103853 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -38,28 +38,32 @@ internal class InteropUIBlockListener : UIManagerListener { afterUIBlocks.add(block) } + @Synchronized override fun willMountItems(uiManager: UIManager) { + if (beforeUIBlocks.isEmpty()) { + return + } // avoid ConcurrentModificationException by iterating over a copy - try { - if (uiManager is UIBlockViewResolver) { - val snapshot = ArrayList(beforeUIBlocks) - beforeUIBlocks.clear() - snapshot.forEach { block -> - block.execute(uiManager) - } + val snapshot = ArrayList(beforeUIBlocks) + beforeUIBlocks.clear() + if (uiManager is UIBlockViewResolver) { + snapshot.forEach { block -> + block.execute(uiManager) } } } + @Synchronized override fun didMountItems(uiManager: UIManager) { + if (afterUIBlocks.isEmpty()) { + return + } // avoid ConcurrentModificationException by iterating over a copy - try { - if (uiManager is UIBlockViewResolver) { - val snapshot = ArrayList(afterUIBlocks) - afterUIBlocks.clear() - snapshot.forEach { block -> - block.execute(uiManager) - } + val snapshot = ArrayList(afterUIBlocks) + afterUIBlocks.clear() + if (uiManager is UIBlockViewResolver) { + snapshot.forEach { block -> + block.execute(uiManager) } } } From 95feba8f34b1b6ea72117b3d9d96fd3d9ba2e4a0 Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Wed, 28 May 2025 08:46:10 -0400 Subject: [PATCH 5/8] All we ever needed was @Synchronized --- .../interop/InteropUiBlockListener.kt | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index d288692b103853..21172244f610b4 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -41,31 +41,27 @@ internal class InteropUIBlockListener : UIManagerListener { @Synchronized override fun willMountItems(uiManager: UIManager) { if (beforeUIBlocks.isEmpty()) { - return - } - // avoid ConcurrentModificationException by iterating over a copy - val snapshot = ArrayList(beforeUIBlocks) - beforeUIBlocks.clear() - if (uiManager is UIBlockViewResolver) { - snapshot.forEach { block -> - block.execute(uiManager) + return + } + beforeUIBlocks.forEach { + if (uiManager is UIBlockViewResolver) { + it.execute(uiManager) } } + beforeUIBlocks.clear() } @Synchronized override fun didMountItems(uiManager: UIManager) { if (afterUIBlocks.isEmpty()) { - return - } - // avoid ConcurrentModificationException by iterating over a copy - val snapshot = ArrayList(afterUIBlocks) - afterUIBlocks.clear() - if (uiManager is UIBlockViewResolver) { - snapshot.forEach { block -> - block.execute(uiManager) + return + } + afterUIBlocks.forEach { + if (uiManager is UIBlockViewResolver) { + it.execute(uiManager) } } + afterUIBlocks.clear() } override fun didDispatchMountItems(uiManager: UIManager) = didMountItems(uiManager) From e6f6d391732bfac3ed32e9c109842e6f27901b8d Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Wed, 28 May 2025 11:49:07 -0400 Subject: [PATCH 6/8] Revert "All we ever needed was @Synchronized" This reverts commit 95feba8f34b1b6ea72117b3d9d96fd3d9ba2e4a0. --- .../interop/InteropUiBlockListener.kt | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index 21172244f610b4..d288692b103853 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -41,27 +41,31 @@ internal class InteropUIBlockListener : UIManagerListener { @Synchronized override fun willMountItems(uiManager: UIManager) { if (beforeUIBlocks.isEmpty()) { - return - } - beforeUIBlocks.forEach { - if (uiManager is UIBlockViewResolver) { - it.execute(uiManager) + return + } + // avoid ConcurrentModificationException by iterating over a copy + val snapshot = ArrayList(beforeUIBlocks) + beforeUIBlocks.clear() + if (uiManager is UIBlockViewResolver) { + snapshot.forEach { block -> + block.execute(uiManager) } } - beforeUIBlocks.clear() } @Synchronized override fun didMountItems(uiManager: UIManager) { if (afterUIBlocks.isEmpty()) { - return - } - afterUIBlocks.forEach { - if (uiManager is UIBlockViewResolver) { - it.execute(uiManager) + return + } + // avoid ConcurrentModificationException by iterating over a copy + val snapshot = ArrayList(afterUIBlocks) + afterUIBlocks.clear() + if (uiManager is UIBlockViewResolver) { + snapshot.forEach { block -> + block.execute(uiManager) } } - afterUIBlocks.clear() } override fun didDispatchMountItems(uiManager: UIManager) = didMountItems(uiManager) From 6fb8ac462140236385d7754222af567de8d84d8c Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Wed, 28 May 2025 15:35:24 -0400 Subject: [PATCH 7/8] The copying of beforeUiblocks and clearing of it still needs to happen in a synchronized block --- .../interop/InteropUiBlockListener.kt | 34 ++++++++++--------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index d288692b103853..ad1ee44ef4253c 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -38,31 +38,33 @@ internal class InteropUIBlockListener : UIManagerListener { afterUIBlocks.add(block) } - @Synchronized override fun willMountItems(uiManager: UIManager) { - if (beforeUIBlocks.isEmpty()) { - return - } - // avoid ConcurrentModificationException by iterating over a copy - val snapshot = ArrayList(beforeUIBlocks) - beforeUIBlocks.clear() + val blocksToExecute: List = synchronized(this) { + if (beforeUIBlocks.isEmpty()) { + return + } + // avoid ConcurrentModificationException by iterating over a copy + ArrayList(beforeUIBlocks).also { beforeUIBlocks.clear() } + } + if (uiManager is UIBlockViewResolver) { - snapshot.forEach { block -> + blocksToExecute.forEach { block -> block.execute(uiManager) } } } - @Synchronized override fun didMountItems(uiManager: UIManager) { - if (afterUIBlocks.isEmpty()) { - return - } - // avoid ConcurrentModificationException by iterating over a copy - val snapshot = ArrayList(afterUIBlocks) - afterUIBlocks.clear() + val blocksToExecute: List = synchronized(this) { + if (afterUIBlocks.isEmpty()) { + return + } + // avoid ConcurrentModificationException by iterating over a copy + ArrayList(afterUIBlocks).also { afterUIBlocks.clear() } + } + if (uiManager is UIBlockViewResolver) { - snapshot.forEach { block -> + blocksToExecute.forEach { block -> block.execute(uiManager) } } From 75f2b751d24e02ab0c8cce77095b8f4a08200a0c Mon Sep 17 00:00:00 2001 From: Jay Koutavas Date: Thu, 29 May 2025 13:54:28 -0400 Subject: [PATCH 8/8] prefer toList() over ArrayList() --- .../fabric/internal/interop/InteropUiBlockListener.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt index ad1ee44ef4253c..c9b7b1540b8e26 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/internal/interop/InteropUiBlockListener.kt @@ -43,11 +43,11 @@ internal class InteropUIBlockListener : UIManagerListener { if (beforeUIBlocks.isEmpty()) { return } - // avoid ConcurrentModificationException by iterating over a copy - ArrayList(beforeUIBlocks).also { beforeUIBlocks.clear() } + beforeUIBlocks.toList().also { beforeUIBlocks.clear() } } if (uiManager is UIBlockViewResolver) { + // avoid ConcurrentModificationException by iterating over a copy blocksToExecute.forEach { block -> block.execute(uiManager) } @@ -59,11 +59,11 @@ internal class InteropUIBlockListener : UIManagerListener { if (afterUIBlocks.isEmpty()) { return } - // avoid ConcurrentModificationException by iterating over a copy - ArrayList(afterUIBlocks).also { afterUIBlocks.clear() } + afterUIBlocks.toList().also { afterUIBlocks.clear() } } if (uiManager is UIBlockViewResolver) { + // avoid ConcurrentModificationException by iterating over a copy blocksToExecute.forEach { block -> block.execute(uiManager) }