YogaNode Finalization is not thread safe

[JuliaSullivanGoogle]

Report

• [ X ] I have searched existing issues and this is not a duplicate

Issues and Steps to Reproduce

Create many YogaNodes. Finalize them on separate threads. If two try to free at the same exact time, it will fail because the finalization code is not thread safe.

Expected Behavior

To remove nodes from the vector in a thread safe way to avoid a crash in the JNI.

Actual Behavior

It crashed during finalization.

Link to Code

The code that isn't thread safe in the JNI: https://github.com/facebook/yoga/blob/v1.19.0/yoga/Yoga.cpp#L249
The code that isn't thread safe in the Java library: https://github.com/facebook/yoga/blob/v1.19.0/java/com/facebook/yoga/YogaNodeJNIFinalizer.java#L28

We were thinking this might need to be synchronized per tree, so we'd need to get the root of the tree?

https://github.com/facebook/yoga/blob/v1.19.0/java/com/facebook/yoga/YogaNode.java#L42

[JuliaSullivanGoogle]

Also, if you try to synchronize it by synchronizing it locally in each finalizer, it will not work. It has to be a global lock around all instances of YogaNodes or else it will still try to access the JNI finalization code at the same time and crash.

[NickGerleman]

That's a nasty issue. From my reading, finalizes runs on a dedicated GC thread, so it seems like any GC of Yoga nodes is unsafe.

Apart from a global lock, I am first wondering that if YGNodeFree didn't mutate the tree, and really just freed the node, if we would be safe from this. This would be breaking for anything relying on YGNodeFree to remove the node from the tree as well (maybe can split into something like YGNodeRemoveAndFree?).

The JNI bindings do already use the node context API to attach some Java specific information. So another option might be to keep information to allow redispatching destruction to the thread which allocated the node.

Need to look closer at all the options here. Ideally we could avoid adding overhead.

Also FYI @jkoritzinsky since this would impact the SafeHandle based finalizers in the C# port.

[JuliaSullivanGoogle]

Is there anything we can do on our side in the meantime? Do you have an idea of how long it would take to address/fix this and what priority it would be?

[JuliaSullivanGoogle]

QQ, does a lock on freeing a node here lock on all YogaNodes across all trees or does it just lock the one tree?

[NickGerleman]

I took a closer look at how memory is managed in the Java bindings, and would like to know more about how you are seeing concurrent modification happening on different threads.

The Java bindings don’t offer a way to explicitly free a node, so the finalizer is always relied upon. A node on the Java side has references to both parent and children Java nodes, so if a Java Node is finalized, it is guaranteed that the native parent and children nodes also belong to dead Java nodes.

Reading through your issue report again, are there multiple threads doing GC at once? So, multiple finalizers running on the same tree? Or are the edits happening some other way?

If we know the entire connected tree is dead, we could probably make freeing an individual node thread safe by removing the steps to detach the node from the tree. I.e. YGNodeFree would just free() without interfering with any other connected nodes, and we know nothing will access the dangling pointers because everything connected is dead and being finalized.

[JuliaSullivanGoogle]

I can confirm this is definitely multiple threads GCing at once. These are some logs I printed out from System.err. These are as follows:

ThreadId, mNativePointer, Root node's mNativePointer

Starting Deletion: 9 139901460002576 139901460004720
Starting Deletion: 40 139901460001936 139901460004720
Finishing Deletion: 9 0 139901460004720
Finishing Deletion: 40 0 139901460004720

I got the Root node's mNativePointer by doing this:

YogaNodeJNIBase parent = getOwner();
while (parent.getOwner() != null) {
    parent = parent.getOwner();
}

These logs start inside the if statement of the freeNatives function and FinishDeletion is at the end of the if statement. The logs above show that there are two threads (9 and 40) and two different nodes (139901460002576, 139901460001936) are being deleted which are from the same parent (139901460004720)

In these logs we got lucky, but in crashes it fails.

[NickGerleman]

If you are able to locally validate a fix, would you be able to test the below patch?

YOGA_EXPORT void YGNodeFree(const YGNodeRef node) {
-  if (YGNodeRef owner = node->getOwner()) {
-    owner->removeChild(node);
-    node->setOwner(nullptr);
-  }
-
-  const uint32_t childCount = YGNodeGetChildCount(node);
-  for (uint32_t i = 0; i < childCount; i++) {
-    const YGNodeRef child = YGNodeGetChild(node, i);
-    child->setOwner(nullptr);
-  }
-
-  node->clearChildren();
  Event::publish<Event::NodeDeallocation>(node, {node->getConfig()});
  delete node;
}

That change cannot be landed as-is, but if it resolves the issue when using the finalizer there should be a straightforward path to a change which can be merged.

[JuliaSullivanGoogle]

This seems to work! I ran it a few thousand times and never got the flake to happen.

[NickGerleman]

Okay, I think the PR attached above should resolve the issue.

[NickGerleman]

Heads up if you were using the last PR that I made a change after talking to folks, to avoid changing behavior of YGNodeFree for safety, and to scope raw deallocation to just JNI. Should be the same for Java users as tested previously, but without changing any existing C API behavior.

[NickGerleman]

Fixed with 3b088c3

[khandpur]

Hi @NickGerleman, thanks for fixing this issue. If I read the PR correctly, it looks like finalizer calls jni_YGNodeDeallocateJNI which calls YGNodeFree which has the old behavior (modifying the tree and free-ing itself). Did we also want to change jni_YGNodeDeallocateJNI to call into YGNodeDeallocate instead of YGNodeFree?

[NickGerleman]

Ugghh, yes, that was left behind in the switch from the one revision to the other. Let me make a new change to fix this.

[khandpur]

Thank you Nick!