[PATCH] msxml3: put_output accepts DOMDocument destination

[Nikolay Sivov]

Hi, Jefferson. Please see comments below.
> @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
>           if (!avail)
>           {
> -
>            IStream_Write(writer->dest, buff->data, buff->written, &written);
> -
>            IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
>             buff->written = 0;
>             if (src_len >= buff->allocated)
> -
>                IStream_Write(writer->dest, data, src_len, &written);
> -
>                IStream_Write(writer->dest_stream, data, src_len, &written);
>             else if (src_len)
>             {
>                 memcpy(buff->data, data, src_len);
>
> @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
>                   /* drain what we go so far */
>                   if (buff->written)
>                   {
> -
>                IStream_Write(writer->dest, buff->data, buff->written, &written);
> -
>                IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
>                 buff->written = 0;
>                 avail = buff->allocated;
>             }

I don't think this is a way to deal with that case. When writer output 
is set to DOM document what will probably happen
is a complete tree created and accumulated during SAX calls, and then 
cloned back to said document on endDocument(). That means we'll simply
have to skip over any writing call, both for existing stream or BSTR 
path, and create nodes instead.
> - case VT_DISPATCH:
> - {
> -
>    IXMLDOMDocument *doc;
> -
>    hr = IUnknown_QueryInterface(V_DISPATCH(&dest), &IID_IXMLDOMDocument, (void**)&doc);
> -
>    if (hr == S_OK)
> -
>    {
> -
>        This->dest_type = MXWriterDestDocument;
> -
>        This->dest_document = doc;
> -
>        break;
> -
>    }
> -
>    FIXME("unhandled interface type for VT_DISPATCH destination\n");
> -
>    return E_NOTIMPL;
> - }

It's impossible to tell if this is correct without tests, e.g. 
VT_UNKNOWN could also be passed with document pointer, it's valid in 
VARIANT sense.

So please, if you're interested in working on this, first submit some 
tests and ideally a bug report, if you can tell which application is 
affected.

[Jefferson Carpenter]

Will write tests, thx.

On 4/14/2019 7:29 AM, Nikolay Sivov wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >               if (!avail)
> >               {
> > -                IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                   buff->written = 0;
> >                   if (src_len >= buff->allocated)
> > -                    IStream_Write(writer->dest, data, src_len, 
> > &written);
> > +                    IStream_Write(writer->dest_stream, data, src_len, 
> > &written);
> >                   else if (src_len)
> >                   {
> >                       memcpy(buff->data, data, src_len);
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -                    IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                    IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                       buff->written = 0;
> >                       avail = buff->allocated;
> >                   }
>
> I don't think this is a way to deal with that case. When writer output 
> is set to DOM document what will probably happen
> is a complete tree created and accumulated during SAX calls, and then 
> cloned back to said document on endDocument(). That means we'll simply
> have to skip over any writing call, both for existing stream or BSTR 
> path, and create nodes instead.
> > +    case VT_DISPATCH:
> > +    {
> > +        IXMLDOMDocument *doc;
> > +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), 
> > &IID_IXMLDOMDocument, (void**)&doc);
> > +        if (hr == S_OK)
> > +        {
> > +            This->dest_type = MXWriterDestDocument;
> > +            This->dest_document = doc;
> > +            break;
> > +        }
> > +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > +        return E_NOTIMPL;
> > +    }
>
> It's impossible to tell if this is correct without tests, e.g. 
> VT_UNKNOWN could also be passed with document pointer, it's valid in 
> VARIANT sense.
>
> So please, if you're interested in working on this, first submit some 
> tests and ideally a bug report, if you can tell which application is 
> affected.

[Jefferson Carpenter]

Is there a way for me to close a patch from my end, or do you have to do that?

On Sun, Apr 14, 2019 at 7:29 AM Nikolay Sivov  wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
> >           if (!avail)
> >           {
> > -
> >            IStream_Write(writer->dest, buff->data, buff->written, &written);
> > -
> >            IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
> >             buff->written = 0;
> >             if (src_len >= buff->allocated)
> > -
> >                IStream_Write(writer->dest, data, src_len, &written);
> > -
> >                IStream_Write(writer->dest_stream, data, src_len, &written);
> >             else if (src_len)
> >             {
> >                 memcpy(buff->data, data, src_len);
> >
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -
> >                IStream_Write(writer->dest, buff->data, buff->written, &written);
> > -
> >                IStream_Write(writer->dest_stream, buff->data, buff->written, &written);
> >                 buff->written = 0;
> >                 avail = buff->allocated;
> >             }
>
> I don't think this is a way to deal with that case. When writer output
> is set to DOM document what will probably happen
> is a complete tree created and accumulated during SAX calls, and then
> cloned back to said document on endDocument(). That means we'll simply
> have to skip over any writing call, both for existing stream or BSTR
> path, and create nodes instead.
> > - case VT_DISPATCH:
> > - {
> > -
> >    IXMLDOMDocument *doc;
> > -
> >    hr = IUnknown_QueryInterface(V_DISPATCH(&dest), &IID_IXMLDOMDocument, (void**)&doc);
> > -
> >    if (hr == S_OK)
> > -
> >    {
> > -
> >        This->dest_type = MXWriterDestDocument;
> > -
> >        This->dest_document = doc;
> > -
> >        break;
> > -
> >    }
> > -
> >    FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > -
> >    return E_NOTIMPL;
> > - }
>
> It's impossible to tell if this is correct without tests, e.g.
> VT_UNKNOWN could also be passed with document pointer, it's valid in
> VARIANT sense.
>
> So please, if you're interested in working on this, first submit some
> tests and ideally a bug report, if you can tell which application is
> affected.

[Jefferson Carpenter]

On 4/14/2019 7:29 AM, Nikolay Sivov wrote:
> Hi, Jefferson. Please see comments below.
> > @@ -344,10 +355,10 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >               if (!avail)
> >               {
> > -                IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                   buff->written = 0;
> >                   if (src_len >= buff->allocated)
> > -                    IStream_Write(writer->dest, data, src_len, 
> > &written);
> > +                    IStream_Write(writer->dest_stream, data, src_len, 
> > &written);
> >                   else if (src_len)
> >                   {
> >                       memcpy(buff->data, data, src_len);
> > @@ -371,7 +382,7 @@ static HRESULT write_output_buffer(mxwriter 
> > *writer, const WCHAR *data, int len)
> >                   /* drain what we go so far */
> >                   if (buff->written)
> >                   {
> > -                    IStream_Write(writer->dest, buff->data, 
> > buff->written, &written);
> > +                    IStream_Write(writer->dest_stream, buff->data, 
> > buff->written, &written);
> >                       buff->written = 0;
> >                       avail = buff->allocated;
> >                   }
>
> I don't think this is a way to deal with that case. When writer output 
> is set to DOM document what will probably happen
> is a complete tree created and accumulated during SAX calls, and then 
> cloned back to said document on endDocument(). That means we'll simply
> have to skip over any writing call, both for existing stream or BSTR 
> path, and create nodes instead.

Indeed, I was thinking that instead of calling write_output_buffer at 
all when the destination is a DOMDocument, the DOMDocument's createNode 
etc. functions would be called instead.  Perhaps the mxwriter would need 
to hold a reference to an IXMLDOMNode for in-progress nodes whose 
attributes and child nodes are presently being assigned.

Note that presently even though write_output_buffer is called when the 
destination is a DOMDocument, it now contains the code

+    else {
+        FIXME("unsupported destination type for writing %d\n", 
writer->dest_type);
+    }

which would be called if the destination were set to a DOMDocument.

It's not clear without tests whether the entire result is cloned back on 
endDocument, or whether the DOMDocument is built progressively in 
response to events sent to the SAXContentHandler.
> > +    case VT_DISPATCH:
> > +    {
> > +        IXMLDOMDocument *doc;
> > +        hr = IUnknown_QueryInterface(V_DISPATCH(&dest), 
> > &IID_IXMLDOMDocument, (void**)&doc);
> > +        if (hr == S_OK)
> > +        {
> > +            This->dest_type = MXWriterDestDocument;
> > +            This->dest_document = doc;
> > +            break;
> > +        }
> > +        FIXME("unhandled interface type for VT_DISPATCH destination\n");
> > +        return E_NOTIMPL;
> > +    }
>
> It's impossible to tell if this is correct without tests, e.g. 
> VT_UNKNOWN could also be passed with document pointer, it's valid in 
> VARIANT sense.

Will write tests ASAP.

[Jefferson Carpenter]

On Monday, April 22, 2019, Jefferson Carpenter <
[e-mail]> wrote:
> Indeed, I was thinking that instead of calling write_output_buffer at all

when the destination is a DOMDocument, the DOMDocument's createNode etc.
functions would be called instead.  Perhaps the mxwriter would need to hold
a reference to an IXMLDOMNode for in-progress nodes whose attributes and
child nodes are presently being assigned.
> Note that presently even though write_output_buffer is called when the

destination is a DOMDocument, it now contains the code
> - else {
> -
>    FIXME("unsupported destination type for writing %d\n",

writer->dest_type);
> - }
>
> which would be called if the destination were set to a DOMDocument.
>
> It's not clear without tests whether the entire result is cloned back on

endDocument, or whether the DOMDocument is built progressively in response
to events sent to the SAXContentHandler.
>

But now I've finished updating my computer, so hopefully no one can hack me
and move copyrighted Windows binaries onto my system.