Skip to content
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

Free all unlinked nodes #1791

Closed
wants to merge 1 commit into from
Closed

Free all unlinked nodes #1791

wants to merge 1 commit into from

Conversation

stevecheckoway
Copy link
Contributor

Nodes that have been relinked should not be freed since they will be
freed when their owning document is freed. Every other node can be freed
directly.

Previously, unlinked DTD nodes were leaked because xmlFreeNodeList
does not act on DTD nodes. Separately, the href and prefix members
of xmlNs nodes (those of type XML_NAMESPACE_DECL) were also being
leaked.

Fixes #1784

@stevecheckoway
Copy link
Contributor Author

Okay, I don't understand the namespace handling but perhaps something else owns the href and prefix strings?

DTD nodes added to a document are not freed when the document is freed.

Fixes #1784
@stevecheckoway
Copy link
Contributor Author

Whelp, I have absolutely no idea what the issue here is then. The Valgrind invalid reads don't seem related to my change.

@flavorjones
Copy link
Member

I'll take a look when I get a chance.

@flavorjones flavorjones added this to the next milestone Dec 1, 2018
@flavorjones flavorjones self-assigned this Dec 1, 2018
@flavorjones
Copy link
Member

@stevecheckoway If you close and reopen this PR you should kick off another build which will (properly) ignore those valgrind errors. See ec52483 for context. No need to rebase.

@flavorjones flavorjones closed this Dec 1, 2018
@flavorjones flavorjones reopened this Dec 1, 2018
@flavorjones
Copy link
Member

Oh - but please rebase to get the JRuby test failures to pass. If you do that in the next day or two, I'll merge into the next release (or if I have time I'll do it myself). Thanks!

@flavorjones
Copy link
Member

I've rebased and pushed to a new branch under #1825. Please follow along there!

@flavorjones flavorjones closed this Dec 1, 2018
@stevecheckoway stevecheckoway deleted the free-all-nodes branch December 1, 2018 18:04
@flavorjones flavorjones added this to the v1.9.0 milestone Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removing internal_subset leaks memory
2 participants