Comments (14)
Okay, I think the PR attached above should resolve the issue.
from yoga.
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.
from yoga.
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.
from yoga.
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.
from yoga.
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?
from yoga.
QQ, does a lock on freeing a node here lock on all YogaNodes across all trees or does it just lock the one tree?
from yoga.
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.
from yoga.
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.
from yoga.
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.
from yoga.
This seems to work! I ran it a few thousand times and never got the flake to happen.
from yoga.
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.
from yoga.
Fixed with 3b088c3
from yoga.
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?
from yoga.
Thank you Nick!
from yoga.
Related Issues (20)
- wasm-sync file index error in package.json HOT 2
- [website-next] Replace Yoga Node Editor with `@theme/CodeBlock` HOT 8
- Add `YGNodeStyleSet<Property>CSS` for CSS values as strings HOT 1
- Add `yoga::Style::resolve()` for ahead-of-time computed whole style HOT 2
- Try using explicit modulemap in CocoaPods build HOT 1
- Typo in recent module.modulemap change that breaks Swift PM-based builds HOT 1
- [JS] Move from embind to manual bindings HOT 1
- Flex shorthand in UseWebDefaults should set basis to 0 instead of auto HOT 3
- React Native iOS build failed HOT 1
- top position is wrong when previous item's height is 0.5, on 3x scale screen HOT 1
- API proposal to get the computed measure size of a node HOT 9
- Proposal: Integrate with google/oss-fuzz for continuous fuzz testing? HOT 9
- Can a Yoga Node have multiple owners? HOT 3
- [JS] Reduce WASM call overhead HOT 6
- Add App Privacy Manifest? HOT 1
- `flexBasis` doesn't recalculate when rotating the screen HOT 1
- __dirname is not defined in ES module scope HOT 3
- RuntimeError with WebAssembly in @react-pdf/yoga on Node.js Server HOT 3
- Why can't we round node width/height directly in roundLayoutResultsToPixelGrid()? HOT 3
- The typescript definitions link in the standalone documentation is incorrect HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from yoga.