attemp at implementing DllCanUnload now for dcompos

[James Hawkins]

Hey all,

It would really help me out if you could look over this patch and see
if I've incorrectly implemented ref-counting and DllCanUnloadNow for
dcompos.  I used suggestions from Rob and a sample implementation
written by him.  The only concern I have is that I'm not sure if all
of these classes are non-heap based.

-- 
James Hawkins

[Robert Shearman]

James Hawkins wrote:
> Hey all,
>
> It would really help me out if you could look over this patch and see
> if I've incorrectly implemented ref-counting and DllCanUnloadNow for
> dcompos.  I used suggestions from Rob and a sample implementation
> written by him.  The only concern I have is that I'm not sure if all
> of these classes are non-heap based.
>
> ------------------------------------------------------------------------
>
> Index: dlls/dmcompos/chordmap.c
> ===================================================================
> RCS file: /home/wine/wine/dlls/dmcompos/chordmap.c,v
> retrieving revision 1.7
> diff -u -r1.7 chordmap.c
> --- dlls/dmcompos/chordmap.c	23 Aug 2004 19:39:57 -0000	1.7
> +++ dlls/dmcompos/chordmap.c	1 Dec 2004 21:15:24 -0000
> @@ -54,18 +54,20 @@
>
> ULONG WINAPI IDirectMusicChordMapImpl_IUnknown_AddRef (LPUNKNOWN iface) {
>   ICOM_THIS_MULTI(IDirectMusicChordMapImpl, UnknownVtbl, iface);
> -	TRACE("(%p): AddRef from %ld\n", This, This->ref);
> -	return ++(This->ref);
> +	TRACE("(%p): AddRef\n", This);
> +	
> +	LockModule();
> +	
> +	return 2; /* non-heap based object */
> }
>
> ULONG WINAPI IDirectMusicChordMapImpl_IUnknown_Release (LPUNKNOWN iface) {
>   ICOM_THIS_MULTI(IDirectMusicChordMapImpl, UnknownVtbl, iface);
> -	ULONG ref = --This->ref;
> -	TRACE("(%p): ReleaseRef to %ld\n", This, This->ref);
> -	if (ref == 0) {
> -		HeapFree(GetProcessHeap(), 0, This);
> -	}
> -	return ref;
> +	TRACE("(%p): ReleaseRef\n", This);
> +	
> +	UnlockModule();
> +	
> +	return 1; /* non-heap based object */
> }

No, this isn't a non-heap based object. The code you removed actually 
frees the object from the heap and further down in the file the 
constructor "DMUSIC_CreateDirectMusicChordMapImpl" allocates it on the 
heap. A sure sign of something being heap-based instead of non-heap 
based is that it does stuff when its ref count goes to zero, as in the 
case above.

Rob

[James Hawkins]

On Wed, 01 Dec 2004 17:44:55 -0600, Robert Shearman <rob(a)codeweavers.com> wrote:
> James Hawkins wrote:
> > Hey all,
> >
> > It would really help me out if you could look over this patch and see
> > if I've incorrectly implemented ref-counting and DllCanUnloadNow for
> > dcompos.  I used suggestions from Rob and a sample implementation
> > written by him.  The only concern I have is that I'm not sure if all
> > of these classes are non-heap based.
> >
> > ------------------------------------------------------------------------
> >
> > Index: dlls/dmcompos/chordmap.c
> > ===================================================================
> > RCS file: /home/wine/wine/dlls/dmcompos/chordmap.c,v
> > retrieving revision 1.7
> > diff -u -r1.7 chordmap.c
> > --- dlls/dmcompos/chordmap.c   23 Aug 2004 19:39:57 -0000      1.7
> > +++ dlls/dmcompos/chordmap.c   1 Dec 2004 21:15:24 -0000
> > @@ -54,18 +54,20 @@
> >
> > ULONG WINAPI IDirectMusicChordMapImpl_IUnknown_AddRef (LPUNKNOWN iface) {
> >       ICOM_THIS_MULTI(IDirectMusicChordMapImpl, UnknownVtbl, iface);
> > -      TRACE("(%p): AddRef from %ld\n", This, This->ref);
> > -      return ++(This->ref);
> > +      TRACE("(%p): AddRef\n", This);
> > +
> > +      LockModule();
> > +
> > +      return 2; /* non-heap based object */
> > }
> >
> > ULONG WINAPI IDirectMusicChordMapImpl_IUnknown_Release (LPUNKNOWN iface) {
> >       ICOM_THIS_MULTI(IDirectMusicChordMapImpl, UnknownVtbl, iface);
> > -      ULONG ref = --This->ref;
> > -      TRACE("(%p): ReleaseRef to %ld\n", This, This->ref);
> > -      if (ref == 0) {
> > -              HeapFree(GetProcessHeap(), 0, This);
> > -      }
> > -      return ref;
> > +      TRACE("(%p): ReleaseRef\n", This);
> > +
> > +      UnlockModule();
> > +
> > +      return 1; /* non-heap based object */
> > }
>
> No, this isn't a non-heap based object. The code you removed actually
> frees the object from the heap and further down in the file the
> constructor "DMUSIC_CreateDirectMusicChordMapImpl" allocates it on the
> heap. A sure sign of something being heap-based instead of non-heap
> based is that it does stuff when its ref count goes to zero, as in the
> case above.
>
> Rob

Here is the second go round at DllCanUnloadNow.  I changed LockModule
and UnlockModule to return the changed ref count.  I use this to
implement AddRef and Release for chordmaptrack.  This is just an idea
I had, so let me know if this works.


-- 
James Hawkins

[Mike Hearn]

On Wed, 01 Dec 2004 22:13:23 -0500, James Hawkins wrote:
> ULONG WINAPI IDirectMusicChordMapImpl_IUnknown_AddRef (LPUNKNOWN iface) {
>   ICOM_THIS_MULTI(IDirectMusicChordMapImpl, UnknownVtbl, iface);
> -	TRACE("(%p): AddRef from %ld\n", This, This->ref);
> -	return ++(This->ref);
> +	TRACE("(%p): AddRef\n", This);
> +	
> +	LockModule();
> +	
> +	return 2; /* non-heap based object */
>  }

What makes you think this is a non-heap object? Almost all COM objects are
heap based except classfactories and process-wide singletons (internal to
COM typically). 

In particular this one maintains its own ref count. You shouldn't be
modifying the way objects to refcounting, you should just be adding the
LockModule/UnlockModule calls.