[PATCH xserver 3/4] animcur: Run the timer from the device, not the screen

[Aaron Plattner]

On 01/08/2018 01:07 PM, Adam Jackson wrote:
> On Mon, 2018-01-08 at 12:39 -0800, Aaron Plattner wrote:
>> Nothing like deploying code in the wild to find bugs. :(
> 
> Hah! So it goes.
> 
>> 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.
> 
> Yeah that's a puzzler. The only place that should zero out anim.pScreen
> is the "switch to static cursor" path in AnimCurDisplayCursor, but that
> path also cancels the timer so AnimCurTimerNotify shouldn't get called
> for that device anymore. Maybe I missed something about how multiple
> cursors work?

Yeah, the problem seems to be when the cursor for the pDev transitions 
from one animated cursor to another. It doesn't cancel the first timer 
from the first cursor, but still schedules a new one for the second 
cursor. Later, when it transitions to a static cursor, it cancels the 
second timer and clears
pDev->spriteInfo->anim.pScreen, and then the first timer fires:

  Dev 0x55e84a07dd70 animcursor went from (nil) to 0x55e84a294340
  Dev 0x55e84a07dd70 animcursor went from (nil) to 0x55e84a3b5ad0
  Dev 0x55e84a07dd70 animcursor went from (nil) to 0x55e84a4cb770, 
scheduled timer 0x55e84a4cbb90
  Dev 0x55e84a07dd70 animcursor went from 0x55e84a4cb770 to 
0x55e84a4b4570, scheduled timer 0x55e84a4b1120
  Dev 0x55e84a07dd70 animcursor went from 0x55e84a4b4570 to 0x55e84a46b800
  Canceled timer 0x55e84a4b1120
  (EE)
  (EE) Backtrace:
  (EE) 0: /usr/lib/xorg-server/Xorg (OsSigHandler+0x3b) [0x55e847e294f0]
  (EE) 1: /usr/lib/libpthread.so.0 (funlockfile+0x50) [0x7fa9aba62dff]
  (EE) 2: /usr/lib/xorg-server/Xorg (dixGetPrivateAddr+0x3e) 
[0x55e847d699f3]
  (EE) 3: /usr/lib/xorg-server/Xorg (dixLookupPrivate+0x2e) [0x55e847d69a83]
  (EE) 4: /usr/lib/xorg-server/Xorg (AnimCurTimerNotify+0x48) 
[0x55e847d69c9f]
  (EE) 5: /usr/lib/xorg-server/Xorg (DoTimer+0x37) [0x55e847e20c63]
  (EE) 6: /usr/lib/xorg-server/Xorg (DoTimers+0x31) [0x55e847e20cc7]
  (EE) 7: /usr/lib/xorg-server/Xorg (check_timers+0x3d) [0x55e847e209a7]
  (EE) 8: /usr/lib/xorg-server/Xorg (WaitForSomething+0x7e) [0x55e847e20a53]
  (EE) 9: /usr/lib/xorg-server/Xorg (Dispatch+0x53) [0x55e847c2c1de]
  (EE) 10: /usr/lib/xorg-server/Xorg (dix_main+0x651) [0x55e847c3b55f]
  (EE) 11: /usr/lib/xorg-server/Xorg (main+0x28) [0x55e847c1bff2]
  (EE) 12: /usr/lib/libc.so.6 (__libc_start_main+0xea) [0x7fa9ab6baf4a]
  (EE) 13: /usr/lib/xorg-server/Xorg (_start+0x2a) [0x55e847c1beea]
  (EE)
  (EE) Segmentation fault at address 0x3e8
  (EE)
  Fatal server error:
  (EE) Caught signal 11 (Segmentation fault). Server aborting

Given how this timer is used, would it make sense to move the timer into 
pDev->spriteInfo->anim.pTimer? The downside there I guess is that 
CloseDevice() would have to TimerFree() it.

-- Aaron