Re: mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation.

[Nikolay Sivov]

On 04.11.2015 11:13, Zhenbo Li wrote:
> - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> - if(FAILED(hres)) {
> -
>    ERR("loadXML(%s) failed: %08x\n", debugstr_w(str), hres);
> -
>    IXMLDOMDocument_Release(xmldoc);
> -
>    SysFreeString(str);
> -
>    return hres;
> - }

This is not how it works. loadXML() returns S_FALSE on load failure as 
well, so you have to test strictly for S_OK, or check VARIANT_BOOL 
argument. You can probably add a test when invalid xml stream is 
received to see how _get_responseXML() behaves, e.g. does it return 
S_FALSE and a valid IDispatch or does it fail and return NULL.

Also I don't think it's a good idea to trace xml contents.

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation.

[Zhenbo Li]

Hi Nikolay,

Thanks for your reply.

2015-11-04 16:35 GMT+08:00 Nikolay Sivov :
> On 04.11.2015 11:13, Zhenbo Li wrote:
> > - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> > - if(FAILED(hres)) {
> > -
> >    ERR("loadXML(%s) failed: %08x\n", debugstr_w(str), hres);
> > -
> >    IXMLDOMDocument_Release(xmldoc);
> > -
> >    SysFreeString(str);
> > -
> >    return hres;
> > - }
>
> This is not how it works. loadXML() returns S_FALSE on load failure as well,
> so you have to test strictly for S_OK, or check VARIANT_BOOL argument. You
> can probably add a test when invalid xml stream is received to see how
> _get_responseXML() behaves, e.g. does it return S_FALSE and a valid
> IDispatch or does it fail and return NULL.

You're right. I checked the test case for msxml3.dll, and modified my code to:

hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
    if(vbool != VARIANT_TRUE) {
        ERR("loadXML(%s) failed: %08x\n", debugstr_w(str), hres);
        IXMLDOMDocument_Release(xmldoc);
        SysFreeString(str);
        return hres;
    }
    SysFreeString(str);
> Also I don't think it's a good idea to trace xml contents.

Sorry, I'm not sure the meaning of "trace xml"
If loadXML() fails, I think the terminal output should provide verbose
information

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation.

[Nikolay Sivov]

On 04.11.2015 15:07, Zhenbo Li wrote:
> Hi Nikolay,
>
> Thanks for your reply.
>
> 2015-11-04 16:35 GMT+08:00 Nikolay Sivov :
> > On 04.11.2015 11:13, Zhenbo Li wrote:
> > > - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> > > - if(FAILED(hres)) {
> > > -
> > >    ERR("loadXML(%s) failed: %08x\n", debugstr_w(str), hres);
> > > -
> > >    IXMLDOMDocument_Release(xmldoc);
> > > -
> > >    SysFreeString(str);
> > > -
> > >    return hres;
> > > - }
> >
> > This is not how it works. loadXML() returns S_FALSE on load failure as well,
> > so you have to test strictly for S_OK, or check VARIANT_BOOL argument. You
> > can probably add a test when invalid xml stream is received to see how
> > _get_responseXML() behaves, e.g. does it return S_FALSE and a valid
> > IDispatch or does it fail and return NULL.
>
> You're right. I checked the test case for msxml3.dll, and modified my code to:
>  hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
>  if(vbool != VARIANT_TRUE) {
>      ERR("loadXML(%s) failed: %08x\n", debugstr_w(str), hres);
>      IXMLDOMDocument_Release(xmldoc);
>      SysFreeString(str);
>      return hres;
>  }
>  SysFreeString(str);

It's safer to check both return value and flag (by the way naming it 
something less generic would be better imho); also initializing flag 
before calling loadXML() is not a bad idea as another precaution.
> > Also I don't think it's a good idea to trace xml contents.
>
> Sorry, I'm not sure the meaning of "trace xml"
> If loadXML() fails, I think the terminal output should provide verbose
> information

I disagree, it's not necessary a big deal to get garbage that's not a 
valid xml steam, and it will show up in +msxml. ERR() feels too much, 
but I'll be fine with it either way, let's see what Jacek thinks.

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation.

[Jacek Caban]

On 11/04/15 13:18, Nikolay Sivov wrote:
> On 04.11.2015 15:07, Zhenbo Li wrote:
> > > Also I don't think it's a good idea to trace xml contents.
> >
> > Sorry, I'm not sure the meaning of "trace xml"
> > If loadXML() fails, I think the terminal output should provide verbose
> > information
>
> I disagree, it's not necessary a big deal to get garbage that's not a
> valid xml steam, and it will show up in +msxml. ERR() feels too much,
> but I'll be fine with it either way, let's see what Jacek thinks.

I already asked Zhenbo to change that in private mails before he sent it
to wine-patches. I agree it's not a big deal, but given that it's
already two of us noticing it, please change it, Zhenbo.

Thanks,
Jacek