- 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

@@ -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;

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

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)
     {

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 );

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()

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);