[tkreuzer] 40196: - 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.
Author: tkreuzer Date: Tue Mar 24 02:49:00 2009 New Revision: 40196
URL: http://svn.reactos.org/svn/reactos?rev=40196&view=rev Log: - 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.
Modified: trunk/reactos/subsystems/win32/win32k/include/dc.h trunk/reactos/subsystems/win32/win32k/include/gdiobj.h trunk/reactos/subsystems/win32/win32k/objects/bitmaps.c trunk/reactos/subsystems/win32/win32k/objects/dclife.c trunk/reactos/subsystems/win32/win32k/objects/gdidbg.c trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c
Modified: trunk/reactos/subsystems/win32/win32k/include/dc.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/inc... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/include/dc.h [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/include/dc.h [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -278,6 +278,20 @@
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;
Modified: trunk/reactos/subsystems/win32/win32k/include/gdiobj.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/inc... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/include/gdiobj.h [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/include/gdiobj.h [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -104,16 +104,13 @@ 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 @@ ASSERT(cLocks >= 1); return cLocks; } +#endif
#endif
Modified: trunk/reactos/subsystems/win32/win32k/objects/bitmaps.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/obj... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/objects/bitmaps.c [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/objects/bitmaps.c [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -932,7 +932,7 @@ PDC pDC; PDC_ATTR pdcattr; HBITMAP hOrgBmp; - PSURFACE psurfBmp; + PSURFACE psurfBmp, psurfOld; HRGN hVisRgn; BOOLEAN bFailed; PBRUSH pbrush; @@ -961,23 +961,26 @@ 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 @@ 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) {
Modified: trunk/reactos/subsystems/win32/win32k/objects/dclife.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/obj... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/objects/dclife.c [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/objects/dclife.c [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -140,6 +140,7 @@ 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 @@ 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 @@ */ 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 );
Modified: trunk/reactos/subsystems/win32/win32k/objects/gdidbg.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/obj... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/objects/gdidbg.c [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/objects/gdidbg.c [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -7,6 +7,7 @@ #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 @@ return r; }
- #define GDIDBG_TRACECALLER() \ DPRINT1("-> called from:\n"); \ KeRosDumpStackFrames(NULL, 20); @@ -243,6 +243,9 @@ #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 @@ 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 @@ 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()
Modified: trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/obj... ============================================================================== --- trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c [iso-8859-1] (original) +++ trunk/reactos/subsystems/win32/win32k/objects/gdiobj.c [iso-8859-1] Tue Mar 24 02:49:00 2009 @@ -481,6 +481,8 @@ * \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 @@
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(); @@ -568,6 +571,15 @@
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 @@ 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 @@ GDI_CleanupForProcess(struct _EPROCESS *Process) { PEPROCESS CurrentProcess; + PW32PROCESS W32Process;
DPRINT("Starting CleanupForProcess prochandle %x Pid %d\n", Process, Process->UniqueProcessId); CurrentProcess = PsGetCurrentProcess(); @@ -730,6 +747,8 @@ { 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); @@ -749,6 +768,10 @@ #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 @@ { 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);
-
tkreuzer＠svn.reactos.org