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

[Adam Jackson]

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

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 think we can just cancel the old timer when changing animcurs.
Patches to follow in a moment.

- ajax