Re: 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?