Closed Bug 1450066 Opened 2 years ago Closed 2 years ago

setInterval fires only once when no delay argument provided

Categories

(Core :: DOM: Core & HTML, defect, P2)

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: aaron.peltz, Assigned: farre)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.66 Safari/537.36

Steps to reproduce:

It seems that failing to provide a delay value to setInterval results in the callback being called once. I would have expected an error or warning, if not the minimum delay as a default if no delay argument is not provided.

To reproduce:
// In console
setInterval(function(){ console.log(Date.now())}, 0) // works

setInterval(function(){ console.log(Date.now())}) // called once


Similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1104064
This is reproducible for me. Putting this under Core:Dom so dev can look into it.
Component: Untriaged → DOM
Product: Firefox → Core
This seems intentional.  I traced it back as far as bug 918345, but it seems doubtful that introduced it.
At first glance it does look like bug 918345 might have changed the semantics...  Certainly nightlies from 2013-10-27 don't show the "called once" behavior... though neither do ones from 2013-10-31.
Looks like bug 918345 added the new codepaths but did not enable them by default.  That happened for the first time in bug 789261.

That said, if I take the first nightly after bug 1005966 lands (not showing this issue) and flip the pref, it still does not show the issue afaict.
Both chrome and edge fire repeated intervals when the argument is left off.  I haven't tested safari.  We should probably remove the code in IsInterval() and just treat a missing arg as zero.
Priority: -- → P2
farre, do you think you had time for this?
Flags: needinfo?(afarre)
Assignee: nobody → afarre
Flags: needinfo?(afarre)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee3dfdfe6c057e983160430427870e48951fc8ad

Easy enough to fix. The spec glances over this, so I filed a spec bug to ask for clarification[1]

[1] https://github.com/whatwg/html/issues/3664
Attachment #8972849 - Flags: review?(bkelly)
Scratch that. The spec is actually very clear, default timeout value for setInterval is 0: https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope-mixin:dom-settimeout
Comment on attachment 8972849 [details] [diff] [review]
0001-Bug-1450066-Fall-back-to-0-if-setInterval-interval-n.patch

I'll update the WindowOrWorkerGlobalScope idl to have default arguments for setInterval.
Attachment #8972849 - Flags: review?(bkelly) → review-
Comment on attachment 8972850 [details] [diff] [review]
0002-Bug-1450066-Test-that-setInterval-handles-missing-in.patch

Review of attachment 8972850 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/webappapis/timers/missing-timeout-setinterval.any.js
@@ +1,4 @@
> +function timeout_trampoline(t, timeout, message) {
> +  t.step_timeout(function() {
> +    // Yield in case we managed to be called before the second interval callback.
> +    t.step_timeout(function() {

Can you explain why we need two timeouts here?

@@ +10,5 @@
> +async_test(function(t) {
> +  let ctr = 0;
> +  let h = setInterval(t.step_func(function() {
> +    if (++ctr == 2) {
> +      clearTimeout(h);

Shouldn't these be clearInterval()?
Attachment #8972850 - Flags: review?(bkelly) → review+
Comment on attachment 8972865 [details] [diff] [review]
0001-Bug-1450066-Fall-back-to-0-if-setInterval-interval-n.patch

Review of attachment 8972865 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8972865 - Flags: review?(bkelly) → review+
s/clearTimeout/clearInterval/

Avoid using the fact that clearTimeout is interchangeable with clearInterval, it's confusing.

Carrying over r+.
Attachment #8972850 - Attachment is obsolete: true
Attachment #8973165 - Flags: review+
Keywords: checkin-needed
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1695371beb
Fall back to 0 if setInterval interval not supplied. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9577b0cdcad5
Test that setInterval handles missing interval argument. r=bkelly
Keywords: checkin-needed
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10856 for changes under testing/web-platform/tests
https://hg.mozilla.org/mozilla-central/rev/af1695371beb
https://hg.mozilla.org/mozilla-central/rev/9577b0cdcad5
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Upstream PR merged
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.