Mike Hearn mike@theoretic.com writes:
ChangeLog: When providing a TypeInfo object, inc/dec the refcount so that programs don't call GetContainingTypeLib and crash as the container has been destroyed.
This doesn't look right, you increment the refcount on every GetTypeInfo but only decrement when the typeinfo object is destroyed.
This doesn't look right, you increment the refcount on every GetTypeInfo but only decrement when the typeinfo object is destroyed.
Well, unless you can call GetTypeInfoOfGuid twice and get two pointers to the same interface (maybe you can, i don't really know but that would seem unusual), I don't understand why this is wrong. When somebody calls GetTypeInfoOfGuid they get an interface, so at some point they should release it, which will in turn release the reference to the typelib. Without this patch, an app did a GetTypeInfo, released the typelib object and then tried to get the container from ITypeInfo which obviously crashed when it tried to do an AddRef on the destroyed object.
Otherwise I don't really understand how this should work, unless you manually add a reference for every type info object when the typelib is first constructed, then manually release also for every object. That would seem a bit odd though, to me.
Could you or somebody else explain how this should work?
thanks -mike
I would expect you to addref the typelib when the ITypeInfo pointer is created, and then when it is destroyed to release it.
In the object that implements the ITypeInfo reference, you should have a pTypeLib->Release() in the destructor and a pTypeLib->AddRef() somewhere in the code that creates the ITypeInfo implementing object. If, for instance, the ITypeLib pointer is passed to the constructor of this object, then the constructor should look something like...
m_pTypeLib = pTypeLib; pTypeLib->AddRef(); ...
and the destructor should look like
m_pTypeLib->Release(); ...
Kelly
----- Original Message ----- From: "Mike Hearn" mike@theoretic.com To: wine-devel@winehq.com Sent: Monday, June 16, 2003 2:28 PM Subject: Re: TypeLib containment refcounting
This doesn't look right, you increment the refcount on every GetTypeInfo but only decrement when the typeinfo object is destroyed.
Well, unless you can call GetTypeInfoOfGuid twice and get two pointers to the same interface (maybe you can, i don't really know but that would seem unusual), I don't understand why this is wrong. When somebody calls GetTypeInfoOfGuid they get an interface, so at some point they should release it, which will in turn release the reference to the typelib. Without this patch, an app did a GetTypeInfo, released the typelib object and then tried to get the container from ITypeInfo which obviously crashed when it tried to do an AddRef on the destroyed object.
Otherwise I don't really understand how this should work, unless you manually add a reference for every type info object when the typelib is first constructed, then manually release also for every object. That would seem a bit odd though, to me.
Could you or somebody else explain how this should work?
thanks -mike
On Mon, 16 Jun 2003 14:43:25 -0500, Sir Kelly Leahy scribed thus:
I would expect you to addref the typelib when the ITypeInfo pointer is created, and then when it is destroyed to release it.
I believe that's what I'm doing. Maybe not.
I think what is actually going on, is that this method returns a pointer to an internal interface that's always present. There is no construction as such. From MSDN:
HRESULT GetTypeInfoOfGuid( REFGUID guid, ITypeInfo FAR* FAR* ppTinfo ); Retrieves the type description that corresponds to the specified GUID.
This confuses me. Does the caller need to release the interface? The page doesn't mention refcounting at all.
"Retrieves" is somewhat fuzzy. The fact that it's a pointer to a pointer suggests that maybe you get a pointer to an internal interface and you shouldn't do any refcounting on it.
In the object that implements the ITypeInfo reference, you should have a pTypeLib->Release() in the destructor and a pTypeLib->AddRef() somewhere in the code that creates the ITypeInfo implementing object.
Well that's C++ stuff. Remember that all this is implemented in pure C. The relevant code searches a linked list for a ITypeInfoImpl struct, which stores all the data and the COM Vtable. It then casts it to an ITypeInfo and incs the refcount:
*ppTInfo = (ITypeInfo*)pTypeInfo; ITypeInfo_AddRef(*ppTInfo);
So, what you actually get out of this method is a pointer to an internal data structure rather than a constructed object....
----- Original Message ----- From: "Mike Hearn" mike@theoretic.com To: wine-devel@winehq.com Sent: Monday, June 16, 2003 3:16 PM Subject: Re: TypeLib containment refcounting
On Mon, 16 Jun 2003 14:43:25 -0500, Sir Kelly Leahy scribed thus:
I would expect you to addref the typelib when the ITypeInfo pointer is created, and then when it is destroyed to release it.
I believe that's what I'm doing. Maybe not.
I think what is actually going on, is that this method returns a pointer to an internal interface that's always present. There is no construction as such. From MSDN:
HRESULT GetTypeInfoOfGuid( REFGUID guid, ITypeInfo FAR* FAR* ppTinfo ); Retrieves the type description that corresponds to the specified GUID.
This confuses me. Does the caller need to release the interface? The page doesn't mention refcounting at all.
"Retrieves" is somewhat fuzzy. The fact that it's a pointer to a pointer suggests that maybe you get a pointer to an internal interface and you shouldn't do any refcounting on it.
The caller should release the object when they're done with it. I guess what I was saying is that there's a few ways this can be done.
One way is for ITypeInfo_AddRef() to addref the type library object that contains it (i.e. add one to the refcount on the ITypeLib struct in C terms). It can either do this every time (in which case, ITypeInfo_Release() should release the type library object each time) or it can do it only when the refcount is zero on entry to ITypeInfo_AddRef() (in which case ITypeInfo_Release() should only Release() the type library object when the result of decrementing the refcount is zero in ITypeInfo_Release()).
Another way is the addref the type library when the ITypeInfo wherever a ITypeInfo interface is initially created (like in GetTypeInfoOfGuid) but I think this is more dangerous, since then you have to have some way for this refcount to go away, and ITypeInfo_Release() doesn't know who created it, just that it was created. If you are careful enough to always put an addref on the library when you create a new ITypeInfo pointer (GetTypeInfoOfGuid, GetTypeInfo, IDispatch::GetTypeInfo, etc.) but this seems like a unwieldy task to guarantee that the code is right in all of these places. It's much easier to make the ITypeInfo_AddRef() and ITypeInfo_Release() call ITypeLib_AddRef() and ITypeLib_Release() on the containing ITypeLib implementation when appropriate.
In the object that implements the ITypeInfo reference, you should have a pTypeLib->Release() in the destructor and a pTypeLib->AddRef() somewhere
in
the code that creates the ITypeInfo implementing object.
Well that's C++ stuff. Remember that all this is implemented in pure C. The relevant code searches a linked list for a ITypeInfoImpl struct, which stores all the data and the COM Vtable. It then casts it to an ITypeInfo and incs the refcount:
*ppTInfo = (ITypeInfo*)pTypeInfo; ITypeInfo_AddRef(*ppTInfo);So, what you actually get out of this method is a pointer to an internal data structure rather than a constructed object....
Sorry... I didn't have the source handy to look at (RMAing my linux box HDD right now!)
As far as C++ stuff vs. C stuff, in essence the pointer to the data structure (including the VMT) is simply a C++ style class without the C++, and with some special requirements... You can just replace my p->method() with OBJECT_method(p) if you like to make it look like what I've seen from wine. I'm still pretty new to the wine source, so it may not be like this everywhere, I just don't have the time to look.
Kelly
Another way is the addref the type library when the ITypeInfo wherever a ITypeInfo interface is initially created (like in GetTypeInfoOfGuid) but I think this is more dangerous, since then you have to have some way for this refcount to go away, and ITypeInfo_Release() doesn't know who created it, just that it was created. If you are careful enough to always put an addref on the library when you create a new ITypeInfo pointer (GetTypeInfoOfGuid, GetTypeInfo, IDispatch::GetTypeInfo, etc.) but this seems like a unwieldy task to guarantee that the code is right in all of these places. It's much easier to make the ITypeInfo_AddRef() and ITypeInfo_Release() call ITypeLib_AddRef() and ITypeLib_Release() on the containing ITypeLib implementation when appropriate.
Yes, this is true. I hadn't considered that, it would be better to put the TypeLib addref in the TypeInfo addref. This feels a bit odd though, it would lead to the TypeLib having a reference count that was "too high" (though I suppose it doesn't really matter) if the receiver did their own AddRef on the TypeInfo, as the refs would propogate up. Still, the actual value of the refcount doesn't matter.
As far as C++ stuff vs. C stuff, in essence the pointer to the data structure (including the VMT) is simply a C++ style class without the C++, and with some special requirements... You can just replace my p->method() with OBJECT_method(p) if you like to make it look like what I've seen from wine. I'm still pretty new to the wine source, so it may not be like this everywhere, I just don't have the time to look.
True, what I meant was that there is no object constructor, unless you count the code where the struct is allocated and appended to the end of the linked list.
thanks -mike
man, 16.06.2003 kl. 21.28 skrev Mike Hearn:
This doesn't look right, you increment the refcount on every GetTypeInfo but only decrement when the typeinfo object is destroyed.
Well, unless you can call GetTypeInfoOfGuid twice and get two pointers to the same interface (maybe you can, i don't really know but that would seem unusual), I don't understand why this is wrong.
That is exactly what you can in the current typelib implementation. The pointers will be the same, too. Whether you can in the real MS implementation I'm not sure, but shouldn't matter, since the current Wine implementation is what you're dealing with.
Well, unless you can call GetTypeInfoOfGuid twice and get two pointers to the same interface (maybe you can, i don't really know but that would seem unusual), I don't understand why this is wrong.
That is exactly what you can in the current typelib implementation. The pointers will be the same, too. Whether you can in the real MS implementation I'm not sure, but shouldn't matter, since the current Wine implementation is what you're dealing with.
Even so, as far as the users of these interfaces are concerned, they have to be properly reference counted which the app I'm dealing with, which is MS native code appears to do.
So, is the problem here that I would be leaking references?
tir, 17.06.2003 kl. 14.55 skrev Mike Hearn:
Well, unless you can call GetTypeInfoOfGuid twice and get two pointers to the same interface (maybe you can, i don't really know but that would seem unusual), I don't understand why this is wrong.
That is exactly what you can in the current typelib implementation. The pointers will be the same, too. Whether you can in the real MS implementation I'm not sure, but shouldn't matter, since the current Wine implementation is what you're dealing with.
Even so, as far as the users of these interfaces are concerned, they have to be properly reference counted which the app I'm dealing with, which is MS native code appears to do.
So, is the problem here that I would be leaking references?
Yes, that's what I think Alexandre's point was. An app can use GetTypeInfoOfGuid twice, the same typeinfo object would be returned in both calls, you addref the typelib twice, the app can then release that typeinfo twice, but with the result that the typeinfo is only destroyed once, so the typelib would be only released once with your patch. So, either addref the typelib only if the app does not already have references to the typeinfo, or easier, always release the typelib in the typeinfo's release whether or not it's the last reference (I'm not sure you'd ever get to the last reference anyway because the typelib structure keeps internal references to typeinfo objects too).
Yes, that's what I think Alexandre's point was. An app can use GetTypeInfoOfGuid twice, the same typeinfo object would be returned in both calls, you addref the typelib twice, the app can then release that typeinfo twice, but with the result that the typeinfo is only destroyed once, so the typelib would be only released once with your patch.
Ah, I understand now. Thanks for explaining this to me.
So, either addref the typelib only if the app does not already have references to the typeinfo, or easier, always release the typelib in the typeinfo's release whether or not it's the last reference (I'm not sure you'd ever get to the last reference anyway because the typelib structure keeps internal references to typeinfo objects too).
Yeah, I think moving the TypeLib release to the TypeInfo release method would be the best option here. I will try and resubmit the patch in the next couple of days.
thanks -mike
Mike...
First of all, sorry but I don't have a linux box right now (bad HDD) so this is the best I can do.
My code below will cause an AddRef only on the first ITypeInfo pointer created (for each ITypeInfo), and a release on the last ITypeInfo pointer released. For each typeinfo struct in the typelib, you will get one reference if a client has any references to that object. As soon as all client references to ITypeInfo's supported by the typeinfo in the library are released, the refcount of the library will go to zero. Of course, you should not addref the library (directly) in GetTypeInfoOfGuid or any other place that doesn't return a ITypeLib pointer.
Here's some source (with changes inline) from the typelib.c in 5/8/2003 source on source.winehq.com
3795 /* ITypeInfo::AddRef 3796 */ 3797 static ULONG WINAPI ITypeInfo_fnAddRef( ITypeInfo2 *iface) 3798 { 3799 ICOM_THIS( ITypeInfoImpl, iface); 3800
here, I'd recommend inserting: if(This->ref == 0) ITypeLib2_AddRef((ITypeLib2 *)This->pTypeLib);
3801 ++(This->ref); 3802 3803 TRACE("(%p)->ref is %u\n",This, This->ref); 3804 return This->ref; 3805 } 3806 3807 /* ITypeInfo::Release 3808 */ 3809 static ULONG WINAPI ITypeInfo_fnRelease( ITypeInfo2 *iface) 3810 { 3811 ICOM_THIS( ITypeInfoImpl, iface); 3812 3813 --(This->ref);
and here, I'd recommend inserting if(This->ref == 0) ITypeLib2_Release((ITypeLib2 *)This->pTypeLib);
3814 3815 TRACE("(%p)->(%u)\n",This, This->ref); 3816 3817 if (!This->ref) 3818 { 3819 FIXME("destroy child objects\n"); 3820 3821 TRACE("destroying ITypeInfo(%p)\n",This); 3822 if (This->Name) 3823 { 3824 SysFreeString(This->Name); 3825 This->Name = 0; 3826 } 3827 3828 if (This->DocString) 3829 { 3830 SysFreeString(This->DocString); 3831 This->DocString = 0; 3832 } 3833 3834 if (This->next) 3835 { 3836 ITypeInfo_Release((ITypeInfo*)This->next); 3837 } 3838 3839 HeapFree(GetProcessHeap(),0,This); 3840 return 0; 3841 } 3842 return This->ref; 3843 }
-
Alexandre Julliard -
Kelly Leahy -
Mike Hearn -
Mike Hearn -
Ove Kaaven