[PATCH xserver] animcur: Move timer into pDev->spriteInfo->anim

[Aaron Plattner]

On 01/09/2018 08:49 AM, Adam Jackson wrote:
> On Mon, 2018-01-08 at 15:02 -0800, Aaron Plattner wrote:
>> Commit 094a63d56fbfb9e23210cc9ac538fb198af37cee moved the timer that handles
>> animated cursors from the per-screen AnimCurScreenRec into the per-cursor
>> AnimCurRec. However, the timer that runs takes the DeviceIntPtr as its argument,
>> and looks up which screen the cursor is supposed to be on from that.
>>
>> This leads to a problem when a device changes from one animated cursor to
>> another: The timer for the first cursor is not canceled. Then, when the device
>> transitions to a static cursor, the second timer is canceled
>> pDev->spriteInfo->anim.pScreen is cleared. Finally, the first timer fires and
>> AnimCurTimerNotify crashes because pScreen is NULL.
>>
>> Since there's only ever supposed to be one timer pending for a given device,
>> move the timer into pDev->spriteInfo->anim.pTimer. This timer is canceled when
>> transitioning to a static cursor, and re-armed when transitioning from one
>> animated cursor to another.
>>
>> Signed-off-by: Aaron Plattner <aplattner at nvidia.com>
>> Reported-by: https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967
>> ---
>> I'm not convinced that I fully understood how these timers worked with multiple
>> X screens before 094a63d56fbf, so hopefully this is an acceptable fix. It seems
>> to work in my single-X-screen testing.
> 
> I think what you've got gets multiple screens right. I had considered
> just always cancelling the timer first, but that would mean re-arming
> the timer constantly too, and the less we can call TimerSet (and thus
> GetTimeInMillis) the better.
> 
> There's one corner case I think this gets wrong:
> 
>> @@ -325,13 +321,11 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int ncursor,
>>      pCursor->id = cid;
>>  
>>      ac = GetAnimCur(pCursor);
>> -    ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL);
> 
> This bit matters. If TimerSet's first argument is NULL it will
> allocate, and that can fail. If the TimerSet in AnimCurDisplayCursor
> would fail, we would need to unwind the work we just did to change the
> sprite, which could itself fail. That's too hairy to want to deal with,
> hence allocating up front. (Granted I didn't _check_ whether that
> allocation succeeded...)

Yeah, that's a good point.

> But then the other problem is:
> 
>> --- a/include/inputstr.h
>> +++ b/include/inputstr.h
>> @@ -514,6 +514,7 @@ typedef struct _SpriteInfoRec {
>>      struct {
>>          CursorPtr pCursor;
>>          ScreenPtr pScreen;
>> +        OsTimerPtr pTimer;
>>          int elt;
>>      } anim;
>>  } SpriteInfoRec, *SpriteInfoPtr;
> 
> That's an ABI header, so I can't cherry-pick this fix to 1.19.

I'd be surprised if anyone outside the server used these fields, but
you're right, better safe than sorry.

> I think we can just cancel the old timer when changing animcurs.
> Patches to follow in a moment.

Agreed.

> - ajax
>