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 <bunglehead(a)gmail.com>:
> 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

-- 
Have a nice day!
Zhenbo Li

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 <bunglehead(a)gmail.com>:
> > 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