Re: wshom.ocx: Properly update typeinfo refcount

[Alexandre Julliard]

Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> @@ -137,8 +137,13 @@ static HRESULT WINAPI WshCollection_GetTypeInfoCount(IWshCollection *iface, UINT
>  static HRESULT WINAPI WshCollection_GetTypeInfo(IWshCollection *iface, UINT iTInfo, LCID lcid, ITypeInfo **ppTInfo)
>  {
>      WshCollection *This = impl_from_IWshCollection(iface);
> +    HRESULT hr;
> +
>      TRACE("(%p)->(%u %u %p)\n", This, iTInfo, lcid, ppTInfo);
> -    return get_typeinfo(IWshCollection_tid, ppTInfo);
> +
> +    hr = get_typeinfo(IWshCollection_tid, ppTInfo);
> +    ITypeInfo_AddRef(*ppTInfo);
> +    return hr;

You'd need to check for error first, but the AddRef should really be in
get_typeinfo().

-- 
Alexandre Julliard
julliard(a)winehq.org

wshom.ocx: Properly update typeinfo refcount

[Nikolay Sivov]

On 11/28/2012 17:43, Alexandre Julliard wrote:
> Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> > @@ -137,8 +137,13 @@ static HRESULT WINAPI WshCollection_GetTypeInfoCount(IWshCollection *iface, UINT
> >   static HRESULT WINAPI WshCollection_GetTypeInfo(IWshCollection *iface, UINT iTInfo, LCID lcid, ITypeInfo **ppTInfo)
> >   {
> >       WshCollection *This = impl_from_IWshCollection(iface);
> > +    HRESULT hr;
> > +
> >       TRACE("(%p)->(%u %u %p)\n", This, iTInfo, lcid, ppTInfo);
> > -    return get_typeinfo(IWshCollection_tid, ppTInfo);
> > +
> > +    hr = get_typeinfo(IWshCollection_tid, ppTInfo);
> > +    ITypeInfo_AddRef(*ppTInfo);
> > +    return hr;
> > You'd need to check for error first, but the AddRef should really be in
> > get_typeinfo().
> > I agree about error part. The reason why AddRef shouldn't be in 
> > get_typeinfo() is to avoid pointless AddRef/Release when pointer does 
> > not go out, and the only case when it's returned to caller is this 
> > GetTypeInfo() method.

wshom.ocx: Properly update typeinfo refcount

[Alexandre Julliard]

Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> On 11/28/2012 17:43, Alexandre Julliard wrote:
> > Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> > > @@ -137,8 +137,13 @@ static HRESULT WINAPI WshCollection_GetTypeInfoCount(IWshCollection *iface, UINT
> > >   static HRESULT WINAPI WshCollection_GetTypeInfo(IWshCollection *iface, UINT iTInfo, LCID lcid, ITypeInfo **ppTInfo)
> > >   {
> > >       WshCollection *This = impl_from_IWshCollection(iface);
> > > +    HRESULT hr;
> > > +
> > >       TRACE("(%p)->(%u %u %p)\n", This, iTInfo, lcid, ppTInfo);
> > > -    return get_typeinfo(IWshCollection_tid, ppTInfo);
> > > +
> > > +    hr = get_typeinfo(IWshCollection_tid, ppTInfo);
> > > +    ITypeInfo_AddRef(*ppTInfo);
> > > +    return hr;
> > > You'd need to check for error first, but the AddRef should really be in
> > > get_typeinfo().
> > > I agree about error part. The reason why AddRef shouldn't be in
> > > get_typeinfo() is to avoid pointless AddRef/Release when pointer does
> > > not go out, and the only case when it's returned to caller is this
> > > GetTypeInfo() method.

Internal functions should follow COM conventions too, otherwise we'll
constantly run into reference counting bugs. AddRef is cheap.

-- 
Alexandre Julliard
julliard(a)winehq.org

wshom.ocx: Properly update typeinfo refcount

[Nikolay Sivov]

On 11/28/2012 18:19, Alexandre Julliard wrote:
> Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> > On 11/28/2012 17:43, Alexandre Julliard wrote:
> > > Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> > > > @@ -137,8 +137,13 @@ static HRESULT WINAPI WshCollection_GetTypeInfoCount(IWshCollection *iface, UINT
> > > >    static HRESULT WINAPI WshCollection_GetTypeInfo(IWshCollection *iface, UINT iTInfo, LCID lcid, ITypeInfo **ppTInfo)
> > > >    {
> > > >        WshCollection *This = impl_from_IWshCollection(iface);
> > > > +    HRESULT hr;
> > > > +
> > > >        TRACE("(%p)->(%u %u %p)\n", This, iTInfo, lcid, ppTInfo);
> > > > -    return get_typeinfo(IWshCollection_tid, ppTInfo);
> > > > +
> > > > +    hr = get_typeinfo(IWshCollection_tid, ppTInfo);
> > > > +    ITypeInfo_AddRef(*ppTInfo);
> > > > +    return hr;
> > > > You'd need to check for error first, but the AddRef should really be in
> > > > get_typeinfo().
> > > > I agree about error part. The reason why AddRef shouldn't be in
> > > > get_typeinfo() is to avoid pointless AddRef/Release when pointer does
> > > > not go out, and the only case when it's returned to caller is this
> > > > GetTypeInfo() method.
> > > > Internal functions should follow COM conventions too, otherwise we'll
> > > > constantly run into reference counting bugs. AddRef is cheap.
> > > > Okay, will do that. Does it mean you want a similar change for 
> > > > mshtml/msxml3 too? For msxml3 it means that 
> > > > http://source.winehq.org/git/wine.git/commitdiff/6e566ce6c24186174ab9c5370a3... 
> > > > should be reverted for example.

wshom.ocx: Properly update typeinfo refcount

[Alexandre Julliard]

Nikolay Sivov <nsivov(a)codeweavers.com> writes:
> Okay, will do that. Does it mean you want a similar change for
> mshtml/msxml3 too? For msxml3 it means that
> http://source.winehq.org/git/wine.git/commitdiff/6e566ce6c24186174ab9c5370a3...
> should be reverted for example.

Yes, that should most likely be reverted.

-- 
Alexandre Julliard
julliard(a)winehq.org