Fix updateChildren re-rentrancy issue

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Roact Changelog
 
 ## Unreleased Changes
+* Fixed a bug where the Roact tree could get into a broken state when processing changes to child instances outside the standard lifecycle.
+  This change is behind the config value tempFixUpdateChildrenReEntrancy ([#301](https://github.com/Roblox/roact/pull/301))
 * Added color schemes for documentation based on user preference ([#290](https://github.com/Roblox/roact/pull/290)).
 * Fixed stack trace level when throwing an error in `createReconciler` ([#297](https://github.com/Roblox/roact/pull/297)).
 * Optimized the memory usage of 'createSignal' implementation. ([#304](https://github.com/Roblox/roact/pull/304))

src/Component.lua

@@ -157,9 +157,22 @@ function Component:setState(mapState)
 		internalData.pendingState = assign(newState, derivedState)
 
 	elseif lifecyclePhase == ComponentLifecyclePhase.Idle then
+		-- Pause parent events when we are updated outside of our lifecycle
+		-- If these events are not paused, our setState can cause a component higher up the
+		-- tree to rerender based on events caused by our component while this reconciliation is happening.
+		-- This could cause the tree to become invalid.
+		local virtualNode = internalData.virtualNode
+		local reconciler = internalData.reconciler
+		if config.tempFixUpdateChildrenReEntrancy then
+			reconciler.suspendParentEvents(virtualNode)
+		end
+
 		-- Outside of our lifecycle, the state update is safe to make immediately
 		self:__update(nil, newState)
 
+		if config.tempFixUpdateChildrenReEntrancy then
+			reconciler.resumeParentEvents(virtualNode)
+		end
 	else
 		local messageTemplate = invalidSetStateMessages.default
 

src/Config.lua

@@ -22,6 +22,10 @@ local defaultConfig = {
 	["elementTracing"] = false,
 	-- Enables validation of component props in stateful components.
 	["propValidation"] = false,
+
+	-- Temporary config for enabling a bug fix for processing events based on updates to child instances
+	-- outside of the standard lifecycle.
+	["tempFixUpdateChildrenReEntrancy"] = false,
 }
 
 -- Build a list of valid configuration values up for debug messages.

src/RobloxRenderer.spec.lua

@@ -11,6 +11,7 @@ return function()
 	local GlobalConfig = require(script.Parent.GlobalConfig)
 	local Portal = require(script.Parent.Portal)
 	local Ref = require(script.Parent.PropMarkers.Ref)
+	local Event = require(script.Parent.PropMarkers.Event)
 
 	local RobloxRenderer = require(script.Parent.RobloxRenderer)
 
@@ -946,4 +947,83 @@ return function()
 			})
 		end)
 	end)
+
+
+	describe("Integration Tests", function()
+		it("should not allow re-entrancy in updateChildren", function()
+			local configValues = {
+				tempFixUpdateChildrenReEntrancy = true,
+			}
+
+			GlobalConfig.scoped(configValues, function()
+				local ChildComponent = Component:extend("ChildComponent")
+
+				function ChildComponent:init()
+					self:setState({
+						firstTime = true
+					})
+				end
+
+				local childCoroutine
+
+				function ChildComponent:render()
+					if self.state.firstTime then
+						return createElement("Frame")
+					end
+
+					return createElement("TextLabel")
+				end
+
+				function ChildComponent:didMount()
+					childCoroutine = coroutine.create(function()
+						self:setState({
+							firstTime = false
+						})
+					end)
+				end
+
+				local ParentComponent = Component:extend("ParentComponent")
+
+				function ParentComponent:init()
+					self:setState({
+						count = 1
+					})
+
+					self.childAdded = function()
+						self:setState({
+							count = self.state.count + 1,
+						})
+					end
+				end
+
+				function ParentComponent:render()
+					return createElement("Frame", {
+						[Event.ChildAdded] = self.childAdded,
+					}, {
+						ChildComponent = createElement(ChildComponent, {
+							count = self.state.count
+						})
+					})
+				end
+
+				local parent = Instance.new("ScreenGui")
+				parent.Parent = game.CoreGui
+
+				local tree = createElement(ParentComponent)
+
+				local hostKey = "Some Key"
+				local instance = reconciler.mountVirtualNode(tree, parent, hostKey)
+
+				coroutine.resume(childCoroutine)
+
+				expect(#parent:GetChildren()).to.equal(1)
+
+				local frame = parent:GetChildren()[1]
+
+				expect(#frame:GetChildren()).to.equal(1)
+
+				reconciler.unmountVirtualNode(instance)
+			end)
+		end)
+	end)
 end

src/createReconciler.lua

@@ -37,6 +37,7 @@ local function createReconciler(renderer)
 		local hostParent = virtualNode.hostParent
 		local hostKey = virtualNode.hostKey
 		local depth = virtualNode.depth
+		local parent = virtualNode.parent
 
 		-- If the node that is being replaced has modified context, we need to
 		-- use the original *unmodified* context for the new node
@@ -50,6 +51,7 @@ local function createReconciler(renderer)
 		-- mountVirtualNode can return nil if the element is a boolean
 		if newNode ~= nil then
 			newNode.depth = depth
+			newNode.parent = parent
 		end
 
 		return newNode
@@ -101,6 +103,7 @@ local function createReconciler(renderer)
 				-- mountVirtualNode can return nil if the element is a boolean
 				if childNode ~= nil then
 					childNode.depth = virtualNode.depth + 1
+					childNode.parent = virtualNode
 					virtualNode.children[childKey] = childNode
 				end
 			end
@@ -279,6 +282,7 @@ local function createReconciler(renderer)
 			[Type] = Type.VirtualNode,
 			currentElement = element,
 			depth = 1,
+			parent = nil,
 			children = {},
 			hostParent = hostParent,
 			hostKey = hostKey,
@@ -437,6 +441,28 @@ local function createReconciler(renderer)
 		return tree
 	end
 
+	local function suspendParentEvents(virtualNode)
+		local parentNode = virtualNode.parent
+		while parentNode do
+			if parentNode.eventManager ~= nil then
+				parentNode.eventManager:suspend()
+			end
+
+			parentNode = parentNode.parent
+		end
+	end
+
+	local function resumeParentEvents(virtualNode)
+		local parentNode = virtualNode.parent
+		while parentNode do
+			if parentNode.eventManager ~= nil then
+				parentNode.eventManager:resume()
+			end
+
+			parentNode = parentNode.parent
+		end
+	end
+
 	reconciler = {
 		mountVirtualTree = mountVirtualTree,
 		unmountVirtualTree = unmountVirtualTree,
@@ -448,6 +474,9 @@ local function createReconciler(renderer)
 		updateVirtualNode = updateVirtualNode,
 		updateVirtualNodeWithChildren = updateVirtualNodeWithChildren,
 		updateVirtualNodeWithRenderResult = updateVirtualNodeWithRenderResult,
+
+		suspendParentEvents = suspendParentEvents,
+		resumeParentEvents = resumeParentEvents,
 	}
 
 	return reconciler