Allow NodeSet to contain Nodes from multiple Documents (fix for #1952) #2186
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flavorjones
added
the
topic/memory
|
The failing test at https://ci.nokogiri.org/builds/14968#L60108339:410 looks legit, and so I'll want to dig in a bit more to understand that segfault before I'm happy with this change. |
|
OK, I understand why this segfaulted and have a fix, I only lack the time to polish and push it. |
|
Note to self, next steps:
|
flavorjones
force-pushed
the
1952-node-set-segfault
branch
3 times, most recently
from
41886d9
to
e461142
Compare
in an attempt to more reliably trigger memory bugs when they exist. This behavior can be changed by setting the env var NOKOGIRI_TEST_GC_SETTING to "none", "minor", or "stress"
after being removed when I ripped out Hoe and Hoe::Debugging
Specifically, this means we've added a mark callback for the NodeSet which marks each of the contained objects. Previously we skipped explicitly marking the contained objects due to the assumption that all the nodes would be from the same document as the NodeSet itself. Fixes #1952.
Previously, we created Ruby objects for namespaces and used a @namespace_cache attribute (an Array) to maintain references to just those Ruby objects. Now that we have a real GC mark function for NodeSet, let's use that instead of the hacky array attribute.
flavorjones
force-pushed
the
1952-node-set-segfault
branch
from
e461142
to
26872c9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem is this PR intended to solve?
fix: allow NodeSets to contain Nodes from multiple Documents
Specifically, this means we've added a mark callback for the NodeSet which marks each of the contained objects.
Previously we skipped explicitly marking the contained objects due to the assumption that all the nodes would be from the same document as the NodeSet itself.
Fixes #1952.
Have you included adequate test coverage?
Yes, additional tests created.
Does this change affect the behavior of either the C or the Java implementations?
This change allows the C implementation to do what the Java implementation has always allowed.