Bug 170775 - [JSC][GTK] glib RunLoop does not accept negative start interval
Summary: [JSC][GTK] glib RunLoop does not accept negative start interval
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-12 09:40 PDT by Yusuke Suzuki
Modified: 2017-04-18 01:08 PDT (History)
13 users (show)

See Also:


Attachments
Patch (3.09 KB, patch)
2017-04-12 09:51 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-04-12 09:40:53 PDT
[JSC][GTK] glib RunLoop does not accept negative start interval
Comment 1 Yusuke Suzuki 2017-04-12 09:51:40 PDT
Created attachment 306915 [details]
Patch
Comment 2 Michael Catanzaro 2017-04-12 15:10:05 PDT
What is trying to use a negative start interval? Why should that not result in an assertion?
Comment 3 Yusuke Suzuki 2017-04-12 18:32:07 PDT
(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.
Comment 4 Yusuke Suzuki 2017-04-16 06:52:07 PDT
Ping?
Comment 5 Saam Barati 2017-04-16 09:54:16 PDT
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?
Comment 6 Yusuke Suzuki 2017-04-16 10:12:09 PDT
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.
Comment 7 Saam Barati 2017-04-16 10:14:41 PDT
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.
Comment 8 Yusuke Suzuki 2017-04-16 10:19:46 PDT
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)`.
Comment 9 Saam Barati 2017-04-16 11:13:10 PDT
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
Comment 10 Carlos Alberto Lopez Perez 2017-04-17 10:59:47 PDT
Could we land this now and leave the improvement over startOneShot() semantics for later? The GTK debug builds are really broken currently.
Comment 11 Yusuke Suzuki 2017-04-18 00:18:00 PDT
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.
Comment 12 Carlos Garcia Campos 2017-04-18 00:26:17 PDT
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.
Comment 13 Yusuke Suzuki 2017-04-18 00:28:44 PDT
Committed r215454: <http://trac.webkit.org/changeset/215454>
Comment 14 Yusuke Suzuki 2017-04-18 01:08:04 PDT
(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. ;)