Re: implement DllCanUnloadNow for dmusic

[Robert Shearman]

James Hawkins wrote:
> Changelog
> - properly implement DllCanUnloadNow ref counting

Sorry, still isn't correct. Please see my comments below.
> Index: dlls/dmusic/buffer.c
>
> RCS file: /home/wine/wine/dlls/dmusic/buffer.c,v
> retrieving revision 1.8
> diff -u -r1.8 buffer.c
> --- dlls/dmusic/buffer.c	6 Sep 2004 21:34:25 -0000	1.8
> +++ dlls/dmusic/buffer.c	4 Dec 2004 20:52:16 -0000
> @@ -38,6 +38,9 @@
> ULONG WINAPI IDirectMusicBufferImpl_AddRef (LPDIRECTMUSICBUFFER iface) {
>    IDirectMusicBufferImpl *This = (IDirectMusicBufferImpl *)iface;
>    TRACE("(%p): AddRef from %ld\n", This, This->ref);
> -
> - DMUSIC_LockModule();
> - return ++(This->ref);
>
> }
>
> @@ -45,9 +48,13 @@
>    IDirectMusicBufferImpl *This = (IDirectMusicBufferImpl *)iface;
>    ULONG ref = --This->ref;
>    TRACE("(%p): ReleaseRef to %ld\n", This, This->ref);
> - if (ref == 0) {
> HeapFree(GetProcessHeap(), 0, This);
> }
> -
> - DMUSIC_UnlockModule();
> - return ref;
>
> }
>
> @@ -160,6 +167,8 @@
>    }
>    dmbuff->lpVtbl = &DirectMusicBuffer_Vtbl;
>    dmbuff->ref = 0; /* will be inited by QueryInterface */
> -
> -
>
> DMUSIC_LockModule();
>
> return IDirectMusicBufferImpl_QueryInterface ((LPDIRECTMUSICBUFFER)dmbuff, lpcGUID, ppobj);
>
> }

Ok, think about this sequence of events that should leave the DLL ref 
count at 0:
1. Create an IDirectMusicBufferImpl object.
1a. Calls LockModule.
1b. Calls QueryInterface which calls LockModule.
2. Call Release, which calls LockModule.

The solution is to either remove the LockModule from the constructor, or 
to remove the LockModule from the AddRef and only call UnlockModule in 
the destructor part of Release. This issue is also present in the other 
files in your patch (not quoted here).

I should have thought about this before when I commented on your last 
patch. I guess I need to put some tests together for this feature.

Rob

implement DllCanUnloadNow for dmusic

[James Hawkins]

On Sat, 04 Dec 2004 15:13:47 -0600, Robert Shearman  wrote:
> James Hawkins wrote:
> > Changelog
> > - properly implement DllCanUnloadNow ref counting
>
> Sorry, still isn't correct. Please see my comments below.
> > Index: dlls/dmusic/buffer.c
> >
> > RCS file: /home/wine/wine/dlls/dmusic/buffer.c,v
> > retrieving revision 1.8
> > diff -u -r1.8 buffer.c
> > --- dlls/dmusic/buffer.c       6 Sep 2004 21:34:25 -0000       1.8
> > +++ dlls/dmusic/buffer.c       4 Dec 2004 20:52:16 -0000
> > @@ -38,6 +38,9 @@
> > ULONG WINAPI IDirectMusicBufferImpl_AddRef (LPDIRECTMUSICBUFFER iface) {
> >       IDirectMusicBufferImpl *This = (IDirectMusicBufferImpl *)iface;
> >       TRACE("(%p): AddRef from %ld\n", This, This->ref);
> > -
> > -
> >  DMUSIC_LockModule();
> > -
> > return ++(This->ref);
> >
> > }
> >
> > @@ -45,9 +48,13 @@
> >       IDirectMusicBufferImpl *This = (IDirectMusicBufferImpl *)iface;
> >       ULONG ref = --This->ref;
> >       TRACE("(%p): ReleaseRef to %ld\n", This, This->ref);
> > -
> > if (ref == 0) {
> >         HeapFree(GetProcessHeap(), 0, This);
> > }
> > -
> > -
> >  DMUSIC_UnlockModule();
> > -
> > return ref;
> >
> > }
> >
> > @@ -160,6 +167,8 @@
> >       }
> >       dmbuff->lpVtbl = &DirectMusicBuffer_Vtbl;
> >       dmbuff->ref = 0; /* will be inited by QueryInterface */
> > -
> > -
> >  DMUSIC_LockModule();
> >
> > return IDirectMusicBufferImpl_QueryInterface ((LPDIRECTMUSICBUFFER)dmbuff, lpcGUID, ppobj);
> >
> > }
>
> Ok, think about this sequence of events that should leave the DLL ref
> count at 0:
> - Create an IDirectMusicBufferImpl object.
>
> 1a. Calls LockModule.
> 1b. Calls QueryInterface which calls LockModule.
> 2. Call Release, which calls LockModule.
>
> The solution is to either remove the LockModule from the constructor, or
> to remove the LockModule from the AddRef and only call UnlockModule in
> the destructor part of Release. This issue is also present in the other
> files in your patch (not quoted here).
>
> I should have thought about this before when I commented on your last
> patch. I guess I need to put some tests together for this feature.
>
> Rob

MSDN about QueryInterface:
"Returns a pointer to a specified interface on an object to which a
client currently holds an interface pointer. This function must call
IUnknown::AddRef on the pointer it returns."

QueryInterface should be calling AddRef, so if any constructor calls
QueryInterface (I'm guessing without looking that they all do) then
there shouldn't be a call to LockModule in that constructor.  Is that
about right?

implement DllCanUnloadNow for dmusic

[Robert Shearman]

James Hawkins wrote:
> On Sat, 04 Dec 2004 15:13:47 -0600, Robert Shearman  wrote:
> > Ok, think about this sequence of events that should leave the DLL ref
> > count at 0:
> > - Create an IDirectMusicBufferImpl object.
> >
> > 1a. Calls LockModule.
> > 1b. Calls QueryInterface which calls LockModule.
> > 2. Call Release, which calls LockModule.
> >
> > The solution is to either remove the LockModule from the constructor, or
> > to remove the LockModule from the AddRef and only call UnlockModule in
> > the destructor part of Release. This issue is also present in the other
> > files in your patch (not quoted here).
> >
> > I should have thought about this before when I commented on your last
> > patch. I guess I need to put some tests together for this feature.
> >
> > Rob
>
> MSDN about QueryInterface:
> "Returns a pointer to a specified interface on an object to which a
> client currently holds an interface pointer. This function must call
> IUnknown::AddRef on the pointer it returns."
>
> QueryInterface should be calling AddRef, so if any constructor calls
> QueryInterface (I'm guessing without looking that they all do) then
> there shouldn't be a call to LockModule in that constructor.  Is that
> about right?

That is correct.

Rob