From c3927d7e156e13ed30cc7c06a2c27ff7fb3affa6 Mon Sep 17 00:00:00 2001 From: Timo Kreuzer Date: Mon, 23 Mar 2009 23:49:00 +0000 Subject: [PATCH] - Don't delete an object that has a shared reference! - Implement DC_vSelectSurface, that dereferences the old SURFACE and references the new. Use it instead of doing it manually. - Select NULL surface when doing cleanup. - Go back to ASSERT in GDIOBJ_ShareUnlockObjByPtr. Should (hopefully) not be hit anymore. - Add additional functions for tracing shared locks in gdidbg.c. - Add debugprints when leaking objects on process cleanup. svn path=/trunk/; revision=40196 --- reactos/subsystems/win32/win32k/include/dc.h | 14 ++++++++ .../subsystems/win32/win32k/include/gdiobj.h | 12 +++---- .../subsystems/win32/win32k/objects/bitmaps.c | 23 ++++++------ .../subsystems/win32/win32k/objects/dclife.c | 5 +-- .../subsystems/win32/win32k/objects/gdidbg.c | 19 +++++++++- .../subsystems/win32/win32k/objects/gdiobj.c | 36 +++++++++++++++---- 6 files changed, 83 insertions(+), 26 deletions(-) diff --git a/reactos/subsystems/win32/win32k/include/dc.h b/reactos/subsystems/win32/win32k/include/dc.h index 39c4c94a5ee..97200ba59ff 100644 --- a/reactos/subsystems/win32/win32k/include/dc.h +++ b/reactos/subsystems/win32/win32k/include/dc.h @@ -278,6 +278,20 @@ INT FASTCALL IntGdiGetDeviceCaps(PDC,INT); extern PPDEVOBJ pPrimarySurface; +VOID +FORCEINLINE +DC_vSelectSurface(PDC pdc, PSURFACE psurfNew) +{ + PSURFACE psurfOld = pdc->dclevel.pSurface; + if (psurfOld) + SURFACE_ShareUnlockSurface(psurfOld); + + if (psurfNew) + GDIOBJ_IncrementShareCount((POBJ)psurfNew); + + pdc->dclevel.pSurface = psurfNew; +} + BOOL FASTCALL IntPrepareDriverIfNeeded(); extern PDEVOBJ PrimarySurface; diff --git a/reactos/subsystems/win32/win32k/include/gdiobj.h b/reactos/subsystems/win32/win32k/include/gdiobj.h index b3395a18865..6bbbe1e8ee4 100644 --- a/reactos/subsystems/win32/win32k/include/gdiobj.h +++ b/reactos/subsystems/win32/win32k/include/gdiobj.h @@ -104,16 +104,13 @@ FORCEINLINE GDIOBJ_ShareUnlockObjByPtr(POBJ Object) { INT cLocks = InterlockedDecrement((PLONG)&Object->ulShareCount); -// ASSERT(cLocks >= 0); - if (cLocks < 0) - { - DbgPrint("Unlocked object %p, that was not locked!\n", Object->hHmgr); - KdSystemDebugControl(TAG('R', 'o', 's', 'D'), NULL, 20, NULL, 0, NULL, KernelMode); - InterlockedIncrement((PLONG)&Object->ulShareCount); - } + ASSERT(cLocks >= 0); return cLocks; } +#ifdef GDI_DEBUG +ULONG FASTCALL GDIOBJ_IncrementShareCount(POBJ Object); +#else ULONG FORCEINLINE GDIOBJ_IncrementShareCount(POBJ Object) @@ -122,5 +119,6 @@ GDIOBJ_IncrementShareCount(POBJ Object) ASSERT(cLocks >= 1); return cLocks; } +#endif #endif diff --git a/reactos/subsystems/win32/win32k/objects/bitmaps.c b/reactos/subsystems/win32/win32k/objects/bitmaps.c index dd71fc8092d..201532c4282 100644 --- a/reactos/subsystems/win32/win32k/objects/bitmaps.c +++ b/reactos/subsystems/win32/win32k/objects/bitmaps.c @@ -932,7 +932,7 @@ NtGdiSelectBitmap( PDC pDC; PDC_ATTR pdcattr; HBITMAP hOrgBmp; - PSURFACE psurfBmp; + PSURFACE psurfBmp, psurfOld; HRGN hVisRgn; BOOLEAN bFailed; PBRUSH pbrush; @@ -961,23 +961,26 @@ NtGdiSelectBitmap( return NULL; } + /* Get the handle for the old bitmap */ + psurfOld = pDC->dclevel.pSurface; + hOrgBmp = psurfOld ? psurfOld->BaseObject.hHmgr : NULL; + + /* FIXME: ros hack */ hOrgBmp = pDC->rosdc.hBitmap; pDC->rosdc.hBitmap = hBmp; - /* Release the old bitmap */ - if (pDC->dclevel.pSurface) - SURFACE_ShareUnlockSurface(pDC->dclevel.pSurface); - + /* Release the old bitmap, reference the new */ + DC_vSelectSurface(pDC, psurfBmp); + // If Info DC this is zero and pSurface is moved to DC->pSurfInfo. - pDC->dclevel.pSurface = psurfBmp; psurfBmp->hDC = hDC; // if we're working with a DIB, get the palette // [fixme: only create if the selected palette is null] if (psurfBmp->hSecure) { -// pDC->rosdcbitsPerPixel = psurfBmp->dib->dsBmih.biBitCount; ??? +// pDC->rosdc.bitsPerPixel = psurfBmp->dib->dsBmih.biBitCount; ??? pDC->rosdc.bitsPerPixel = BitsPerFormat(psurfBmp->SurfObj.iBitmapFormat); } else @@ -985,16 +988,16 @@ NtGdiSelectBitmap( pDC->rosdc.bitsPerPixel = BitsPerFormat(psurfBmp->SurfObj.iBitmapFormat); } + /* FIXME; improve by using a region without a handle and selecting it */ hVisRgn = NtGdiCreateRectRgn(0, 0, psurfBmp->SurfObj.sizlBitmap.cx, psurfBmp->SurfObj.sizlBitmap.cy); - /* Keep a shared reference on the bitmap */ - GDIOBJ_IncrementShareCount((POBJ)psurfBmp); + /* Release the exclusive lock */ SURFACE_UnlockSurface(psurfBmp); - /* Regenerate the XLATEOBJs. */ + /* Regenerate the XLATEOBJs. (hack!) */ pbrush = BRUSH_LockBrush(pdcattr->hbrush); if (pbrush) { diff --git a/reactos/subsystems/win32/win32k/objects/dclife.c b/reactos/subsystems/win32/win32k/objects/dclife.c index 9a3fadb120a..bc66e743ee3 100644 --- a/reactos/subsystems/win32/win32k/objects/dclife.c +++ b/reactos/subsystems/win32/win32k/objects/dclife.c @@ -140,6 +140,7 @@ DC_Cleanup(PVOID ObjectBody) PDC pDC = (PDC)ObjectBody; if (pDC->rosdc.DriverName.Buffer) ExFreePoolWithTag(pDC->rosdc.DriverName.Buffer, TAG_DC); + DC_vSelectSurface(pDC, NULL); return TRUE; } @@ -264,7 +265,7 @@ IntGdiCreateDC(PUNICODE_STRING Driver, pdc->dhpdev = PrimarySurface.hPDev; if(pUMdhpdev) pUMdhpdev = pdc->dhpdev; // set DHPDEV for device. pdc->ppdev = (PVOID)&PrimarySurface; - pdc->rosdc.hBitmap = (HBITMAP)PrimarySurface.pSurface; + pdc->rosdc.hBitmap = (HBITMAP)PrimarySurface.pSurface; // <- what kind of haxx0ry is that? // ATM we only have one display. pdcattr->ulDirty_ |= DC_PRIMARY_DISPLAY; @@ -310,7 +311,7 @@ IntGdiCreateDC(PUNICODE_STRING Driver, */ pdc->dctype = DC_TYPE_INFO; // pdc->pSurfInfo = - pdc->dclevel.pSurface = NULL; + DC_vSelectSurface(pdc, NULL); pdcattr->crBackgroundClr = pdcattr->ulBackgroundClr = RGB(255, 255, 255); pdcattr->crForegroundClr = RGB(0, 0, 0); DC_UnlockDc( pdc ); diff --git a/reactos/subsystems/win32/win32k/objects/gdidbg.c b/reactos/subsystems/win32/win32k/objects/gdidbg.c index 145d6cc5dc4..a67189d3e58 100644 --- a/reactos/subsystems/win32/win32k/objects/gdidbg.c +++ b/reactos/subsystems/win32/win32k/objects/gdidbg.c @@ -7,6 +7,7 @@ static int leak_reported = 0; #define GDI_STACK_LEVELS 12 static ULONG GDIHandleAllocator[GDI_HANDLE_COUNT][GDI_STACK_LEVELS+1]; static ULONG GDIHandleLocker[GDI_HANDLE_COUNT][GDI_STACK_LEVELS+1]; +static ULONG GDIHandleShareLocker[GDI_HANDLE_COUNT][GDI_STACK_LEVELS+1]; static ULONG GDIHandleDeleter[GDI_HANDLE_COUNT][GDI_STACK_LEVELS+1]; struct DbgOpenGDIHandle { @@ -233,7 +234,6 @@ GdiDbgHTIntegrityCheck() return r; } - #define GDIDBG_TRACECALLER() \ DPRINT1("-> called from:\n"); \ KeRosDumpStackFrames(NULL, 20); @@ -243,6 +243,9 @@ GdiDbgHTIntegrityCheck() #define GDIDBG_TRACELOCKER(handle) \ DPRINT1("-> locked from:\n"); \ KeRosDumpStackFrames(GDIHandleLocker[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); +#define GDIDBG_TRACESHARELOCKER(handle) \ + DPRINT1("-> locked from:\n"); \ + KeRosDumpStackFrames(GDIHandleShareLocker[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); #define GDIDBG_TRACEDELETER(handle) \ DPRINT1("-> deleted from:\n"); \ KeRosDumpStackFrames(GDIHandleDeleter[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); @@ -250,6 +253,8 @@ GdiDbgHTIntegrityCheck() CaptureStackBackTace((PVOID*)GDIHandleAllocator[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); #define GDIDBG_CAPTURELOCKER(handle) \ CaptureStackBackTace((PVOID*)GDIHandleLocker[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); +#define GDIDBG_CAPTURESHARELOCKER(handle) \ + CaptureStackBackTace((PVOID*)GDIHandleShareLocker[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); #define GDIDBG_CAPTUREDELETER(handle) \ CaptureStackBackTace((PVOID*)GDIHandleDeleter[GDI_HANDLE_GET_INDEX(handle)], GDI_STACK_LEVELS); #define GDIDBG_DUMPHANDLETABLE() \ @@ -262,13 +267,25 @@ GdiDbgHTIntegrityCheck() DPRINT1("[%d] Handle 0x%p Locked by 0x%x (we're 0x%x)\n", Attempts, Handle, PrevThread, Thread); \ } +ULONG +FASTCALL +GDIOBJ_IncrementShareCount(POBJ Object) +{ + INT cLocks = InterlockedIncrement((PLONG)&Object->ulShareCount); + GDIDBG_CAPTURESHARELOCKER(Object->hHmgr); + ASSERT(cLocks >= 1); + return cLocks; +} + #else #define GDIDBG_TRACECALLER() #define GDIDBG_TRACEALLOCATOR(index) #define GDIDBG_TRACELOCKER(index) +#define GDIDBG_TRACESHARELOCKER(index) #define GDIDBG_CAPTUREALLOCATOR(index) #define GDIDBG_CAPTURELOCKER(index) +#define GDIDBG_CAPTURESHARELOCKER(index) #define GDIDBG_CAPTUREDELETER(handle) #define GDIDBG_DUMPHANDLETABLE() #define GDIDBG_INITLOOPTRACE() diff --git a/reactos/subsystems/win32/win32k/objects/gdiobj.c b/reactos/subsystems/win32/win32k/objects/gdiobj.c index 31786ccf884..4039dc86528 100644 --- a/reactos/subsystems/win32/win32k/objects/gdiobj.c +++ b/reactos/subsystems/win32/win32k/objects/gdiobj.c @@ -481,6 +481,8 @@ GDIOBJ_FreeObj(POBJ pObject, UCHAR BaseType) * \return Returns TRUE if succesful. * \return Returns FALSE if the cleanup routine returned FALSE or the object doesn't belong * to the calling process. + * + * \bug This function should return VOID and kill the object no matter what... */ BOOL INTERNAL_CALL GDIOBJ_FreeObjByHandle(HGDIOBJ hObj, DWORD ExpectedType) @@ -537,8 +539,9 @@ LockHandle: Object = Entry->KernelData; - if (Object->cExclusiveLock == 0 || - Object->Tid == (PW32THREAD)PsGetCurrentThreadWin32Thread()) + if ((Object->cExclusiveLock == 0 || + Object->Tid == (PW32THREAD)PsGetCurrentThreadWin32Thread()) && + Object->ulShareCount == 0) { BOOL Ret; PW32PROCESS W32Process = PsGetCurrentProcessWin32Process(); @@ -569,6 +572,15 @@ LockHandle: GDIDBG_CAPTUREDELETER(hObj); return Ret; } + else if (Object->ulShareCount != 0) + { + DPRINT("Object %p, ulShareCount = %d\n", Object->hHmgr, Object->ulShareCount); + GDIDBG_TRACECALLER(); + GDIDBG_TRACESHARELOCKER(GDI_HANDLE_GET_INDEX(hObj)); + (void)InterlockedExchangePointer((PVOID*)&Entry->ProcessId, PrevProcId); + /* Don't wait on shared locks */ + return FALSE; + } else { /* @@ -702,8 +714,12 @@ IntDeleteHandlesForProcess(struct _EPROCESS *Process, ULONG ObjectType) simply ignore this fact here. */ ObjectHandle = (HGDIOBJ)(Index | (Entry->Type << GDI_ENTRY_UPPER_SHIFT)); - if (GDIOBJ_FreeObjByHandle(ObjectHandle, GDI_OBJECT_TYPE_DONTCARE) && - W32Process->GDIObjects == 0) + if (!GDIOBJ_FreeObjByHandle(ObjectHandle, GDI_OBJECT_TYPE_DONTCARE)) + { + DPRINT1("Failed to delete object %p!\n", ObjectHandle); + } + + if (W32Process->GDIObjects == 0) { /* there are no more gdi handles for this process, bail */ break; @@ -723,6 +739,7 @@ BOOL INTERNAL_CALL GDI_CleanupForProcess(struct _EPROCESS *Process) { PEPROCESS CurrentProcess; + PW32PROCESS W32Process; DPRINT("Starting CleanupForProcess prochandle %x Pid %d\n", Process, Process->UniqueProcessId); CurrentProcess = PsGetCurrentProcess(); @@ -731,6 +748,8 @@ GDI_CleanupForProcess(struct _EPROCESS *Process) KeAttachProcess(&Process->Pcb); } + W32Process = (PW32PROCESS)CurrentProcess->Win32Process; + /* Delete objects. Begin with types that are not referenced by other types */ IntDeleteHandlesForProcess(Process, GDILoObjType_LO_DC_TYPE); IntDeleteHandlesForProcess(Process, GDILoObjType_LO_BRUSH_TYPE); @@ -749,6 +768,10 @@ GDI_CleanupForProcess(struct _EPROCESS *Process) #endif DPRINT("Completed cleanup for process %d\n", Process->UniqueProcessId); + if (W32Process->GDIObjects > 0) + { + DPRINT1("Leaking %d handles!\n", W32Process->GDIObjects); + } return TRUE; } @@ -973,11 +996,12 @@ GDIOBJ_ShareLockObj(HGDIOBJ hObj, DWORD ExpectedType) { Object = (POBJ)Entry->KernelData; -#ifdef GDI_DEBUG +GDIDBG_CAPTURESHARELOCKER(HandleIndex); +#ifdef GDI_DEBUG3 if (InterlockedIncrement((PLONG)&Object->ulShareCount) == 1) { memset(GDIHandleLocker[HandleIndex], 0x00, GDI_STACK_LEVELS * sizeof(ULONG)); - RtlCaptureStackBackTrace(1, GDI_STACK_LEVELS, (PVOID*)GDIHandleLocker[HandleIndex], NULL); + RtlCaptureStackBackTrace(1, GDI_STACK_LEVELS, (PVOID*)GDIHandleShareLocker[HandleIndex], NULL); } #else InterlockedIncrement((PLONG)&Object->ulShareCount);