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.
Created attachment 349004 [details] Adding EXPECT to the TestWTF RunLoop tests in order to check the timer status
Created attachment 349114 [details] Patch
Comment on attachment 349114 [details] Patch r=me
Comment on attachment 349114 [details] Patch Clearing flags on attachment: 349114 Committed r235784: <https://trac.webkit.org/changeset/235784>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44226604>
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.
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
Re-opened since this is blocked by bug 189421
(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?
(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.
Created attachment 349292 [details] Patch
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.".
(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);
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?
(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.
Created attachment 425179 [details] Patch
Created attachment 425286 [details] patch for recheck
Comment on attachment 425286 [details] patch for recheck r=me
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].