-
-
Notifications
You must be signed in to change notification settings - Fork 905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Potential GC Bug in Nokogiri 1.17.0 #3359
Comments
While trying to repro locally I ran a couple times into a different crash:
This one rings a bell as we've been trying to track this one down at Shopify for over a year now (crash in |
I seem to have a good repro for it: byroot@16f3df5 I'll try to figure a fix. |
Better repro: byroot@5f88bd9 I'm struggling a bit to follow the relation between the various objects and struct. What I've managed to figure so far:
I'm not yet clear if the bug is that this node should have been marked, or if the bug is that a |
Interestingly if I use Summoning @peterzhu2118 and @XrXr as I'll keep digging some more, but this seem to be above my pay grade, and some |
Thanks for the repro, I'll dig in tomorrow. |
Most likely this is related to #3117 |
Yeah, OK, I understand what's going on. I don't think it's worth having the superteam spend time on it, unless you want to. I've got a couple of ideas for fixes, I'd like to get some sleep and figure out what's best in the morning. |
I mostly pinged them for the |
to make sure it's marked properly. Also, we make sure the new node is decorated if necessary with its document's decorators. Fixes #3359
to make sure it's marked properly. Also, we make sure the new node is decorated if necessary with its document's decorators. Fixes #3359
@byroot Thank you SO MUCH for getting such a tight repro. It was easy to diagnose from there, but you did the hard part. |
**What problem is this PR intended to solve?** `Node#dup` adds the new node to the document's node cache to make sure it's marked properly. Fixes #3359. Also, we make sure the new node is decorated if necessary with its document's decorators. **Have you included adequate test coverage?** Yes! HUGE thanks to @byroot for doing the work to create a tight reproduction. 🙇 **Does this change affect the behavior of either the C or the Java implementations?** Both bugfixes affect the C implementation only.
Just ran into a crash on Rails CI with Ruby 3.2:
https://buildkite.com/rails/rails/builds/114545#0193ab67-9d1b-458c-b14d-f1751a85d259
The text was updated successfully, but these errors were encountered: