Multiple domdoc and xmlDoc objects may need to share a common properties object but may be released independently. So add a reference count on the properties object.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43377 Signed-off-by: Francois Gouget fgouget@codeweavers.com --- v2: Use Interlocked{Inc,Dec}rement().
{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..19768423b8f 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 = InterlockedIncrement(&properties->refs); + TRACE("(%p)->(%d)\n", properties, ref); + return ref; +} + +static LONG properties_release(domdoc_properties* properties) +{ + LONG ref = InterlockedDecrement(&properties->refs); + 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;
-
Francois Gouget