Am Donnerstag, den 16.10.2008, 13:55 -0500 schrieb Jacek Caban: @@ -932,9 +933,17 @@ static HRESULT WINAPI domdoc_put_documentElement( return hr;
xmlNode = impl_from_IXMLDOMNode( elementNode ); - xmlDocSetRootElement( get_doc(This), xmlNode->node); + + if(!xmlNode->node->parent) + if(xmldoc_remove_orphan(xmlNode->node->doc, xmlNode->node) != S_OK) + WARN("%p is not an orphan of %p\n", xmlNode->node->doc, xmlNode->node); + + oldRoot = xmlDocSetRootElement( get_doc(This), xmlNode->node); IXMLDOMNode_Release( elementNode );
+ if(oldRoot) + xmldoc_add_orphan(oldRoot->doc, oldRoot); + return S_OK; }
This patch is against my intention of the orphan list. The orphan list should contain nodes that are associated to the document, but are not in the tree (i.e. not an descendant of the document root element). The orphan list is used to not forget to free these nodes (that logically reference the document and have to keep the document alive) when the document itself (i.e. the root document element) is freed. Now oldRoot already *is* the root element, that is oldRoot->doc == oldRoot. Placeing the element itself on the orphan list causes it to be freed when freeing the orphans and a second time (some lines later) when the document element is freed.
So, I doubt this patch is correct.
Regards, Michael Karcher
Michael Karcher wrote:
Am Donnerstag, den 16.10.2008, 13:55 -0500 schrieb Jacek Caban: @@ -932,9 +933,17 @@ static HRESULT WINAPI domdoc_put_documentElement( return hr;
xmlNode = impl_from_IXMLDOMNode( elementNode );
- xmlDocSetRootElement( get_doc(This), xmlNode->node);
if(!xmlNode->node->parent)
if(xmldoc_remove_orphan(xmlNode->node->doc, xmlNode->node) != S_OK)WARN("%p is not an orphan of %p\n", xmlNode->node->doc, xmlNode->node);oldRoot = xmlDocSetRootElement( get_doc(This), xmlNode->node); IXMLDOMNode_Release( elementNode );
if(oldRoot)
xmldoc_add_orphan(oldRoot->doc, oldRoot);return S_OK;
}
This patch is against my intention of the orphan list. The orphan list should contain nodes that are associated to the document, but are not in the tree (i.e. not an descendant of the document root element). The orphan list is used to not forget to free these nodes (that logically reference the document and have to keep the document alive) when the document itself (i.e. the root document element) is freed. Now oldRoot already *is* the root element, that is oldRoot->doc == oldRoot. Placeing the element itself on the orphan list causes it to be freed when freeing the orphans and a second time (some lines later) when the document element is freed.
So, I doubt this patch is correct.
This patch fixes regression in Photoshop CS4 installer caused by your patch. The double free is exactly what happened with your patch and my patch fixes it. The new root has to be removed from orphan list because it is added to DOM tree. Otherwise we'd free it two times. Also oldRoot is no longer in DOM tree, so it should be added to orphan list to be freed later.
Jacek
Am Donnerstag, den 16.10.2008, 15:42 -0500 schrieb Jacek Caban:
So, I doubt this patch is correct.
This patch fixes regression in Photoshop CS4 installer caused by your patch. The double free is exactly what happened with your patch and my patch fixes it. The new root has to be removed from orphan list because it is added to DOM tree. Otherwise we'd free it two times. Also oldRoot is no longer in DOM tree, so it should be added to orphan list to be freed later.
Sorry, I was confused. Your patch is completely correct. I mixed up the root element and the document element.
Regards, Michael Karcher
-
Jacek Caban -
Michael Karcher