[Webkit-unassigned] [Bug 189335] [RunLoopGeneric] OneShotTimer should not remain "isActive" after fired

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=189335

--- Comment #15 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 349292
  --> https://bugs.webkit.org/attachment.cgi?id=349292
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349292&action=review

>>> Source/WTF/wtf/generic/RunLoopGeneric.cpp:54
>>> +            deactivate();
>> 
>> Why do we deactivate the timer before `m_function()` is called?
>> Cocoa RunLoop says "A non-repeating timer is automatically invalidated after it fires.".
> 
> I'd just started looking at finishing up this patch, and was playing with the positioning based on this comment. However, I noticed that WTF_RunLoop.OneShotTimer failed if we move the deactivation until after the function is called as it appears to expect isActive() to be false at the time DerivedOneShotTimer's fired function is called.
> 
> Looking at some other cases, RunLoopCF seems to call CFRunLoopTimerInvalidate for the timer before calling the fired function as well if CFRunLoopTimerDoesRepeat is false which makes isActive() return false:
> 
>     m_timer = createTimer(interval, repeat, [] (CFRunLoopTimerRef cfTimer, void* context) {
>         AutodrainedPool pool;
> 
>         auto timer = static_cast<TimerBase*>(context);
>         if (!CFRunLoopTimerDoesRepeat(cfTimer))
>             CFRunLoopTimerInvalidate(cfTimer);
> 
>         timer->fired();
>     }, this);

My memory of this issue is that a lot of WebKit authors like to write code of the form "if (!timer.isActive()) { scheduleTimer(); }". For code like that, it is essential for isActive() to become false before we invoke the timer fired callback. That way, if something happens inside the timer fired callback that triggers scheduling the timer again, we do it right, instead of believing, incorrectly, that the timer is already scheduled.

I think that's why I added the explicit call to CFRunLoopTimerInvalidate() for non-repeating timers. Apparently I missed the RunLoopGeneric case.

> Source/WTF/wtf/generic/RunLoopGeneric.cpp:59
> +        if (isActive())
> +            updateReadyTime();

Why did the ready time update also move? For a non-repeating timer, it seems irrelevant. For a repeating timer, wouldn't I want to know the *next* scheduled repeat time, rather than the one that just happened?

> Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:88
> +            RunLoop::current().dispatch([this] {

Delaying the action of this test until the next runloop iteration seems to skip testing the interesting case. The interesting case is whether isActive() is true or false *inside the timer callback function*, not at some later time.

Can you elaborate on why this test changed to compute its result on the next runloop iteration?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210331/2b98bb9e/attachment.htm>