Bug 170775 – [JSC][GTK] glib RunLoop does not accept negative start interval

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC][GTK] glib RunLoop does not accept negative start interval

[Yusuke Suzuki]

Created attachment 306915 [details]
Patch

[Michael Catanzaro]

What is trying to use a negative start interval? Why should that not result in an assertion?

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #2)
> What is trying to use a negative start interval? Why should that not result
> in an assertion?

In the GCActivityCallback timer, we usually repeatedly invokes the callback with decade, but sometime we would like to invoke the callback soon.
In the code, we calculate the delta in scheduleTimer. But above calculation can result negative interval if enough time is already spent. It is valid. In such a case, we should schedule the timer with 0_s. This patch just does so.

Previously, we directly use glib timer, which does not have any assertion with negative delays. But now, we start using RunLoop::Timer, which has such an assertion. Thus we should have this patch.

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 306915 [details]
Patch

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

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));

Why is this needed?

[Yusuke Suzuki]

Comment on attachment 306915 [details]
Patch

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

>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
> 
> Why is this needed?

For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.

[Saam Barati]

Comment on attachment 306915 [details]
Patch

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

>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
>> 
>> Why is this needed?
> 
> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.

Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code.

[Yusuke Suzuki]

Comment on attachment 306915 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>>>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
>>> 
>>> Why is this needed?
>> 
>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.
> 
> Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code.

I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`.

[Saam Barati]

Comment on attachment 306915 [details]
Patch

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

r=me

>>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>>>>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
>>>> 
>>>> Why is this needed?
>>> 
>>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.
>> 
>> Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code.
> 
> I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`.

Ok. It seems reasonable to me to make the change in startOneShot, but I'll leave it up to you

[Carlos Alberto Lopez Perez]

Could we land this now and leave the improvement over startOneShot() semantics for later? The GTK debug builds are really broken currently.

[Yusuke Suzuki]

Comment on attachment 306915 [details]
Patch

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

>>>>>> Source/JavaScriptCore/heap/GCActivityCallback.cpp:92
>>>>>> +    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
>>>>> 
>>>>> Why is this needed?
>>>> 
>>>> For `std::max<Seconds>()`, this is because RunLoop::Timer in glib has assumption that incoming interval is not negative.
>>> 
>>> Why not remove that assumption since it's wrong? Seems cleaner to me. Numbers less than current time happen for very natural reasons in code.
>> 
>> I think `startOneShot(negative)` is ok. But I'm worried about that it becomes a bit ugly semantics when getting `startRepeating(negative)`.
> 
> Ok. It seems reasonable to me to make the change in startOneShot, but I'll leave it up to you

OK, I'll do the both. While accepting negative seconds is ok, I think leaving `std::max<Seconds>()` is good annotation that this may produce negative values.
Since we do not hit that assertion before, it means that all the other places simply uses positive value, it is good.

[Carlos Garcia Campos]

Note that GSource actually accepts negative values, but with a different meaning. It only accepts -1 to disable the source, which means never fire this. If negative values are allowed by RunLoop in general, and mean fire ASAP, we should handle that case in the runLoop glib implementation by using 0 in such cases.

[Yusuke Suzuki]

Committed r215454: <http://trac.webkit.org/changeset/215454>

[Yusuke Suzuki]

(In reply to Carlos Garcia Campos from comment #12)
> Note that GSource actually accepts negative values, but with a different
> meaning. It only accepts -1 to disable the source, which means never fire
> this. If negative values are allowed by RunLoop in general, and mean fire
> ASAP, we should handle that case in the runLoop glib implementation by using
> 0 in such cases.

Right. Landed one does so. ;)

[Yusuke Suzuki]

*** Bug 170893 has been marked as a duplicate of this bug. ***