Fix updateChildren re-rentrancy issue

[ConorGriffin37]

In certain circumstances it is possible for updateChildren in createReconicler to be re-rendered for a virtualNode while it is already happening causing two Roblox instances to be created for the same element.

For an example of how this happens, see the added unit test.

What happens in the test is:

1. The components are rendered the first time and ChildComponent renders a Frame
2. In the spawn in ChildComponent:didMount, firstTime is set to false
3. ChildComponent's Frame is unmounted and a new TextLabel is added
4. The DescendantAdded event is processed
5. ChildComponent rerenders, the old virtualNode for the Frame is still in it's children.
6. ChildComponent's Frame is unmounted again and a second TextLabel is added

This example is kind of a contrived, but this did happen in the UIBlox GridView but with UI events such as AbsoluteContentSize Changed instead of DescentantAdded.

Solution

Suspend the events of parent nodes when updating a component outside of the standard lifecycle. Suspending the parent events means that no change we make can cause a parent node to reconcile while we are also reconciling.

[ZoteTheMighty]

Hmm this is a pretty interesting one! I think we're hitting a confluence of different things that our existing guarantees aren't accounting for, namely the fact that event suspension isn't happening on ParentComponent while its child is updating via setState, since the update came from deeper in the tree.

I don't know if the fix as written really addresses the right part of the issue; maybe it makes the most sense to track when the reconciler is updating, and defer any new updates (presumably caused by Component:setState -> Component:__update -> Component:__resolveUpdate -> reconciler.updateVirtualNodeWithRenderResult, but potentially caused by the more dubious case of a Roact.update cascading down). Since Roact is currently synchronous, it seems like we should just be wary of the unit of work we're dealing with.

On the other hand, we might also consider just warning folks that listening to changes in the hierarchy and responding synchronously is dangerous. I suspect that wrapping the body of the self.descendantAdded callback in a spawn dodges the issue as well?

[ZoteTheMighty]

I'd be curious to know what @oltrep thinks of this!

[ConorGriffin37]

Yeah, maybe more broad event suspension would be a better fix for this. The current fix is way more fragile to future changes even if we add a test for this specific issue.

I don't think we can simply warn folks that listening to changes in the hierarchy and responding synchronously is dangerous because this would mean that many places we respond to AbsoluteContentSize and AbsoluteSize changes would have to be wrapped to spawns. The example shown here is a very basic example so I used DescendantAdded but this is also possible to achieve with components that do bottom up sizing and UI-system events.

[ZoteTheMighty]

I don't think we can simply warn folks that listening to changes in the hierarchy and responding synchronously is dangerous because this would mean that many places we respond to AbsoluteContentSize and AbsoluteSize changes would have to be wrapped to spawns.

I was referring to specifically changes in the hierarchy, not just derived properties in general. This doesn't happen with things like AbsoluteSize because event suspension keeps that subtree consistent.

[ConorGriffin37]

That's not true, this bug in LoadableGridView happens because it listens to a AbsoluteContentSize change.

Event suspension does not prevent it because the events are not suspended in the parent component at the time due to the spawn.

What happens is:

1. GridView calls onWidthChanged inside the spawn
2. DefaultMetricsGridView re-renders based on this new container width
3. The Old Roblox instance for the GridView is unmounted (Instance A)
4. A new Roblox instance for the GridView is created (Instance B) and added.
5. This causes the LoadableGridView to re-render due to listening to AbsoluteContentSize Changed.
6. The Old Roblox instance is still considered the instance for the element because it has not been correctly updated in Roact
7. Instance A is tried to be unmounted again
8. A new Roblox instance for the GridView is created (Instance C) and added.
9. Instance C and Instance B both now exist at the same time.

[matthargett]

This need to be covered with a test, which may help illuminate the discussion thread on the PR as well. Thanks for taking the time to contribute!

[ConorGriffin37]

@matthargett Yeah, I wasn't quite sure how to add a test for this because of Lemur not mocking ChildAdded in a way where it is actually ever fired and Lemur being read-only so I can't go there to try to commit a fix for that.

I did write a test for this which I included in the description. To actually be able to run it as part of the Roact tests I'd probably need to fork Lemur and add a comprehensive mock for DescendantAdded. Wanted to get feedback on the approach first before going to this trouble.

[coveralls]

Coverage increased (+0.8%) to 94.945% when pulling 1422c09 on ConorGriffin37:updateChildrenReEntracyIssue into 8312f84 on Roblox:master.

[ConorGriffin37]

Was totally wrong about why the test didn't work, yield across metamethod/C-call boundary was being caused by using wait, not something to do with the ChildAdded. I changed it to use a coroutine instead of spawn and got rid of the wait. The test works correctly now.

[ConorGriffin37]

Changed to an alternative solution based on suspending parent events when setState is called outside of the components lifecyle. Let me know what you think of this!

[ZoteTheMighty]

Had one thing that looked like an omission; however, some surface level mucking with the logic didn't change anything, so it may not be a gap that gets hit in practice

[ZoteTheMighty]

Is it possible that we we want to to add this in replaceVirtualNode too?

[ConorGriffin37]

Yeah, I think we do. Since we also set depth there that makes sense.

[matthargett]

When we update the ChangeLog for the release, we should mention that this fix is behind a config flag.