aaron.peltz Reporter
	Description • 6 years ago

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

	Kanchan Kumari QA
	Comment 1 • 6 years ago

This is reproducible for me. Putting this under Core:Dom so dev can look into it.

	Ben Kelly [:bkelly, not reviewing]
	Comment 2 • 6 years ago

This seems intentional.  I traced it back as far as bug 918345, but it seems doubtful that introduced it.

	Ben Kelly [:bkelly, not reviewing]
	Comment 3 • 6 years ago

Forgot the code link:

https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/dom/base/nsGlobalWindowInner.cpp#6689-6691

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 6 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 6 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 6 • 6 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 6 years ago

farre, do you think you had time for this?

	Andreas Farre [:farre] Assignee
	Comment 8 • 6 years ago

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

	Andreas Farre [:farre] Assignee
	Comment 10 • 6 years ago

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

	Andreas Farre [:farre] Assignee
	Comment 11 • 6 years ago

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.

	Andreas Farre [:farre] Assignee
	Comment 12 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c609836c923d35858b03b44a630cba9b2d39f3

Changed to follow spec: https://html.spec.whatwg.org/multipage/webappapis.html#windoworworkerglobalscope-mixin:dom-settimeout

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 6 years ago

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()?

	Ben Kelly [:bkelly, not reviewing]
	Comment 14 • 6 years ago

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!

	Andreas Farre [:farre] Assignee
	Comment 15 • 6 years ago

s/clearTimeout/clearInterval/

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

Carrying over r+.

	Pulsebot
	Comment 16 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 17 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10856 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 18 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/w3c/web-platform-tests/pull/10856
* continuous-integration/travis-ci/pr (https://travis-ci.org/w3c/web-platform-tests/builds/375081261?utm_source=github_status&utm_medium=notification)

	Natalia Csoregi [:nataliaCs]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/af1695371beb
https://hg.mozilla.org/mozilla-central/rev/9577b0cdcad5

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 20 • 6 years ago

Upstream PR merged