Created attachment 173918 [details] [review]
Patch [1/4]

Created attachment 173919 [details] [review]
Patch [2/4]

Created attachment 173920 [details] [review]
Patch [3/4]

Created attachment 173921 [details] [review]
Patch [4/4]

> Moreover, on library unloading xmlCleanupParser is called and
> this causes bad crashes if libxml++ is used in a plugin for a program which
> itself uses libxml2.

Does this only happen during explicit unloading via dlclose()? Quite apart from everything else, that seems like something that we should move to an explicit deinit() method that should be called only by apps at the end of their main().

xmlCleanupParser is called inside the destructor of a static C++ object so it should be called when the lib is unloaded in any case, both explicity or implicity (for example when unloading the last library that uses it)

To avoid changing the behavior of the lib for existing apps i suggest adding a settable flag. The default behavior would be calling xmlCleanupParser on unload. Setting the flag should avoid that.

Actually, calling xmlCleanupParser is so bad, and useful only for valgrind checks, that I am happy to change that behaviour.

Murray, could you please review the patches? This issue is pretty much a blocker for the release of the new version of lightspark.

It's the weekend. Have a little patience. I can't believe it's that urgent.

I first want to solve (separately and simply) the main problem that you found - use of xmlCleanupParser. Then I'll look at the rest, hopefully tomorrow.

Created attachment 174048 [details] [review]
combined.patch

I've removed the xmlCleanupParser() call in master.

And here is the combined patch, against the current master. The separation of the commits doesn't seem particularly useful.

Review of attachment 174048 [details] [review]:

::: libxml++/document.h
@@ -173,0 +173,7 @@
+  ///Construct the right C++ instances for a given element
+  static void create_wrapper(_xmlNode* node);
+  ///Recursively destroy the created C++ instances
... 4 more ...

You seem to use casts to make sure that the correct free_wrappers() method is called. Wouldn't it be simpler to just do the check inside the function, assuming that they share a "base" struct that makes that possible?

::: libxml++/nodes/node.cc
@@ -33,3 +34,3 @@
 Element* Node::get_parent()
 {
-  return cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE ? 
+  if(cobj()->parent && cobj()->parent->type == XML_ELEMENT_NODE)

I'd prefer simple checks at the start instead of nesting:

if(!thing)
  return 0;

And please don't use NULL in C++ code. Particularly when !thing is simpler.

@@ -72,3 +91,3 @@
-        if(name.empty() || name == (const char*)child->name)
-          children.push_back(reinterpret_cast<Node*>(child->_private));
-      }
+        if(child->_private==NULL)
+          Document::create_wrapper(child);
+        children.push_back(reinterpret_cast<Node*>(child->_private));

You have used reinterpret_cast<> here though we use static_cast<> elsewhere, I think everywhere, when casting from void* to a C++ type. Please keep it consistent.

@@ -103,2 +113,5 @@
   _xmlNode* node = xmlAddChild(impl_, child);
   if(node)
+  {
+    if(node->_private==NULL)
+      Document::create_wrapper(node);

If we do the if() check inside create_wrapper() then we don't need to do it repeatedly every time we call it.

::: libxml++/validators/dtdvalidator.cc
@@ -103,3 +108,3 @@
   if(dtd_)
   {
-    xmlFreeDtd(dtd_->cobj());
+    //Make a local copy as the wrapper is destroyed first

This comment should make it clear that the "this" instance itself is destroyed at this point. It's effectively a "delete this", so we need to be very careful not to do anything after this point.

Actually, I am concerned: The release_underlying() methods are called from destructors and also from regular methods, which makes me worry about two things:
1. When called from a destructor, isn't this effectively going to call delete on something that has just had delete called on it?
2. When called from a regular method isn't this just going to break that instance by deleting it?

These might not be new problems, of course.

(In reply to comment #11)
> ::: libxml++/validators/dtdvalidator.cc
> @@ -103,3 +108,3 @@
>    if(dtd_)
>    {
> -    xmlFreeDtd(dtd_->cobj());
> +    //Make a local copy as the wrapper is destroyed first
> 
> This comment should make it clear that the "this" instance itself is destroyed
> at this point. It's effectively a "delete this", so we need to be very careful
> not to do anything after this point.
> 
> Actually, I am concerned: The release_underlying() methods are called from
> destructors and also from regular methods, which makes me worry about two
> things:
> 1. When called from a destructor, isn't this effectively going to call delete
> on something that has just had delete called on it?
> 2. When called from a regular method isn't this just going to break that
> instance by deleting it?
> 
> These might not be new problems, of course.
Humm.. it looks to me like the 'this' is not being destroyed. It's the Dtd object contained in DtdValidator that will be destroyed.

Created attachment 174062 [details] [review]
Update combined patch

I've fixed the patch as suggested

(In reply to comment #12)
> Humm.. it looks to me like the 'this' is not being destroyed. It's the Dtd
> object contained in DtdValidator that will be destroyed.

Ah, OK then.

It looks generally good. I'm asking on the mailing list too, in case someone knows of a problem with this approach. It's a large change so I want to be cautious.

I do guess that Document::create_wrapper() should be in Node instead of Document. And create_cpp_instance() might be more obvious. But I can do that myself later.

There has been any suggestion from the ML?

Created attachment 174431 [details] [review]
0001-Make-libxml-compatible-with-libxml2-usage3.patch

The patch with compilation errors corrected. I am now testing this.

I've pushed that an an additional small change. I'll keep testing that. Could you please confirm that the code in git master really fixes your problems, please.

I'm now testing git master with no issue. Do you think it's possible to release a new version to push this fix to distros?

This is now in the unstable libxml++ 2.33.1 tarball release. Thanks.

People should watch our for problems with this.