[PATCH] msxml3: Refcount the domdoc/xmldoc properties. - wine-devel

29 Apr 2021

Multiple domdoc and xmlDoc objects may need to share a common properties 
structure but even though they may be released independently. So add a 
reference count on the properties structure.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43377
Signed-off-by: Francois Gouget fgouget@codeweavers.com
---
{create,copy}_properties() both initialize the refcount to 1 because
it's simpler that way given that the object is always used after being
created. create_priv() initializes its refcount to 0 instead but then
its use case seems more complex.
I decided to investigate this because msxml3:domdoc started failing on
2021-03-25.
https://test.winehq.org/data/patterns-tb-wine.html
Given the Wine trouble the day before this looked like it could be the
symptom of some widespread issue. But then I could still reproduce the
issue when building from the 23rd source which was puzzling. It turns
out this is an old failure (as evidenced by the bug) and we just got
lucky to avoid crashes for a while.
---
 dlls/msxml3/domdoc.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/dlls/msxml3/domdoc.c b/dlls/msxml3/domdoc.c
index cf4f0433218..73ac97ab89a 100644
--- a/dlls/msxml3/domdoc.c
+++ b/dlls/msxml3/domdoc.c
@@ -80,6 +80,7 @@ static const WCHAR PropertyNormalizeAttributeValuesW[] = {'N','o','r','m','a','l
  * We need to preserve this when reloading a document,
  * and also need access to it from the libxml backend. */
 typedef struct {
+    LONG refs;
     MSXML_VERSION version;
     VARIANT_BOOL preserving;
     IXMLDOMSchemaCollection2* schemaCache;
@@ -290,6 +291,7 @@ static domdoc_properties *create_properties(MSXML_VERSION version)
 {
     domdoc_properties *properties = heap_alloc(sizeof(domdoc_properties));
+    properties->refs = 1;
     list_init(&properties->selectNsList);
     properties->preserving = VARIANT_FALSE;
     properties->schemaCache = NULL;
@@ -316,6 +318,7 @@ static domdoc_properties* copy_properties(domdoc_properties const* properties)
if (pcopy)
     {
+        pcopy->refs = 1;
         pcopy->version = properties->version;
         pcopy->preserving = properties->preserving;
         pcopy->schemaCache = properties->schemaCache;
@@ -345,9 +348,22 @@ static domdoc_properties* copy_properties(domdoc_properties const* properties)
     return pcopy;
 }
-static void free_properties(domdoc_properties* properties)
+LONG properties_add_ref(domdoc_properties* properties)
 {
-    if (properties)
+    LONG ref = InterlockedExchangeAdd(&properties->refs, 1) + 1;
+    TRACE("(%p)->(%d)\n", properties, ref);
+    return ref;
+}
+
+static LONG properties_release(domdoc_properties* properties)
+{
+    LONG ref = InterlockedExchangeAdd(&properties->refs, -1) - 1;
+    TRACE("(%p)->(%d)\n", properties, ref);
+
+    if (ref < 0)
+        WARN("negative refcount, expect troubles\n");
+
+    if (ref == 0)
     {
         if (properties->schemaCache)
             IXMLDOMSchemaCollection2_Release(properties->schemaCache);
@@ -357,6 +373,8 @@ static void free_properties(domdoc_properties* properties)
             IUri_Release(properties->uri);
         heap_free(properties);
     }
+
+    return ref;
 }
static void release_namespaces(domdoc *This)
@@ -622,7 +640,8 @@ LONG xmldoc_release_refs(xmlDocPtr doc, LONG refs)
             xmlFreeNode( orphan->node );
             heap_free( orphan );
         }
-        free_properties(priv->properties);
+        if (priv->properties)
+            properties_release(priv->properties);
         heap_free(doc->_private);
xmlFreeDoc(doc);
@@ -679,10 +698,17 @@ static HRESULT attach_xmldoc(domdoc *This, xmlDocPtr xml )
if(This->node.node)
     {
+        if (properties_from_xmlDocPtr(get_doc(This)))
+            properties_release(properties_from_xmlDocPtr(get_doc(This)));
         priv_from_xmlDocPtr(get_doc(This))->properties = NULL;
         if (xmldoc_release(get_doc(This)) != 0)
+        {
+            /* The xmlDocPtr object can no longer use the properties of this
+             * domdoc object. So give it its own copy.
+             */
             priv_from_xmlDocPtr(get_doc(This))->properties =
                 copy_properties(This->properties);
+        }
     }
This->node.node = (xmlNodePtr) xml;
@@ -690,7 +716,11 @@ static HRESULT attach_xmldoc(domdoc *This, xmlDocPtr xml )
     if(This->node.node)
     {
         xmldoc_add_ref(get_doc(This));
+        /* Only attach new xmlDocPtr objects, i.e. ones for which properties
+         * is still NULL.
+         */
         priv_from_xmlDocPtr(get_doc(This))->properties = This->properties;
+        properties_add_ref(properties_from_xmlDocPtr(get_doc(This)));
     }
return S_OK;
@@ -975,6 +1005,7 @@ static ULONG WINAPI domdoc_Release( IXMLDOMDocument3 *iface )
         for (eid = 0; eid < EVENTID_LAST; eid++)
             if (This->events[eid]) IDispatch_Release(This->events[eid]);
+        properties_release(This->properties);
         release_namespaces(This);
         heap_free(This);
     }
@@ -3676,6 +3707,8 @@ HRESULT get_domdoc_from_xmldoc(xmlDocPtr xmldoc, IXMLDOMDocument3 **document)
     doc->validating = 0;
     doc->resolving = 0;
     doc->properties = properties_from_xmlDocPtr(xmldoc);
+    if (doc->properties)
+        properties_add_ref(doc->properties);
     doc->error = S_OK;
     doc->site = NULL;
     doc->base_uri = NULL;
@@ -3714,7 +3747,8 @@ HRESULT DOMDocument_create(MSXML_VERSION version, void **ppObj)
     hr = get_domdoc_from_xmldoc(xmldoc, (IXMLDOMDocument3**)ppObj);
     if(FAILED(hr))
     {
-        free_properties(properties_from_xmlDocPtr(xmldoc));
+        if (properties_from_xmlDocPtr(xmldoc))
+            properties_release(properties_from_xmlDocPtr(xmldoc));
         heap_free(xmldoc->_private);
         xmlFreeDoc(xmldoc);
         return hr;
-- 
2.20.1


    