973849 - clearTimeout / clearInterval now throw when called without arguments

Status: RESOLVED FIXED

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

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

Firefox automatically upgraded from 27.0.0 to 27.0.1 and now the second step of complicated javascript on http://thegatheringinlittlerock.com/ will no longer run in canvas.


Actual results:

First step of js shows the loading clock, but when the site is loaded and runs, the canvas "growing tree" no longer functions.


Expected results:

The tree should have grown in canvas as it did with previous version...

Comment 1

[Alice0775 White]

An error shown in Error console:

Error: TypeError: Not enough arguments to Window.clearTimeout.
Source File: http://thegatheringinlittlerock.com/js/gathering.js
Line: 348

Regression window:
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5717dfc80960
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131026184508
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/508288a2b62c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131027021707
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5717dfc80960&tochange=508288a2b62c
Regressed by:
508288a2b62c	Peter Van der Beken — Bug 918345 - Turn on WebIDL binding generation for Window and hook it up to quickstubs. r=bz.

Comment 2

[Masatoshi Kimura [:emk]]

Chrome didn't throw, but IE11 throws like our new behavior.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The relevant line in the script is:

	clearTimeout();

The old (XPCOM) implementation explicitly listed the argument as optional here, and ignored the no-op call if no argument was passed.  Wish we'd had tests for that bit..

Our new behavior is following the spec, like IE11 is, but maybe the spec just needs changing here.

As data, I just tested the IE behavior on http://fiddle.jshell.net/DXPN4/1/show/ and a no-arguments call to clearTimeout is a no-op in IE8, but throws in IE9+ in standards mode.  Presumably this site was never tested in IE.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

I assume, btw, that the page meant to call "timeoutClear", not "clearTimeout", on that line.  But of course that wouldn't do much good, since that would only clear one timeout, not all 10 it wants to clear.  So this page is generally just pretty broken in terms of its timeout handling...

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

If we're following the current spec, and the page is broken in handling timeouts, this doesn't look like a release-blocker (or even a valid bug).

Comment 7

[Boris Zbarsky [:bzbarsky]]

This is breaking other sites too.  I've filed a spec bug at https://www.w3.org/Bugs/Public/show_bug.cgi?id=24778 and I think we should revert this change on our end, though probably not for 27 at this point.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Allow clearInterval/clearTimeout calls with no arguments; just treat them as no-ops.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8380242 [details] [diff] [review]
Allow clearInterval/clearTimeout calls with no arguments; just treat them as no-ops.

ah, we special case 0 in the implementation.

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab395abf96b9

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8380242 [details] [diff] [review]
Allow clearInterval/clearTimeout calls with no arguments; just treat them as no-ops.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 918345 
User impact if declined: Some websites are broken
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): Low risk.  Just restores
   the Firefox 26 behavior for clearInterval/clearTimeout.
String or IDL/UUID changes made by this patch:  None

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/016b9229193e because I wasn't bright enough to figure out whether it was this or bug 972312 causing Windows builds to say https://tbpl.mozilla.org/php/getParsedLog.php?id=35109146&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=35109059&tree=Mozilla-Inbound

Comment 13

[Boris Zbarsky [:bzbarsky]]

It was the other bug.  Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d22d19bcd2f

Comment 14

[Sylvestre Ledru [:Sylvestre]]

I will wait for the patch in m-c to approve the uplift

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6d22d19bcd2f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c92d1386c0ae

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c336953de579

Comment 18

[u279076]

Deprioritizing this for QA testing given in-testsuite coverage. Please remove the [qa-] whiteboard tag and add the verifyme keyword if this needs manual testing before release.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c336953de579