[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.
23 Mar
2009
23 Mar
'09
11:49 p.m.
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/in…
==============================================================================
--- 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/in…
==============================================================================
--- 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/ob…
==============================================================================
--- 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/ob…
==============================================================================
--- 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/ob…
==============================================================================
--- 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/ob…
==============================================================================
--- 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);
5757
days inactive
5757
days old
0 comments
1 participants
participants (1)
-
tkreuzer＠svn.reactos.org