[ros-diffs] [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/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);