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

[Nikolay Sivov]

I don't know if it's terribly important in practice or not, but it could 
be better to always initialize out pointer to NULL.
> +    hres = IXMLDOMDocument_QueryInterface(xmldoc, &IID_IObjectSafety, (void**)&safety);
> +    if(SUCCEEDED(hres)) {
> +        hres = IObjectSafety_SetInterfaceSafetyOptions(safety, NULL,
> +            INTERFACESAFE_FOR_UNTRUSTED_CALLER | INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER,
> +            INTERFACESAFE_FOR_UNTRUSTED_CALLER | INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER);
> +        IObjectSafety_Release(safety);
> +        if(FAILED(hres)) {
> +            ERR("SetInterfaceSafetyOptions(%p) failed: %08x\n", safety, hres);
> +            IXMLDOMDocument_Release(xmldoc);
> +            return hres;
> +        }
> +    } else {
> +        ERR("QueryInterface(IID_IObjectSafety) failed: %08x\n", hres);
> +        IXMLDOMDocument_Release(xmldoc);
> +        return hres;
> +    }

This looks too complicated to me, I don't think any failure here is 
critical.

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

[Zhenbo Li]

Hi Nikolay,

2015-11-04 22:44 GMT+08:00 Nikolay Sivov <bunglehead(a)gmail.com>:
> I don't know if it's terribly important in practice or not, but it could be
> better to always initialize out pointer to NULL.

So how about adding such code at the beginning of this function

    if(!p)
        return E_POINTER;
    *p = NULL;
> > +    hres = IXMLDOMDocument_QueryInterface(xmldoc, &IID_IObjectSafety,
> > (void**)&safety);
> > +    if(SUCCEEDED(hres)) {
> > +        hres = IObjectSafety_SetInterfaceSafetyOptions(safety, NULL,
> > +            INTERFACESAFE_FOR_UNTRUSTED_CALLER |
> > INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER,
> > +            INTERFACESAFE_FOR_UNTRUSTED_CALLER |
> > INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER);
> > +        IObjectSafety_Release(safety);
> > +        if(FAILED(hres)) {
> > +            ERR("SetInterfaceSafetyOptions(%p) failed: %08x\n", safety,
> > hres);
> > +            IXMLDOMDocument_Release(xmldoc);
> > +            return hres;
> > +        }
> > +    } else {
> > +        ERR("QueryInterface(IID_IObjectSafety) failed: %08x\n", hres);
> > +        IXMLDOMDocument_Release(xmldoc);
> > +        return hres;
> > +    }
>
> This looks too complicated to me, I don't think any failure here is
> critical.

The code is too complicated to me, either. But I haven't find a better
solution for that.


-- 
Have a nice day!
Zhenbo Li

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

[Nikolay Sivov]

On 04.11.2015 18:11, Zhenbo Li wrote:
> Hi Nikolay,
>
> 2015-11-04 22:44 GMT+08:00 Nikolay Sivov <bunglehead(a)gmail.com>:
> > I don't know if it's terribly important in practice or not, but it could be
> > better to always initialize out pointer to NULL.
>
> So how about adding such code at the beginning of this function
>
> if(!p)
>          return E_POINTER;
>      *p = NULL;

Sure, if test proves that it handles NULL arguments.
> > > +    hres = IXMLDOMDocument_QueryInterface(xmldoc, &IID_IObjectSafety,
> > > (void**)&safety);
> > > +    if(SUCCEEDED(hres)) {
> > > +        hres = IObjectSafety_SetInterfaceSafetyOptions(safety, NULL,
> > > +            INTERFACESAFE_FOR_UNTRUSTED_CALLER |
> > > INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER,
> > > +            INTERFACESAFE_FOR_UNTRUSTED_CALLER |
> > > INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER);
> > > +        IObjectSafety_Release(safety);
> > > +        if(FAILED(hres)) {
> > > +            ERR("SetInterfaceSafetyOptions(%p) failed: %08x\n", safety,
> > > hres);
> > > +            IXMLDOMDocument_Release(xmldoc);
> > > +            return hres;
> > > +        }
> > > +    } else {
> > > +        ERR("QueryInterface(IID_IObjectSafety) failed: %08x\n", hres);
> > > +        IXMLDOMDocument_Release(xmldoc);
> > > +        return hres;
> > > +    }
> >
> > This looks too complicated to me, I don't think any failure here is
> > critical.
>
> The code is too complicated to me, either. But I haven't find a better
> solution for that.

If I was doing it, I'd simply QI for IObjectSafety, if SUCCEEDED() -> 
SetInterfaceSafetyOptions(), and Release(), no additional error 
checking. Jacek, what do you think?