[PATCH] msxml3: Handle SAX tags and characters for mxwriter domdoc.
Moved some code from mxwriter ISAXContentHandler implementation into free functions that can be swapped out for DOMDocument functions. For other mxwriter callback functions, immediately return E_NOTIMPL if the destination is a DOMDocument. Added test_mxwriter_domdoc_start_end_document to help check the implementation of DOMDocument locking. Added "BOOL locked" to XML documents and a private interface that allows this variable to be set and checked. (This variable is not used thread safely. `LONG refs` is thread safe, but `struct list orphans` is not.) thanks, Jefferson
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=88535 Your paranoid android. === debiant2 (64 bit WoW report) === msxml3: Unhandled exception: page fault on execute access to 0x00f65f10 in 64-bit code (0x0000000000f65f10).
On 4/10/2021 2:18 AM, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=88535
Your paranoid android.
=== debiant2 (64 bit WoW report) ===
msxml3: Unhandled exception: page fault on execute access to 0x00f65f10 in 64-bit code (0x0000000000f65f10).
It looks to me like this crash is not related to my patch. However, I did notice I left some debug code (FIXME("here\n")) in the patch. I will do another check for code like that, but wait for any further comments before putting in a Patch V2.
On 4/10/21 4:27 AM, Jefferson Carpenter wrote:
Moved some code from mxwriter ISAXContentHandler implementation into free functions that can be swapped out for DOMDocument functions.
For other mxwriter callback functions, immediately return E_NOTIMPL if the destination is a DOMDocument.
Added test_mxwriter_domdoc_start_end_document to help check the implementation of DOMDocument locking.
Added "BOOL locked" to XML documents and a private interface that allows this variable to be set and checked. (This variable is not used thread safely. `LONG refs` is thread safe, but `struct list orphans` is not.)
thanks, Jefferson @@ -125,6 +125,7 @@ struct domdoc IPersistStreamInit IPersistStreamInit_iface; IObjectWithSite IObjectWithSite_iface; IObjectSafety IObjectSafety_iface; + IWineXMLDOMDocumentLock IWineXMLDOMDocumentLock_iface; IConnectionPointContainer IConnectionPointContainer_iface; LONG ref; VARIANT_BOOL async; Maybe that could work, yes. @@ -209,6 +210,7 @@ typedef struct _xmldoc_priv { LONG refs; struct list orphans; domdoc_properties* properties; + BOOL locked; } xmldoc_priv; Why does it have to be in _priv?
diff --git a/dlls/msxml3/node.c b/dlls/msxml3/node.c index 67f322eb0e5..9cbb2305830 100644 --- a/dlls/msxml3/node.c +++ b/dlls/msxml3/node.c @@ -457,6 +457,8 @@ HRESULT node_insert_before(xmlnode *This, IXMLDOMNode *new_child, const VARIANT if(!new_child) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + node_obj = get_node_obj(new_child); if(!node_obj) return E_FAIL;
@@ -569,6 +571,8 @@ HRESULT node_replace_child(xmlnode *This, IXMLDOMNode *newChild, IXMLDOMNode *ol if(!newChild || !oldChild) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + if(ret) *ret = NULL;
@@ -627,6 +631,8 @@ HRESULT node_remove_child(xmlnode *This, IXMLDOMNode* child, IXMLDOMNode** oldCh
if(!child) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + if(oldChild) *oldChild = NULL;
Maybe, but there's many more methods that modify the tree.
+static HRESULT writer_end_tag_domdoc(mxwriter *writer, const WCHAR *qname, int len) { + HRESULT hr; + IXMLDOMNode *parent_node; + + IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 0); + + hr = domdoc_maybe_write_chars(writer); + if (FAILED(hr)) return hr; + + IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 1); + + hr = IXMLDOMNode_get_parentNode(writer->current_node, &parent_node); + if (FAILED(hr)) return hr; + + IXMLDOMNode_Release(writer->current_node); + writer->current_node = parent_node; + return hr; +} I don't understand this pattern. Idea I had was to lock when output is set to a document, and unlock when everything is written. Is that wrong?
+static HRESULT writer_start_tag_string_len_stream(mxwriter *writer, const WCHAR *qname, int len) +{ + static const WCHAR ltW[] = {'<'}; + + close_element_starttag(writer); + set_element_name(writer, qname ? qname : emptyW, qname ? len : 0); + + write_node_indent(writer); + + write_output_buffer(writer, ltW, 1); + write_output_buffer(writer, qname ? qname : emptyW, qname ? len : 0); + writer_inc_indent(writer); + + return S_OK; +} + +static HRESULT writer_start_tag_bstr_stream(mxwriter *writer, BSTR name) +{ + return writer_start_tag_string_len_stream(writer, name, SysStringLen(name)); +} + Do you need both?
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=89999 Your paranoid android. === debiant2 (build log) === error: corrupt patch at line 203 Task: Patch failed to apply === debiant2 (build log) === error: corrupt patch at line 203 Task: Patch failed to apply
On 5/6/2021 2:12 PM, Nikolay Sivov wrote:
@@ -209,6 +210,7 @@ typedef struct _xmldoc_priv { LONG refs; struct list orphans; domdoc_properties* properties; + BOOL locked; } xmldoc_priv; Why does it have to be in _priv?
I put it there so it could be accessed by the Node functions. For example so node_append_child can tell whether the document is locked.
diff --git a/dlls/msxml3/node.c b/dlls/msxml3/node.c index 67f322eb0e5..9cbb2305830 100644 --- a/dlls/msxml3/node.c +++ b/dlls/msxml3/node.c @@ -457,6 +457,8 @@ HRESULT node_insert_before(xmlnode *This, IXMLDOMNode *new_child, const VARIANT if(!new_child) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + node_obj = get_node_obj(new_child); if(!node_obj) return E_FAIL;
@@ -569,6 +571,8 @@ HRESULT node_replace_child(xmlnode *This, IXMLDOMNode *newChild, IXMLDOMNode *ol if(!newChild || !oldChild) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + if(ret) *ret = NULL;
@@ -627,6 +631,8 @@ HRESULT node_remove_child(xmlnode *This, IXMLDOMNode* child, IXMLDOMNode** oldCh
if(!child) return E_INVALIDARG;
+ if (xmldoc_get_locked(This->node->doc)) return E_FAIL; + if(oldChild) *oldChild = NULL;
Maybe, but there's many more methods that modify the tree.
Yes, in fact I missed some. I could write a test that sets domdoc output, then calls all of them - to figure out which ones would need to have this check.
+static HRESULT writer_end_tag_domdoc(mxwriter *writer, const WCHAR *qname, int len) { + HRESULT hr; + IXMLDOMNode *parent_node; + + IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 0); + + hr = domdoc_maybe_write_chars(writer); + if (FAILED(hr)) return hr; + + IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 1); + + hr = IXMLDOMNode_get_parentNode(writer->current_node, &parent_node); + if (FAILED(hr)) return hr; + + IXMLDOMNode_Release(writer->current_node); + writer->current_node = parent_node; + return hr; +} I don't understand this pattern. Idea I had was to lock when output is set to a document, and unlock when everything is written. Is that wrong?
The mxwriter needs to somehow be able to write to the domdoc even when the domdoc is locked. So it either needs a private interface that skips the locked check, or for the lock to be lifted temporarily. FWIW I don't like this because it isn't thread safe. If there were some badly-behaved code writing to a domdoc in one thread while an mxwriter was writing in another thread, there could be some spurious writes to the domdoc. I think the mxwriter's private interface should include functions that skip the locked check. It will also need IXMLDOMNodeLock (in addition to IXMLDOMDocumentLock) in order to call appendChild during startElement and due to characters.
+static HRESULT writer_start_tag_string_len_stream(mxwriter *writer, const WCHAR *qname, int len) +{ + static const WCHAR ltW[] = {'<'}; + + close_element_starttag(writer); + set_element_name(writer, qname ? qname : emptyW, qname ? len : 0); + + write_node_indent(writer); + + write_output_buffer(writer, ltW, 1); + write_output_buffer(writer, qname ? qname : emptyW, qname ? len : 0); + writer_inc_indent(writer); + + return S_OK; +} + +static HRESULT writer_start_tag_bstr_stream(mxwriter *writer, BSTR name) +{ + return writer_start_tag_string_len_stream(writer, name, SysStringLen(name)); +} + Do you need both?
Added both because the SAX and VBSAX handlers use WCHAR and BSTR respectively - and likewise the stream destination operates on WCHARs and the domdoc destination operates on BSTRs. So having both and having one call into the other (bstr { string_len(); } for stream, string_len { bstr(); } for domdoc) avoids converting back and forth in some cases. Thanks for your comments, Jefferson
On 5/6/2021 6:11 PM, Jefferson Carpenter wrote:
On 5/6/2021 2:12 PM, Nikolay Sivov wrote:
Maybe, but there's many more methods that modify the tree.
Yes, in fact I missed some. I could write a test that sets domdoc output, then calls all of them - to figure out which ones would need to have this check.
This is what I was talking about by testing "all of" the DOMDocument methods and seeing which ones get disabled when it is an active mxwriter destination. These are only the IXMLDOMNode methods - not any of the additional IXMLDOMElement or IXMLDOMDocument methods - but they already show some interesting behavior, e.g. what transformNode does that is not documented on MSDN. I would want to also do this for at least IXMLDOMElement and IXMLDOMDocument. Next version of this patch is planned to include this (with todo_wine added for the remaining unimplemented cases) - unless you don't think it's a good idea, in which case I won't include it. Thanks, Jefferson
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=90108 Your paranoid android. === debiant2 (32 bit report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00470e54). === debiant2 (32 bit French report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00470e54). === debiant2 (32 bit Japanese:Japan report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00470e54). === debiant2 (32 bit Chinese:China report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00470e54). === debiant2 (32 bit WoW report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00470e54). === debiant2 (64 bit WoW report) === msxml3: saxreader.c:4649: Test failed: Failed to set writer output, hr 0x80004001. saxreader.c:4687: Test failed: Unexpected hr 0x1. Unhandled exception: page fault on read access to 0x00000000 in 64-bit code (0x000000000046518e).
participants (3)
-
Jefferson Carpenter -
Marvin -
Nikolay Sivov