Bug 189335 – [RunLoopGeneric] OneShotTimer should not remain "isActive" after fired

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yoshiaki Jitsukawa]

Unlike the other implementations, RunLoopGeneric's one-shot timer remains active when being fired(). 

It seems that this assertion expects the one-shot timer to be deactivated there.
https://github.com/WebKit/webkit/blob/master/Source/WebKit/UIProcess/BackgroundProcessResponsivenessTimer.cpp#L150

I think RunLoopGeneric should follow the other RunLoop implementations.

[Yoshiaki Jitsukawa]

Created attachment 349004 [details]
Adding EXPECT to the TestWTF RunLoop tests in order to check the timer status

[Yoshiaki Jitsukawa]

Created attachment 349114 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 349114 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 349114 [details]
Patch

Clearing flags on attachment: 349114

Committed r235784: <https://trac.webkit.org/changeset/235784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44226604>

[Truitt Savell]

After this change https://trac.webkit.org/changeset/235784/webkit

we are seeing:

Failed

    TestWTF.WTF_RunLoop.ChainingOneShotTimer
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:120
        Value of: isActive()
          Actual: true
        Expected: false
        
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:120
        Value of: isActive()
          Actual: true
        Expected: false
        

    TestWTF.WTF_RunLoop.OneShotTimer
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp:88
        Value of: isActive()
          Actual: true
        Expected: false


This is occurring on Mac wk1.

[Truitt Savell]

Failure Result:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/12640

stdio:
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/12640/steps/run-api-tests/logs/stdio

[WebKit Commit Bot]

Re-opened since this is blocked by bug 189421

[Yusuke Suzuki]

(In reply to WebKit Commit Bot from comment #9)
> Re-opened since this is blocked by bug 189421

It seems that RunLoopCF does not agree to the fix in this patch.
Yoshiaki, can you take a look?

[Yoshiaki Jitsukawa]

(In reply to Yusuke Suzuki from comment #10)
> (In reply to WebKit Commit Bot from comment #9)
> > Re-opened since this is blocked by bug 189421
> 
> It seems that RunLoopCF does not agree to the fix in this patch.
> Yoshiaki, can you take a look?

Sorry, my assumption was wrong.

https://developer.apple.com/documentation/corefoundation/1543110-cfrunlooptimerisvalid
says "A non-repeating timer is automatically invalidated after it fires."

Let me fix.

[Yoshiaki Jitsukawa]

Created attachment 349292 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 349292 [details]
Patch

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

> Source/WTF/wtf/generic/RunLoopGeneric.cpp:54
>          if (!m_isRepeating)
> -            return false;
> +            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.".

[Stephan Szabo]

(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 349292 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349292&action=review
> 
> > Source/WTF/wtf/generic/RunLoopGeneric.cpp:54
> >          if (!m_isRepeating)
> > -            return false;
> > +            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);

[Geoffrey Garen]

Comment on attachment 349292 [details]
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?

[Stephan Szabo]

(In reply to Geoffrey Garen from comment #15)
> Comment on attachment 349292 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349292&action=review
> 
> > 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?

Yes, that's true, and I'm guessing that the time the function takes shouldn't be effectively added to the repeat time, so that should move back.

> > 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?

I'm not sure why it was done originally, but it does seem strange. I'll recheck since that test file has also seemingly changed since the original patch.

[Stephan Szabo]

Created attachment 425179 [details]
Patch

[Stephan Szabo]

Created attachment 425286 [details]
patch for recheck

[Geoffrey Garen]

Comment on attachment 425286 [details]
patch for recheck

r=me

[EWS]

Committed r275674 (236310@main): <https://commits.webkit.org/236310@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425286 [details].