lv2 / pugl

A minimal portable API for embeddable GUIs
https://gitlab.com/lv2/pugl/
ISC License
174 stars 34 forks source link

puglStopTimer() kills more that one timer #101

Closed 7890 closed 1 year ago

7890 commented 1 year ago

Hi, this line could be problematic: https://github.com/lv2/pugl/blob/9b5a0871c1a8771dbe204e60f437653a67abd42a/src/x11.c#L1381 It will eventually remove another registered timer.

        memmove(w->timers + i,
                w->timers + i + 1,
                sizeof(PuglTimer) * (w->numTimers - i - 1));
        memset(&w->timers[i], 0, sizeof(PuglTimer)); ///<<< removing this solved weird timer behaviour

To trigger the bug, register more than one timer, stop the 1st registered -> 2nd registered will also stop (unexpectedly). Please confirm, greetings

rantlivelintkale commented 1 year ago

I assume the memset was meant to be on &w->timers[w->numTimers - 1] to set the last index of w->timers zero since that memmove is moving every timer one index backwards leaving the last index »duplicated». Now, it seems to memmove the next timer on top of the one that is removed and then memset that moved timer to zero.

7890 commented 1 year ago

Yes, that could make sense. The code could be like

if (i != w->numTimers - 1) {
  memmove(w->timers + i,
          w->timers + i + 1,
          sizeof(PuglTimer) * (w->numTimers - i - 1));
}
memset(&w->timers[w->numTimers - 1], 0, sizeof(PuglTimer));
--w->numTimers;
return PUGL_SUCCESS;

(untested)

7890 commented 1 year ago

Let's write a test to detect missing timers, then create a PR with test & fix.

drobilla commented 1 year ago

Fixed in 6b31fb5, thanks.