From e3d2a7a613366c26e5316cf582d9a8c6c6692b0a Mon Sep 17 00:00:00 2001 From: Peter Hutterer Date: Wed, 21 Apr 2010 11:47:24 +1000 Subject: [PATCH] Revert "mi: don't thrash resources when displaying the software cursor across screens" This commit leads to a segfault on the very first XTS test case. Backtrace: 0: /opt/xorg/bin/Xorg (xorg_backtrace+0x3b) [0x80a33db] 1: /opt/xorg/bin/Xorg (0x8048000+0x62a75) [0x80aaa75] 2: (vdso) (__kernel_rt_sigreturn+0x0) [0x5d140c] 3: /lib/libc.so.6 (0x9bb000+0x73579) [0xa2e579] 4: /lib/libc.so.6 (realloc+0xe0) [0xa2e830] 5: /opt/xorg/bin/Xorg (Xrealloc+0x33) [0x80a3f33] 6: /opt/xorg/bin/Xorg (0x8048000+0x1ab79) [0x8062b79] 7: /opt/xorg/bin/Xorg (0x8048000+0x1ac4e) [0x8062c4e] 8: /opt/xorg/bin/Xorg (RegisterExtensionNames+0x2ce) [0x8062fbe] 9: /opt/xorg/bin/Xorg (AddExtension+0x19a) [0x807bd7a] 10: /opt/xorg//lib/xorg/modules/extensions/libextmod.so (0x728000+0x1169a) [0x73969a] 11: /opt/xorg/bin/Xorg (InitExtensions+0x85) [0x80c0eb5] 12: /opt/xorg/bin/Xorg (0x8048000+0x1a51d) [0x806251d] 13: /lib/libc.so.6 (__libc_start_main+0xe6) [0x9d1bb6] 14: /opt/xorg/bin/Xorg (0x8048000+0x1a2a1) [0x80622a1] Segmentation fault at address 0x10b2d5f8 valgrind output: ==5069== Invalid read of size 4 ==5069== at 0x80F928D: FreePicture (picture.c:1531) ==5069== by 0x818DDEF: miDCDeviceCleanup (midispcur.c:867) ==5069== by 0x81B97F0: miSpriteDeviceCursorCleanup (misprite.c:968) ==5069== by 0x80995FA: miPointerDeviceCleanup (mipointer.c:292) ==5069== by 0x807973E: CloseDevice (devices.c:840) ==5069== by 0x80799B6: CloseDownDevices (devices.c:933) ==5069== by 0x8062705: main (main.c:309) ==5069== Address 0x4cce844 is 12 bytes inside a block of size 84 free'd ==5069== at 0x40057F6: free (vg_replace_malloc.c:325) ==5069== by 0x80A3DE0: Xfree (utils.c:1154) ==5069== by 0x80F9332: FreePicture (picture.c:1576) ==5069== by 0x80FBB4B: PictureDestroyWindow (picture.c:69) ==5069== by 0x810B1A3: damageDestroyWindow (damage.c:1840) ==5069== by 0x80864F1: FreeWindowResources (window.c:846) ==5069== by 0x8086812: DeleteWindow (window.c:925) ==5069== by 0x806B53E: FreeClientResources (resource.c:806) ==5069== by 0x806B60F: FreeAllResources (resource.c:823) ==5069== by 0x80626E4: main (main.c:299) ==5069== ==5069== Invalid write of size 4 ==5069== at 0x80F9295: FreePicture (picture.c:1531) ==5069== by 0x818DDEF: miDCDeviceCleanup (midispcur.c:867) ==5069== by 0x81B97F0: miSpriteDeviceCursorCleanup (misprite.c:968) ==5069== by 0x80995FA: miPointerDeviceCleanup (mipointer.c:292) ==5069== by 0x807973E: CloseDevice (devices.c:840) ==5069== by 0x80799B6: CloseDownDevices (devices.c:933) ==5069== by 0x8062705: main (main.c:309) ==5069== Address 0x4cce844 is 12 bytes inside a block of size 84 free'd ==5069== at 0x40057F6: free (vg_replace_malloc.c:325) ==5069== by 0x80A3DE0: Xfree (utils.c:1154) ==5069== by 0x80F9332: FreePicture (picture.c:1576) ==5069== by 0x80FBB4B: PictureDestroyWindow (picture.c:69) ==5069== by 0x810B1A3: damageDestroyWindow (damage.c:1840) ==5069== by 0x80864F1: FreeWindowResources (window.c:846) ==5069== by 0x8086812: DeleteWindow (window.c:925) ==5069== by 0x806B53E: FreeClientResources (resource.c:806) ==5069== by 0x806B60F: FreeAllResources (resource.c:823) ==5069== by 0x80626E4: main (main.c:299) XTS test case: Xproto pAllocColor This reverts commit 00b8b7ad61b6f818271fb4d1e383113170309d72. Signed-off-by: Peter Hutterer Signed-off-by: Keith Packard --- mi/midispcur.c | 266 +++++++++++++++++++++++++++++-------------------- 1 file changed, 157 insertions(+), 109 deletions(-) diff --git a/mi/midispcur.c b/mi/midispcur.c index 3a31b74d5..3fb7e02b5 100644 --- a/mi/midispcur.c +++ b/mi/midispcur.c @@ -59,9 +59,9 @@ static DevPrivateKey miDCScreenKey = &miDCScreenKeyIndex; static Bool miDCCloseScreen(int index, ScreenPtr pScreen); -/* per device per-screen private data */ -static int miDCSpriteKeyIndex[MAXSCREENS]; -static DevPrivateKey miDCSpriteKey = miDCSpriteKeyIndex; +/* per device private data */ +static int miDCSpriteKeyIndex; +static DevPrivateKey miDCSpriteKey = &miDCSpriteKeyIndex; typedef struct { GCPtr pSourceGC, pMaskGC; @@ -75,10 +75,10 @@ typedef struct { #endif } miDCBufferRec, *miDCBufferPtr; -#define MIDCBUFFER(dev, screen) \ +#define MIDCBUFFER(dev) \ ((DevHasCursor(dev)) ? \ - (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey + (screen)->myNum) : \ - (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey + (screen)->myNum)) + (miDCBufferPtr)dixLookupPrivate(&dev->devPrivates, miDCSpriteKey) : \ + (miDCBufferPtr)dixLookupPrivate(&dev->u.master->devPrivates, miDCSpriteKey)) /* * The core pointer buffer will point to the index of the virtual core pointer @@ -158,6 +158,10 @@ miDCInitialize (ScreenPtr pScreen, miPointerScreenFuncPtr screenFuncs) return TRUE; } +#define tossGC(gc) (gc ? FreeGC (gc, (GContext) 0) : 0) +#define tossPix(pix) (pix ? (*pScreen->DestroyPixmap) (pix) : TRUE) +#define tossPict(pict) (pict ? FreePicture (pict, 0) : 0) + static Bool miDCCloseScreen (int index, ScreenPtr pScreen) { @@ -179,6 +183,7 @@ miDCRealizeCursor (ScreenPtr pScreen, CursorPtr pCursor) } #ifdef ARGB_CURSOR +#define EnsurePicture(picture,draw,win) (picture || miDCMakePicture(&picture,draw,win)) static VisualPtr miDCGetWindowVisual (WindowPtr pWin) @@ -410,8 +415,12 @@ miDCPutBits ( (*maskGC->ops->PushPixels) (maskGC, pPriv->maskBits, pDrawable, w, h, x, y); } +#define EnsureGC(gc,win) (gc || miDCMakeGC(&gc, win)) + static GCPtr -miDCMakeGC(WindowPtr pWin) +miDCMakeGC( + GCPtr *ppGC, + WindowPtr pWin) { GCPtr pGC; int status; @@ -422,6 +431,7 @@ miDCMakeGC(WindowPtr pWin) pGC = CreateGC((DrawablePtr)pWin, GCSubwindowMode|GCGraphicsExposures, gcvals, &status, (XID)0, serverClient); + *ppGC = pGC; return pGC; } @@ -446,11 +456,22 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates, miDCScreenKey); pWin = WindowTable[pScreen->myNum]; - pBuffer = MIDCBUFFER(pDev, pScreen); + pBuffer = MIDCBUFFER(pDev); #ifdef ARGB_CURSOR if (pPriv->pPicture) { + /* see comment in miDCPutUpCursor */ + if (pBuffer->pRootPicture && + pBuffer->pRootPicture->pDrawable && + pBuffer->pRootPicture->pDrawable->pScreen != pScreen) + { + tossPict(pBuffer->pRootPicture); + pBuffer->pRootPicture = NULL; + } + + if (!EnsurePicture(pBuffer->pRootPicture, &pWin->drawable, pWin)) + return FALSE; CompositePicture (PictOpOver, pPriv->pPicture, NULL, @@ -463,6 +484,33 @@ miDCPutUpCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, else #endif { + /** + * XXX: Before MPX, the sourceGC and maskGC were attached to the + * screen, and would switch as the screen switches. With mpx we have + * the GC's attached to the device now, so each time we switch screen + * we need to make sure the GC's are allocated on the new screen. + * This is ... not optimal. (whot) + */ + if (pBuffer->pSourceGC && pScreen != pBuffer->pSourceGC->pScreen) + { + tossGC(pBuffer->pSourceGC); + pBuffer->pSourceGC = NULL; + } + + if (pBuffer->pMaskGC && pScreen != pBuffer->pMaskGC->pScreen) + { + tossGC(pBuffer->pMaskGC); + pBuffer->pMaskGC = NULL; + } + + if (!EnsureGC(pBuffer->pSourceGC, pWin)) + return FALSE; + if (!EnsureGC(pBuffer->pMaskGC, pWin)) + { + FreeGC (pBuffer->pSourceGC, (GContext) 0); + pBuffer->pSourceGC = 0; + return FALSE; + } miDCPutBits ((DrawablePtr)pWin, pPriv, pBuffer->pSourceGC, pBuffer->pMaskGC, x, y, pCursor->bits->width, pCursor->bits->height, @@ -483,7 +531,7 @@ miDCSaveUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen, pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates, miDCScreenKey); - pBuffer = MIDCBUFFER(pDev, pScreen); + pBuffer = MIDCBUFFER(pDev); pSave = pBuffer->pSave; pWin = WindowTable[pScreen->myNum]; @@ -496,7 +544,14 @@ miDCSaveUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen, if (!pSave) return FALSE; } - + /* see comment in miDCPutUpCursor */ + if (pBuffer->pSaveGC && pBuffer->pSaveGC->pScreen != pScreen) + { + tossGC(pBuffer->pSaveGC); + pBuffer->pSaveGC = NULL; + } + if (!EnsureGC(pBuffer->pSaveGC, pWin)) + return FALSE; pGC = pBuffer->pSaveGC; if (pSave->drawable.serialNumber != pGC->serialNumber) ValidateGC ((DrawablePtr) pSave, pGC); @@ -517,13 +572,20 @@ miDCRestoreUnderCursor (DeviceIntPtr pDev, ScreenPtr pScreen, pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates, miDCScreenKey); - pBuffer = MIDCBUFFER(pDev, pScreen); + pBuffer = MIDCBUFFER(pDev); pSave = pBuffer->pSave; pWin = WindowTable[pScreen->myNum]; if (!pSave) return FALSE; - + /* see comment in miDCPutUpCursor */ + if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen) + { + tossGC(pBuffer->pRestoreGC); + pBuffer->pRestoreGC = NULL; + } + if (!EnsureGC(pBuffer->pRestoreGC, pWin)) + return FALSE; pGC = pBuffer->pRestoreGC; if (pWin->drawable.serialNumber != pGC->serialNumber) ValidateGC ((DrawablePtr) pWin, pGC); @@ -545,7 +607,7 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen, pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates, miDCScreenKey); - pBuffer = MIDCBUFFER(pDev, pScreen); + pBuffer = MIDCBUFFER(pDev); pSave = pBuffer->pSave; pWin = WindowTable[pScreen->myNum]; @@ -554,7 +616,14 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen, */ if (!pSave) return FALSE; - + /* see comment in miDCPutUpCursor */ + if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen) + { + tossGC(pBuffer->pRestoreGC); + pBuffer->pRestoreGC = NULL; + } + if (!EnsureGC(pBuffer->pRestoreGC, pWin)) + return FALSE; pGC = pBuffer->pRestoreGC; if (pWin->drawable.serialNumber != pGC->serialNumber) ValidateGC ((DrawablePtr) pWin, pGC); @@ -593,7 +662,14 @@ miDCChangeSave (DeviceIntPtr pDev, ScreenPtr pScreen, (*pGC->ops->CopyArea) ((DrawablePtr) pSave, (DrawablePtr) pWin, pGC, 0, sourcey, -dx, copyh, x + dx, desty); } - + /* see comment in miDCPutUpCursor */ + if (pBuffer->pSaveGC && pBuffer->pSaveGC->pScreen != pScreen) + { + tossGC(pBuffer->pSaveGC); + pBuffer->pSaveGC = NULL; + } + if (!EnsureGC(pBuffer->pSaveGC, pWin)) + return FALSE; pGC = pBuffer->pSaveGC; if (pSave->drawable.serialNumber != pGC->serialNumber) ValidateGC ((DrawablePtr) pSave, pGC); @@ -690,7 +766,7 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, pScreenPriv = (miDCScreenPtr)dixLookupPrivate(&pScreen->devPrivates, miDCScreenKey); pWin = WindowTable[pScreen->myNum]; - pBuffer = MIDCBUFFER(pDev, pScreen); + pBuffer = MIDCBUFFER(pDev); pTemp = pBuffer->pTemp; if (!pTemp || @@ -733,9 +809,17 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, #ifdef ARGB_CURSOR if (pPriv->pPicture) { - if (!pBuffer->pTempPicture) - miDCMakePicture(&pBuffer->pTempPicture, &pTemp->drawable, pWin); + /* see comment in miDCPutUpCursor */ + if (pBuffer->pTempPicture && + pBuffer->pTempPicture->pDrawable && + pBuffer->pTempPicture->pDrawable->pScreen != pScreen) + { + tossPict(pBuffer->pTempPicture); + pBuffer->pTempPicture = NULL; + } + if (!EnsurePicture(pBuffer->pTempPicture, &pTemp->drawable, pWin)) + return FALSE; CompositePicture (PictOpOver, pPriv->pPicture, NULL, @@ -748,12 +832,38 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, else #endif { + if (!pBuffer->pPixSourceGC) + { + pBuffer->pPixSourceGC = CreateGC ((DrawablePtr)pTemp, + GCGraphicsExposures, &gcval, &status, (XID)0, serverClient); + if (!pBuffer->pPixSourceGC) + return FALSE; + } + if (!pBuffer->pPixMaskGC) + { + pBuffer->pPixMaskGC = CreateGC ((DrawablePtr)pTemp, + GCGraphicsExposures, &gcval, &status, (XID)0, serverClient); + if (!pBuffer->pPixMaskGC) + return FALSE; + } miDCPutBits ((DrawablePtr)pTemp, pPriv, pBuffer->pPixSourceGC, pBuffer->pPixMaskGC, dx, dy, pCursor->bits->width, pCursor->bits->height, source, mask); } + /* see comment in miDCPutUpCursor */ + if (pBuffer->pRestoreGC && pBuffer->pRestoreGC->pScreen != pScreen) + { + tossGC(pBuffer->pRestoreGC); + pBuffer->pRestoreGC = NULL; + } + /* + * copy the temporary pixmap onto the screen + */ + + if (!EnsureGC(pBuffer->pRestoreGC, pWin)) + return FALSE; pGC = pBuffer->pRestoreGC; if (pWin->drawable.serialNumber != pGC->serialNumber) ValidateGC ((DrawablePtr) pWin, pGC); @@ -767,113 +877,51 @@ miDCMoveCursor (DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor, static Bool miDCDeviceInitialize(DeviceIntPtr pDev, ScreenPtr pScreen) { - miDCBufferPtr pBuffer; - WindowPtr pWin; - XID gcval = FALSE; - int status; - int i; + miDCBufferPtr pBuffer; - if (!DevHasCursor(pDev)) - return TRUE; - - for (i = 0; i < screenInfo.numScreens; i++) - { - pScreen = screenInfo.screens[i]; - - pBuffer = xalloc(sizeof(miDCBufferRec)); - if (!pBuffer) - goto failure; - - dixSetPrivate(&pDev->devPrivates, miDCSpriteKey + pScreen->myNum, pBuffer); - pWin = WindowTable[pScreen->myNum]; - - pBuffer->pSourceGC = miDCMakeGC(pWin); - if (!pBuffer->pSourceGC) - goto failure; - - pBuffer->pMaskGC = miDCMakeGC(pWin); - if (!pBuffer->pMaskGC) - goto failure; - - pBuffer->pSaveGC = miDCMakeGC(pWin); - if (!pBuffer->pSaveGC) - goto failure; - - pBuffer->pRestoreGC = miDCMakeGC(pWin); - if (!pBuffer->pRestoreGC) - goto failure; - - pBuffer->pMoveGC = CreateGC ((DrawablePtr)pWin, - GCGraphicsExposures, &gcval, &status, (XID)0, serverClient); - if (!pBuffer->pMoveGC) - goto failure; - - pBuffer->pPixSourceGC = CreateGC ((DrawablePtr)pWin, - GCGraphicsExposures, &gcval, &status, (XID)0, serverClient); - if (!pBuffer->pPixSourceGC) - goto failure; - - pBuffer->pPixMaskGC = CreateGC ((DrawablePtr)pWin, - GCGraphicsExposures, &gcval, &status, (XID)0, serverClient); - if (!pBuffer->pPixMaskGC) - goto failure; + pBuffer = xalloc(sizeof(miDCBufferRec)); + dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, pBuffer); + pBuffer->pSourceGC = + pBuffer->pMaskGC = + pBuffer->pSaveGC = + pBuffer->pRestoreGC = + pBuffer->pMoveGC = + pBuffer->pPixSourceGC = + pBuffer->pPixMaskGC = NULL; #ifdef ARGB_CURSOR - miDCMakePicture(&pBuffer->pRootPicture, &pWin->drawable, pWin); - if (!pBuffer->pRootPicture) - goto failure; - - pBuffer->pTempPicture = NULL; + pBuffer->pRootPicture = NULL; + pBuffer->pTempPicture = NULL; #endif - - // these get (re)allocated lazily depending on the cursor size - pBuffer->pSave = pBuffer->pTemp = NULL; - } + pBuffer->pSave = pBuffer->pTemp = NULL; return TRUE; - -failure: - - miDCDeviceCleanup(pDev, pScreen); - - return FALSE; } static void miDCDeviceCleanup(DeviceIntPtr pDev, ScreenPtr pScreen) { miDCBufferPtr pBuffer; - int i; if (DevHasCursor(pDev)) { - for (i = 0; i < screenInfo.numScreens; i++) - { - pScreen = screenInfo.screens[i]; - - pBuffer = MIDCBUFFER(pDev, pScreen); - - if (pBuffer) - { - if (pBuffer->pSourceGC) FreeGC(pBuffer->pSourceGC, (GContext) 0); - if (pBuffer->pMaskGC) FreeGC(pBuffer->pMaskGC, (GContext) 0); - if (pBuffer->pSaveGC) FreeGC(pBuffer->pSaveGC, (GContext) 0); - if (pBuffer->pRestoreGC) FreeGC(pBuffer->pRestoreGC, (GContext) 0); - if (pBuffer->pMoveGC) FreeGC(pBuffer->pMoveGC, (GContext) 0); - if (pBuffer->pPixSourceGC) FreeGC(pBuffer->pPixSourceGC, (GContext) 0); - if (pBuffer->pPixMaskGC) FreeGC(pBuffer->pPixMaskGC, (GContext) 0); - + pBuffer = MIDCBUFFER(pDev); + tossGC (pBuffer->pSourceGC); + tossGC (pBuffer->pMaskGC); + tossGC (pBuffer->pSaveGC); + tossGC (pBuffer->pRestoreGC); + tossGC (pBuffer->pMoveGC); + tossGC (pBuffer->pPixSourceGC); + tossGC (pBuffer->pPixMaskGC); + tossPix (pBuffer->pSave); + tossPix (pBuffer->pTemp); #ifdef ARGB_CURSOR - if (pBuffer->pRootPicture) FreePicture(pBuffer->pRootPicture, 0); - if (pBuffer->pTempPicture) FreePicture(pBuffer->pTempPicture, 0); +#if 0 /* This has been free()d before */ + tossPict (pScreenPriv->pRootPicture); +#endif + tossPict (pBuffer->pTempPicture); #endif - - if (pBuffer->pSave) (*pScreen->DestroyPixmap)(pBuffer->pSave); - if (pBuffer->pTemp) (*pScreen->DestroyPixmap)(pBuffer->pTemp); - - xfree(pBuffer); - dixSetPrivate(&pDev->devPrivates, miDCSpriteKey + pScreen->myNum, NULL); - } - } + xfree(pBuffer); + dixSetPrivate(&pDev->devPrivates, miDCSpriteKey, NULL); } }