170775 2017-04-12 09:40:53 -0700 [JSC][GTK] glib RunLoop does not accept negative start interval 2017-06-04 00:34:05 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=171555 P2 Normal --- 1 ysuzuki ysuzuki benjamin buildbot cdumez cgarcia clopez cmarcelo dbates keith_miller mark.lam mcatanzaro msaboff pranjal.jumde saam xan.lopez oldest_to_newest 1296750 0 ysuzuki 2017-04-12 09:40:53 -0700 [JSC][GTK] glib RunLoop does not accept negative start interval 1296752 1 306915 ysuzuki 2017-04-12 09:51:40 -0700 Created attachment 306915 Patch 1296855 2 mcatanzaro 2017-04-12 15:10:05 -0700 What is trying to use a negative start interval? Why should that not result in an assertion? 1296902 3 ysuzuki 2017-04-12 18:32:07 -0700 (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. 1297757 4 ysuzuki 2017-04-16 06:52:07 -0700 Ping? 1297766 5 306915 saam 2017-04-16 09:54:16 -0700 Comment on attachment 306915 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? 1297769 6 306915 ysuzuki 2017-04-16 10:12:09 -0700 Comment on attachment 306915 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. 1297773 7 306915 saam 2017-04-16 10:14:41 -0700 Comment on attachment 306915 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. 1297774 8 306915 ysuzuki 2017-04-16 10:19:46 -0700 Comment on attachment 306915 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)`. 1297778 9 306915 saam 2017-04-16 11:13:10 -0700 Comment on attachment 306915 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 1297949 10 clopez 2017-04-17 10:59:47 -0700 Could we land this now and leave the improvement over startOneShot() semantics for later? The GTK debug builds are really broken currently. 1298202 11 306915 ysuzuki 2017-04-18 00:18:00 -0700 Comment on attachment 306915 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. 1298205 12 cgarcia 2017-04-18 00:26:17 -0700 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. 1298207 13 ysuzuki 2017-04-18 00:28:44 -0700 Committed r215454: <http://trac.webkit.org/changeset/215454> 1298221 14 ysuzuki 2017-04-18 01:08:04 -0700 (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. ;) 1315395 15 ysuzuki 2017-06-03 04:34:12 -0700 *** Bug 170893 has been marked as a duplicate of this bug. *** 306915 2017-04-12 09:51:40 -0700 2017-04-16 11:13:10 -0700 Patch bug-170775-20170413015139.patch text/plain 3163 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1MjY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBl Y2M5NTkyZDc2ZjQ5NWNmMzIwMTEzODM3YzZlM2ViODkwODQ2NzNmLi5kYTZiYjI5YzRkOTQ1NTEy ODA1MzM1MjFiZGYxZjgzYzVhNDkyNjk2IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs NSArMSwxNSBAQAogMjAxNy0wNC0xMiAgWXVzdWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWls LmNvbT4KIAorICAgICAgICBbSlNDXVtHVEtdIGdsaWIgUnVuTG9vcCBkb2VzIG5vdCBhY2NlcHQg bmVnYXRpdmUgc3RhcnQgaW50ZXJ2YWwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE3MDc3NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogaGVhcC9HQ0FjdGl2aXR5Q2FsbGJhY2suY3BwOgorICAgICAgICAo SlNDOjpHQ0FjdGl2aXR5Q2FsbGJhY2s6OnNjaGVkdWxlVGltZXIpOgorCisyMDE3LTA0LTEyICBZ dXN1a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21haWwuY29tPgorCiAgICAgICAgIFtXVEZdIElu dHJvZHVjZSBUaHJlYWQgY2xhc3MgYW5kIHVzZSBSZWZQdHI8VGhyZWFkPiBhbmQgYWxpZ24gV2lu ZG93cyBUaHJlYWRpbmcgaW1wbGVtZW50YXRpb24gc2VtYW50aWNzIHRvIFB0aHJlYWQgb25lCiAg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzA1MDIKIApk aWZmIC0tZ2l0IGEvU291cmNlL1dURi9DaGFuZ2VMb2cgYi9Tb3VyY2UvV1RGL0NoYW5nZUxvZwpp bmRleCAzMzJhZjk4NmU0YWExZTM3OWNjOTJmZGE0ZjVhMTcwODhkMzYwOGZjLi40Njc0ZjIwOTFi OTIwNTJmOGU2YzJmMGQzZDQwZjA1NWNmMGFiMDAyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL0No YW5nZUxvZworKysgYi9Tb3VyY2UvV1RGL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDE3 LTA0LTEyICBZdXN1a2UgU3V6dWtpICA8dXRhdGFuZS50ZWFAZ21haWwuY29tPgogCisgICAgICAg IFtKU0NdW0dUS10gZ2xpYiBSdW5Mb29wIGRvZXMgbm90IGFjY2VwdCBuZWdhdGl2ZSBzdGFydCBp bnRlcnZhbAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTcwNzc1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg UnVuTG9vcDo6VGltZXIgZm9yIGdsaWIgZG9lcyBub3QgYWNjZXB0IG5lZ2F0aXZlIHN0YXJ0IGlu dGVydmFsLgorICAgICAgICBXZSB1c2UgMF9zIGlmIHRoZSBnaXZlbiB2YWx1ZSBpcyBuZWdhdGl2 ZS4KKworICAgICAgICAqIHd0Zi9nbGliL1J1bkxvb3BHTGliLmNwcDoKKyAgICAgICAgKFdURjo6 UnVuTG9vcDo6VGltZXJCYXNlOjpzZWNvbmRzVW50aWxGaXJlKToKKworMjAxNy0wNC0xMiAgWXVz dWtlIFN1enVraSAgPHV0YXRhbmUudGVhQGdtYWlsLmNvbT4KKwogICAgICAgICBbV1RGXSBJbnRy b2R1Y2UgVGhyZWFkIGNsYXNzIGFuZCB1c2UgUmVmUHRyPFRocmVhZD4gYW5kIGFsaWduIFdpbmRv d3MgVGhyZWFkaW5nIGltcGxlbWVudGF0aW9uIHNlbWFudGljcyB0byBQdGhyZWFkIG9uZQogICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTcwNTAyCiAKZGlm ZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0dDQWN0aXZpdHlDYWxsYmFjay5j cHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9HQ0FjdGl2aXR5Q2FsbGJhY2suY3BwCmlu ZGV4IGMwMjcyMWRmOWJiYzJmYmIyOGRkZDE0ZDMxMmEwM2IyYjhlNGE3OTMuLjAwN2ZkZDgxMzM4 NzFjY2NmMzViYWI2MmNmN2Q1NWVkODNiZGE5YzAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS9oZWFwL0dDQWN0aXZpdHlDYWxsYmFjay5jcHAKKysrIGIvU291cmNlL0phdmFTY3Jp cHRDb3JlL2hlYXAvR0NBY3Rpdml0eUNhbGxiYWNrLmNwcApAQCAtODksNyArODksNyBAQCB2b2lk IEdDQWN0aXZpdHlDYWxsYmFjazo6c2NoZWR1bGVUaW1lcihTZWNvbmRzIG5ld0RlbGF5KQogICAg IG1fZGVsYXkgPSBuZXdEZWxheTsKIAogICAgIFNlY29uZHMgc2Vjb25kc1VudGlsRmlyZSA9IG1f dGltZXIuc2Vjb25kc1VudGlsRmlyZSgpOwotICAgIG1fdGltZXIuc3RhcnRPbmVTaG90KHNlY29u ZHNVbnRpbEZpcmUgLSBkZWx0YSk7CisgICAgbV90aW1lci5zdGFydE9uZVNob3Qoc3RkOjptYXg8 U2Vjb25kcz4oc2Vjb25kc1VudGlsRmlyZSAtIGRlbHRhLCAwX3MpKTsKIH0KIAogdm9pZCBHQ0Fj dGl2aXR5Q2FsbGJhY2s6OmNhbmNlbFRpbWVyKCkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3Rm L2dsaWIvUnVuTG9vcEdMaWIuY3BwIGIvU291cmNlL1dURi93dGYvZ2xpYi9SdW5Mb29wR0xpYi5j cHAKaW5kZXggY2ViNDliMzAxMjFlNWEzMWQwODVkZjk1NzRiNDAxMWI1MGJjNWY0OC4uYTdiODJk MDI4ZTgwNTBkMTg3NjBlM2M5NzRiNTcxM2M3OTJjYzVmNSAxMDA2NDQKLS0tIGEvU291cmNlL1dU Ri93dGYvZ2xpYi9SdW5Mb29wR0xpYi5jcHAKKysrIGIvU291cmNlL1dURi93dGYvZ2xpYi9SdW5M b29wR0xpYi5jcHAKQEAgLTIxOCw3ICsyMTgsMTAgQEAgYm9vbCBSdW5Mb29wOjpUaW1lckJhc2U6 OmlzQWN0aXZlKCkgY29uc3QKIAogU2Vjb25kcyBSdW5Mb29wOjpUaW1lckJhc2U6OnNlY29uZHNV bnRpbEZpcmUoKSBjb25zdAogewotICAgIHJldHVybiBzdGQ6Om1heDxTZWNvbmRzPihTZWNvbmRz Ojpmcm9tTWljcm9zZWNvbmRzKGdfc291cmNlX2dldF9yZWFkeV90aW1lKG1fc291cmNlLmdldCgp KSAtIGdfZ2V0X21vbm90b25pY190aW1lKCkpLCAwX3MpOworICAgIGdpbnQ2NCB0aW1lID0gZ19z b3VyY2VfZ2V0X3JlYWR5X3RpbWUobV9zb3VyY2UuZ2V0KCkpOworICAgIGlmICh0aW1lICE9IC0x KQorICAgICAgICByZXR1cm4gc3RkOjptYXg8U2Vjb25kcz4oU2Vjb25kczo6ZnJvbU1pY3Jvc2Vj b25kcyh0aW1lIC0gZ19nZXRfbW9ub3RvbmljX3RpbWUoKSksIDBfcyk7CisgICAgcmV0dXJuIDBf czsKIH0KIAogfSAvLyBuYW1lc3BhY2UgV1RGCg==