Nothing like deploying code in the wild to find bugs. :( The user on the forum reported a crash after this patch and I reproduced it locally:
Thread 1 "Xorg" received signal SIGSEGV, Segmentation fault. 0x000055604b0b8acd in dixGetPrivateAddr (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:123 123 ../include/privates.h: No such file or directory. (gdb) bt #0 0x000055604b0b8acd in dixGetPrivateAddr (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:123 #1 0x000055604b0b8b5d in dixLookupPrivate (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:165 #2 0x000055604b0b8d79 in AnimCurTimerNotify (timer=0x55604d751a30, now=45483869, arg=0x55604d31cb70) at animcur.c:134 #3 0x000055604b15fc70 in DoTimer () #4 0x000055604b15fcd7 in DoTimers () #5 0x000055604b15ffa7 in WaitForSomething () #6 0x000055604af90ec1 in Dispatch () at dispatch.c:422 #7 0x000055604af9e7dd in dix_main (argc=14, argv=0x7ffd9f1c4c78, envp=0x7ffd9f1c4cf0) at main.c:287 #8 0x00007f64f5282f4a in __libc_start_main () at /usr/lib/libc.so.6 #9 0x000055604af82a8a in _start () #2 0x000055604b0b8d79 in AnimCurTimerNotify (timer=0x55604d751a30, now=45483869, arg=0x55604d31cb70) at animcur.c:134 134 in animcur.c (gdb) p pScreen $8 = (ScreenPtr) 0x0 I'm not sure how this is happening, yet. -- Aaron On 11/06/2017 12:19 PM, Adam Jackson wrote: > This is very slightly more efficient since the callback now doesn't need > to walk every input device, instead we know exactly which device's > cursor is being updated. AnimCurTimerNotify() gets outdented nicely as a > result. A more important side effect is that we can stop using the > TimerAbsolute mode and just pass in the relative delay. > > In AnimCurSetCursorPosition, we no longer need to rearm the timer with > the new screen; it is enough to update the device's state. In > AnimCurDisplayCursor we need to notice when we're switching from > animated cursor to regular and cancel the existing timer. > > Signed-off-by: Adam Jackson <a...@redhat.com> > --- > render/animcur.c | 87 > +++++++++++++++++++------------------------------------- > 1 file changed, 29 insertions(+), 58 deletions(-) > > diff --git a/render/animcur.c b/render/animcur.c > index 05dfc640aa..e59a7c3c40 100644 > --- a/render/animcur.c > +++ b/render/animcur.c > @@ -55,6 +55,7 @@ typedef struct _AnimCurElt { > typedef struct _AnimCur { > int nelt; /* number of elements in the elts array */ > AnimCurElt *elts; /* actually allocated right after the > structure */ > + OsTimerPtr timer; > } AnimCurRec, *AnimCurPtr; > > typedef struct _AnimScrPriv { > @@ -65,8 +66,6 @@ typedef struct _AnimScrPriv { > RealizeCursorProcPtr RealizeCursor; > UnrealizeCursorProcPtr UnrealizeCursor; > RecolorCursorProcPtr RecolorCursor; > - OsTimerPtr timer; > - Bool timer_set; > } AnimCurScreenRec, *AnimCurScreenPtr; > > static unsigned char empty[4]; > @@ -131,49 +130,27 @@ AnimCurCursorLimits(DeviceIntPtr pDev, > static CARD32 > AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void *arg) > { > - ScreenPtr pScreen = arg; > + DeviceIntPtr dev = arg; > + ScreenPtr pScreen = dev->spriteInfo->anim.pScreen; > AnimCurScreenPtr as = GetAnimCurScreen(pScreen); > - DeviceIntPtr dev; > - Bool activeDevice = FALSE; > - CARD32 soonest = ~0; /* earliest time to wakeup again */ > - > - for (dev = inputInfo.devices; dev; dev = dev->next) { > - if (IsPointerDevice(dev) && pScreen == > dev->spriteInfo->anim.pScreen) { > - if (!activeDevice) > - activeDevice = TRUE; > - > - if ((INT32) (now - dev->spriteInfo->anim.time) >= 0) { > - AnimCurPtr ac = GetAnimCur(dev->spriteInfo->anim.pCursor); > - int elt = (dev->spriteInfo->anim.elt + 1) % ac->nelt; > - DisplayCursorProcPtr DisplayCursor; > - > - /* > - * Not a simple Unwrap/Wrap as this > - * isn't called along the DisplayCursor > - * wrapper chain. > - */ > - DisplayCursor = pScreen->DisplayCursor; > - pScreen->DisplayCursor = as->DisplayCursor; > - (void) (*pScreen->DisplayCursor) (dev, > - pScreen, > - ac->elts[elt].pCursor); > - as->DisplayCursor = pScreen->DisplayCursor; > - pScreen->DisplayCursor = DisplayCursor; > - > - dev->spriteInfo->anim.elt = elt; > - dev->spriteInfo->anim.time = now + ac->elts[elt].delay; > - } > > - if (soonest > dev->spriteInfo->anim.time) > - soonest = dev->spriteInfo->anim.time; > - } > - } > + AnimCurPtr ac = GetAnimCur(dev->spriteInfo->anim.pCursor); > + int elt = (dev->spriteInfo->anim.elt + 1) % ac->nelt; > + DisplayCursorProcPtr DisplayCursor = pScreen->DisplayCursor; > > - if (activeDevice) > - return soonest - now; > + /* > + * Not a simple Unwrap/Wrap as this isn't called along the DisplayCursor > + * wrapper chain. > + */ > + pScreen->DisplayCursor = as->DisplayCursor; > + (void) (*pScreen->DisplayCursor) (dev, pScreen, ac->elts[elt].pCursor); > + as->DisplayCursor = pScreen->DisplayCursor; > + pScreen->DisplayCursor = DisplayCursor; > > - as->timer_set = FALSE; > - return 0; > + dev->spriteInfo->anim.elt = elt; > + dev->spriteInfo->anim.time = now + ac->elts[elt].delay; > + > + return ac->elts[elt].delay; > } > > static Bool > @@ -199,17 +176,19 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr > pScreen, CursorPtr pCursor) > pDev->spriteInfo->anim.pCursor = pCursor; > pDev->spriteInfo->anim.pScreen = pScreen; > > - if (!as->timer_set) { > - TimerSet(as->timer, TimerAbsolute, > pDev->spriteInfo->anim.time, > - AnimCurTimerNotify, pScreen); > - as->timer_set = TRUE; > - } > + ac->timer = TimerSet(ac->timer, 0, ac->elts[0].delay, > + AnimCurTimerNotify, pDev); > } > } > else > ret = TRUE; > } > else { > + CursorPtr old = pDev->spriteInfo->anim.pCursor; > + > + if (old && IsAnimCur(old)) > + TimerCancel(GetAnimCur(old)->timer); > + > pDev->spriteInfo->anim.pCursor = 0; > pDev->spriteInfo->anim.pScreen = 0; > ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor); > @@ -228,12 +207,6 @@ AnimCurSetCursorPosition(DeviceIntPtr pDev, > Unwrap(as, pScreen, SetCursorPosition); > if (pDev->spriteInfo->anim.pCursor) { > pDev->spriteInfo->anim.pScreen = pScreen; > - > - if (!as->timer_set) { > - TimerSet(as->timer, TimerAbsolute, pDev->spriteInfo->anim.time, > - AnimCurTimerNotify, pScreen); > - as->timer_set = TRUE; > - } > } > ret = (*pScreen->SetCursorPosition) (pDev, pScreen, x, y, > generateEvent); > Wrap(as, pScreen, SetCursorPosition, AnimCurSetCursorPosition); > @@ -308,11 +281,6 @@ AnimCurInit(ScreenPtr pScreen) > return FALSE; > > as = GetAnimCurScreen(pScreen); > - as->timer = TimerSet(NULL, TimerAbsolute, 0, AnimCurTimerNotify, > pScreen); > - if (!as->timer) { > - return FALSE; > - } > - as->timer_set = FALSE; > > Wrap(as, pScreen, CloseScreen, AnimCurCloseScreen); > > @@ -360,10 +328,14 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, > int ncursor, > > pCursor->id = cid; > > + ac = GetAnimCur(pCursor); > + ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL); > + > /* security creation/labeling check */ > rc = XaceHook(XACE_RESOURCE_ACCESS, client, cid, RT_CURSOR, pCursor, > RT_NONE, NULL, DixCreateAccess); > if (rc != Success) { > + TimerFree(ac->timer); > dixFiniPrivates(pCursor, PRIVATE_CURSOR); > free(pCursor); > return rc; > @@ -373,7 +345,6 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int > ncursor, > * Fill in the AnimCurRec > */ > animCursorBits.refcnt++; > - ac = GetAnimCur(pCursor); > ac->nelt = ncursor; > ac->elts = (AnimCurElt *) (ac + 1); > > _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel