[PATCH] msxml3: Handle SAX tags and characters for mxwriter domdoc.

[Jefferson Carpenter]

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