Fix #80927: Removing documentElement after creating attribute node: p…

[ndossche]

…ossible use-after-free

Not 100% convinced that this solution is optimal. Also targeting master only because this is a non-trivial change in how the destruction works.
Marking as draft for now until I convince myself more that there's no better alternative.

[ndossche]

I don't see another way to solve this issue ATM.

The main problem is that libxml2 doesn't have a reference count on the namespace declaration. I understand why this is, it's non-trivial to keep track of (e.g. clones, imports, adoptions, ...) and normally it doesn't matter.

We don't actually need to save the namespace declaration if we know the subtree it belongs to has no references from userland. However, we can't know that without traversing the whole subtree (=> slow), or without adding some subtree metadata (=> also slow). So we have to assume we need to save everything.

However, namespace declarations are quite rare in comparison to other node types. Most node types are either elements, text or attributes. And you only need one namespace declaration per namespace (in principle). So I expect the number of namespace declarations to be low for an average XML document.

In the worst possible case we have to save all namespace declarations when we for example remove the whole document. But given the above reasoning this likely won't be a lot of declarations even in the worst case.
A single declaration only takes about 48 bytes of memory, and I don't expect the worst case to occur very often (why would you remove the whole document?).
So I guess this is fine (but still ugly).

I'm going to mark this as ready, and update the comment in the code to include some of this text for future reference.

[Girgias]

LGTM, just minor comments that can be ignored.

[Girgias]

You could use an Enum and type against this for the variation arg

[Girgias]

I get that this how it was previously written, and the default case is very small and clear. But I wonder if it just doesn't make more sense to keep it self-contained and have:

				xmlFreeNode(node);
				break;

here too.