Last Comment Bug 394769 - setTimeout/setInterval "lateness" argument breaks expected behavior
: setTimeout/setInterval "lateness" argument breaks expected behavior
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla13
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
https://bugzilla.mozilla.org/show_bug...
: 10637 299476 430771 443391 512259 512838 557073 645385 682841 749489 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-03 10:10 PDT by Robert Kieffer
Modified: 2012-04-27 07:38 PDT (History)
30 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Web Worker Testcase (HTML file) (203 bytes, text/html)
2009-08-26 15:43 PDT, Saint Wesonga
no flags Details
Web Worker Testcase (JavaScript file) (326 bytes, text/x-c)
2009-08-26 15:53 PDT, Saint Wesonga
no flags Details
Patch v1 (6.21 KB, patch)
2009-08-26 16:25 PDT, Saint Wesonga
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Patch v2 (7.10 KB, patch)
2009-09-04 20:46 PDT, Saint Wesonga
no flags Details | Diff | Splinter Review
Example showing javascript-only, cross-browser method for supporting 'lateness' (1.28 KB, text/html)
2009-09-05 07:32 PDT, Robert Kieffer
no flags Details
Example showing javascript-only, cross-browser method for supporting 'lateness' (3.29 KB, text/html)
2009-09-07 14:29 PDT, Robert Kieffer
no flags Details
Patch (24.14 KB, patch)
2009-09-20 10:28 PDT, Saint Wesonga
jst: review-
Details | Diff | Splinter Review
Simple workaround - Standardizes Mozilla's setTimeout while keeping lateness information available (1.60 KB, text/html)
2009-09-22 01:44 PDT, Jordan Osete
no flags Details
Patch v3 (13.12 KB, patch)
2009-12-05 16:52 PST, Saint Wesonga
no flags Details | Diff | Splinter Review
Remove the additional lateness argument for timeouts and intervals. (9.02 KB, patch)
2012-02-07 11:41 PST, Boris Zbarsky [:bz]
jst: review+
Details | Diff | Splinter Review

Description Robert Kieffer 2007-09-03 10:10:02 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20061201 Firefox/2.0.0.6 (Ubuntu-feisty)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20061201 Firefox/2.0.0.6 (Ubuntu-feisty)

Functions invoked by setTimeout are passed an extra, "lateness" argument in Mozilla.  This is documented in Bug 10637, which was marked as "WONTFIX" seven years ago.

However as recently as last year, people are still filing bugs against this "behavior".  Without documentation - and this continues to be undocumented - the perceived behavior is that setTimeout is being passed a random integer argument, which I think most people rightly interpret as a bug.

The lateness argument is completely counter-intuitive.  It is not documented, not supported in other browsers, and not part of any standard.  As a result, this breaks any case where developers give special meaning to lack of an argument. For example:

  /**
   * Set/reset the busy state
   */
  function setBusy(flag) {
    if (flag) {
      // Show a busy indicator
      ...
    } else {
      // Hide the indicator
      ...
    }
  }

  // Reset the busy state after we finish processing
  setTimeout(setBusy, 100);

The code above will always "show a busy indicator" on mozilla, while other platforms will "hide the indicator".


Reproducible: Always
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-09-03 10:37:47 PDT
So it seems like you're asking for the decision made in bug 10637 (WONTFIX) to be reconsidered?
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2007-09-04 13:13:48 PDT
Given that there hasn't been that many bugs filed on this, i'd rather not remove the feature, as that might cause bigger problems. I'd instead recommend that we actually document this and get it into the appropriate specs.
Comment 3 Aiko 2008-04-25 13:11:09 PDT
*** Bug 430771 has been marked as a duplicate of this bug. ***
Comment 4 Artemy Tregubenko 2008-07-03 05:45:35 PDT
*** Bug 443391 has been marked as a duplicate of this bug. ***
Comment 5 Artemy Tregubenko 2008-07-03 05:50:51 PDT
I've run into this behaviour today too, and also consider it a bug. I believe there won't be any bigger problems, because anyway this doesn't work in all other browsers. 

Note that 9 years passed and not a word was added to documentation of mozilla itself. How many years would it take to promote that change to new spec, and implement it in other browsers, and replace old inconsistent browsers? 
Comment 6 Jonathan Watt [:jwatt] 2009-08-25 11:06:18 PDT
*** Bug 512259 has been marked as a duplicate of this bug. ***
Comment 7 Jonathan Watt [:jwatt] 2009-08-25 11:07:10 PDT
Confirming this bug as suggested by Brendan in bug 394769.
Comment 8 Jonathan Watt [:jwatt] 2009-08-25 11:09:21 PDT
Err, I mean in bug 512259.
Comment 9 Saint Wesonga 2009-08-26 15:43:05 PDT
Created attachment 396879 [details]
Web Worker Testcase (HTML file)

This behavior appears to be happening in Web Worker timeouts as well.
Comment 10 Saint Wesonga 2009-08-26 15:53:42 PDT
Created attachment 396882 [details]
Web Worker Testcase (JavaScript file)
Comment 11 Saint Wesonga 2009-08-26 16:25:45 PDT
Created attachment 396896 [details] [diff] [review]
Patch v1

What's the correct action to take on the line with the NS_CreateJSArgv call? A zero sized array doesn't seem sensible
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2009-08-26 16:35:58 PDT
Comment on attachment 396896 [details] [diff] [review]
Patch v1

Over to Ben as he wrote this code.
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2009-08-26 16:50:43 PDT
Wait a sec... We're removing the lateness arg to timeouts?! Sicking is correct that I wrote this code for workers (to emulate what we do on normal pages) but I can't make the decision to remove it.

This needs some serious consideration from sicking, jst, and others.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2009-08-26 17:01:36 PDT
Err, yeah, so there's lots of things that are weird here.

First of all, this bug as filed is not about workers at all. In fact, it was filed before workers weren't even thought of.

Second, there is a spec here, which we presumably should follow. However it turns out that we don't actually follow the spec. But neither does this patch. And there's some discussion regarding if the spec needs to be tweaked.

See bug 512645 for some discussion.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2009-08-26 17:35:33 PDT
Urg, sorry, ignore my previous comment. I misunderstood Ben's comment.

I don't know who will make the decision to remove this argument. I don't feel strongly either way.

Do see comment 2 as well.
Comment 16 Jonathan Watt [:jwatt] 2009-08-26 17:45:38 PDT
Seemed to me like that decision was made in bug 512259 comment 4.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-26 18:29:14 PDT
Comment on attachment 396896 [details] [diff] [review]
Patch v1

- In dom/base/nsIScriptTimeoutHandler.h:

-
-  // Set the "secret" final lateness arg.  This will be called before
-  // GetArgv(), which should reflect this lateness value.
-  virtual void SetLateness(PRIntervalTime aHowLate) = 0;

Since this interface is changing, it should get a new IID (i.e. NS_ISCRIPTTIMEOUTHANDLER_IID needs to be a new UUID).

r+sr=jst with that.
Comment 18 Robert Kieffer 2009-08-26 18:35:57 PDT
@jst comment 17 : Hrm, does that mean there's been a decision to fix this?  From the outside it seems a little odd that there's still a question about this if /be has blessed it. (i.e, "what comment 16 said")

Am I correct in interpreting the HTML5 spec ( http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#windowtimers) to mean that the 'lateness' argument makes the Mozilla implementation non-compliant?  (the only args a handler gets should be those passed as additional args to setTimeout, right?)

Finally, if there's still some debate here, perhaps we could "test the waters" by removing 'lateness' in the next alpha/beta candidate to see if anyone complains about it's absence.  If nothing else, that would at least give us concrete examples of why it's needed, and people who care about it's survival to debate with.
Comment 19 Blake Kaplan (:mrbkap) 2009-08-27 07:52:24 PDT
*** Bug 512838 has been marked as a duplicate of this bug. ***
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-08-27 11:01:35 PDT
We have code in toolkit that makes use of this argument:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbox.xml#229
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/notification.xml#253
Comment 21 Dão Gottwald [:dao] 2009-08-27 12:52:49 PDT
Also http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#745

And I wouldn't be surprised if jQuery used this for animations, as John Resig blogged about it last year.

This shouldn't be removed without a suitable replacement.
Comment 22 Saint Wesonga 2009-09-04 20:46:37 PDT
Created attachment 398816 [details] [diff] [review]
Patch v2
Comment 23 Saint Wesonga 2009-09-04 20:54:47 PDT
In response to comment 20 & comment 21: Aren't these basic optimizations that take advantage of the argument (rather than required logic)?

As for usage in jQuery mentioned in comment 21 - it seems jQuery would still have to account for all other browsers, which do not support this argument. Therefore this shouldn't be an issue.
Comment 24 Dão Gottwald [:dao] 2009-09-05 03:01:22 PDT
(In reply to comment #23)
> In response to comment 20 & comment 21: Aren't these basic optimizations that
> take advantage of the argument

Yes. What's your point?

> As for usage in jQuery mentioned in comment 21 - it seems jQuery would still
> have to account for all other browsers, which do not support this argument.
> Therefore this shouldn't be an issue.

If this argument allows web content to perform better, then removing it without substitution is an issue.
Comment 25 David Allsopp 2009-09-05 03:50:09 PDT
(In reply to Comment #24)

> > In response to comment 20 & comment 21: Aren't these basic optimizations
> > that take advantage of the argument
>
> Yes. What's your point?

I imagine it's that removing the feature doesn't mean that the algorithm can no longer be implemented - it will just be slower.

> > As for usage in jQuery mentioned in comment 21 - it seems jQuery would still
> > have to account for all other browsers, which do not support this argument.
> > Therefore this shouldn't be an issue.
>
> If this argument allows web content to perform better, then removing it
> without substitution is an issue.

However, time would suggest that there's no risk of this argument ever being standardised and surely standards-compliancy is the more important and worthy goal? Perhaps a Firefox-specific separate version of setTimeout would be better (either another function called something like setTimeoutWithDelay or a third optional boolean parameter to setTimeout and setInterval which indicates that the extra argument to the function is wanted). That way, jQuery specifically *activates* a Firefox optimisation for Firefox rather than having to specifically *disable* it for all other browsers.
Comment 26 Dão Gottwald [:dao] 2009-09-05 03:54:58 PDT
(In reply to comment #25)
> I imagine it's that removing the feature doesn't mean that the algorithm can no
> longer be implemented - it will just be slower.

We don't want it to be slower.

> (either another function called something like setTimeoutWithDelay

Right, so you're suggesting a substitution. That wouldn't be standards-compliant either, but anyway, the argument shouldn't be removed without that substitution being in place.

> or a third
> optional boolean parameter to setTimeout and setInterval which indicates that
> the extra argument to the function is wanted).

setTimeout and setInterval already take three or more arguments.
Comment 27 Dão Gottwald [:dao] 2009-09-05 03:55:39 PDT
Comment on attachment 398816 [details] [diff] [review]
Patch v2

This regresses bug 430925.
Comment 28 David Allsopp 2009-09-05 04:15:28 PDT
(In reply to comment #26)
> (In reply to comment #25)
> > I imagine it's that removing the feature doesn't mean that the algorithm
> > can no longer be implemented - it will just be slower.
> We don't want it to be slower.

Agreed - but it's a very different scenario from not working at all.

> > (either another function called something like setTimeoutWithDelay
> Right, so you're suggesting a substitution. That wouldn't be
> standards-compliant either, 

But it's infinitely better than the present scenario - an undocumented piece of very confusing non-standard behaviour that actively breaks code written for a standards-compliant browser in a way that could never itself be adopted. At least having an alternate way of invoking the behaviour is something which could conceivably be standardised in the future.

> but anyway, the argument shouldn't be removed without that substitution
> being in place.
Agreed.

> > or a third optional boolean parameter to setTimeout and setInterval which
> > indicates that the extra argument to the function is wanted).
> setTimeout and setInterval already take three or more arguments.

I didn't realise this had changed in HTML 5 (or has it had that before then?)
Comment 29 Robert Kieffer 2009-09-05 07:32:05 PDT
Created attachment 398848 [details]
Example showing javascript-only, cross-browser method for supporting 'lateness'

> > > or a third optional boolean parameter to setTimeout and setInterval which
> > > indicates that the extra argument to the function is wanted).
> > setTimeout and setInterval already take three or more arguments.
> 
> I didn't realise this had changed in HTML 5 (or has it had that before then?)

It hasn't changed. These methods take only two meaningful arguments.  The spec specifies:

  handle = window.setTimeout(handler [, timeout [, arguments ]])
  handle = window.setInterval(handler [, timeout [, arguments ]])

Where, "Any _arguments_ are passed straight through to the handler".  See http://www.w3.org/TR/html5/browsers.html#timers

Adding a third argument that controls the presence of lateness would break this signature.  
And to repeat my previous question, is it fair to say that this also implies handler functions should expect to receive _arguments_, and only _arguments_?  i.e. the current "lateness" argument makes Firefox non-compliant?

Finally, is built-in support for 'lateness' actually necessary?  It's not _that_ hard to implement using simple, cross-browser, javascript.  See the attached example.  3rd party code could easily use this approach, or something similar, to add support where needed (with the added benefit that it would work consistently across browsers.)
Comment 30 Dão Gottwald [:dao] 2009-09-05 07:42:21 PDT
(In reply to comment #29)
> Created an attachment (id=398848) [details]
> Example showing javascript-only, cross-browser method for supporting 'lateness'

This seems to be always higher than the native lateness, probably because using Date isn't free and maybe some other overhead.
Comment 31 Jordan Osete 2009-09-07 03:41:30 PDT
Couldn't we just replace the lateness arg with a global getCurrentTimeoutLateness function ? This way timeouts and intervals would be consistent across browsers, and the functionnality is still there for those who need it.

setTimeout( function(){
  var lateness = getCurrentTimeoutLateness();
  alert( "arguments.length is " + arguments.length + "\n" +
    "we are late by " + lateness + "ms" );  //alerts "1"
}, 500 );
Comment 32 Jordan Osete 2009-09-07 03:45:42 PDT
Whoops, the code in my previous comment is incorrect, sorry. Here's the correct one:

setTimeout( function(){
  var lateness = getCurrentTimeoutLateness();
  alert( "arguments.length is " + arguments.length + "\n" + //alerts "1"
    "we are late by " + lateness + "ms" );
}, 500, "foo" );
Comment 33 Robert Kieffer 2009-09-07 06:59:20 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > Created an attachment (id=398848) [details] [details]
> > Example showing javascript-only, cross-browser method for supporting 'lateness'
> 
> This seems to be always higher than the native lateness, probably because using
> Date isn't free and maybe some other overhead.

I'm seeing a very slight variation (0-1ms) between the two.  Which isn't surprising.  My code adds the overhead of a few function calls, the creation of a date instance, a couple array assignments... stuff that will execute in <1-2ms in most browser engines.  But, yeah, there will be a difference because these are different implementations.

But the difference will be consistent (or nearly so).  Just as it is for the native implementation, which has it's own overhead.  Thus I'll argue that for all practical purposes these two implementations are the same.  If you care about the offset introduced by my implementation, then you probably care about the offset of the native implementation, in which case neither is satisfactory.

Do you have a use case where my solution wouldn't suffice?
Comment 34 Dão Gottwald [:dao] 2009-09-07 07:05:09 PDT
The difference I've seen was about 10ms.
Comment 35 Robert Kieffer 2009-09-07 14:29:50 PDT
Created attachment 399116 [details]
Example showing javascript-only, cross-browser method for supporting 'lateness'

(In reply to comment #34)
> The difference I've seen was about 10ms.

After a bit more playing around, I'm able to reproduce similar varitions - mostly by doing things like window resizing to mess with the CPU.  But after doing some investigation, I'm forced to conclude that the problem is *not* with my example code.  Rather, it's in the native implementation.  Here's why ...

First, I've updated my example code to report the lateness provided by both the setTimerWithLateness method and that of the native setInterval/setTimeout methods.  It now also shows running totals of these two values, as well as the absolute "drift" of the timer. (By "drift", I mean the difference in timing between that of a perfect timer and an actual one.  E.g. if a one second timer fires 60 times, and the last call happens at 1:05 instead of 1:00, the "drift" is 5000 ms.)

The result is interesting...

The "lateness" provided by setTimerWithLateness maps nicely to drift.  Just sum lateness and, voila, you have drift.  Nice and logical.

For the native "lateness", this isn't the case.  It adds up to... well, I don't know what... a number that slowly diverges from anything meaningful.  You can see this if you let the example run for a bit and fiddle around with the window size to get a few non-zero lateness values.  The "Sum of lateness - setInterval" value becomes less and less meaningful.

So I'm forced to admit that I don't know what the native "lateness" value is measuring.  I couldn't find it documented anywhere either.  Which has me *seriously* wondering how anyone but the original author of the code, or somebody sitting next to him, would find it useful.

With regard to the accuracy of my setTimerWithLateness implementation, the overhead it introduces is negligable.  To test this, I put together the following JSLitmus test:
  http://broofa.com/Tools/JSLitmus/tests/date.html

Here are the results I get when running this in FF on my MacBook:
  http://tinyurl.com/nt2ud6

The first number, "new Date()" shows Date object creation is a non-issue - they get created at a rate of 2.2M/second.  The second number, "lateness handler overhead," measures the total overhead my example introduces; It shows that intermediate handler method that gets created can be called called 71K/sec.  I.e. the total overhead per timer call is < 0.02ms.

Results in Opera ( http://tinyurl.com/nxk24m ) and Safari ( http://tinyurl.com/n5mrn3 ) show even less overhead.  I have no results for IE or Chrome, but there'd have to be a 50x differnce for it to matter.

Based on this, I'll make the following assertions:
- The current implementation of "lateness" is of marginal utility to all but the inner-most circle of Mozilla developers since there's no concise description of what it's measuring.
- A 3rd-party, javascript alternative, such as the example attachment does not have performance issues
- ... and results in a "lateness" value that is well understood
- ... and works across all browsers.
Comment 36 Saint Wesonga 2009-09-20 10:28:33 PDT
Created attachment 401720 [details] [diff] [review]
Patch

What's the best name for the substitution that mimics the current behavior? I called the new JS function setTimeoutWithDelay but that may or may not be an accurate choice.
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2009-09-21 12:29:45 PDT
Comment on attachment 401720 [details] [diff] [review]
Patch

Unless this get standardized and agreed upon by other browser vendors we are not going to add a new flavor of setTimeout/Interval. r- until then.
Comment 38 Jordan Osete 2009-09-22 01:44:20 PDT
Created attachment 402055 [details]
Simple workaround - Standardizes Mozilla's setTimeout while keeping lateness information available

This is a simple way to standardize setTimeout / setInterval in every browser, while still providing the lateness information if it is available.

As suggested in my previous post, it replaces the additional argument with a global mozGetCurrentTimeoutLanteness function. It is not ideal, but as there is no standard namespace or object for timeouts, it seems there is no choice.

I think it may be better than the various setTimeoutWithLateness() variants, as it does not change the number of arguments that are passed to the handler. Ie the same handler can be used whether there is lateness information available or not (as demonstrated).

I would like this kind of solution to be used in Mozilla, instead of the current non-standard behaviour.
Comment 39 Saint Wesonga 2009-12-05 16:52:52 PST
Created attachment 416301 [details] [diff] [review]
Patch v3

Trying Jordan's approach by making the lateness a property of the timeout (slightly modified such that mozGetTimeoutLateness takes a timer id like clearTimeout). Also note: this patch also removes the equivalent lateness DOM worker code but does NOT replace it with anything else.
Comment 40 Nickolay_Ponomarev 2010-04-04 09:59:38 PDT
I'm adding setInterval to the summary, since it appears this bug is about both. And this is in fact mentioned in the documentation, please update when checking in.

https://developer.mozilla.org/en/DOM/window.setTimeout#.22Lateness.22_argument
https://developer.mozilla.org/en/DOM/window.setInterval#Callback_arguments
Comment 41 Nickolay_Ponomarev 2010-04-04 10:00:25 PDT
*** Bug 557073 has been marked as a duplicate of this bug. ***
Comment 42 Boris Zbarsky [:bz] 2011-03-26 13:08:05 PDT
*** Bug 645385 has been marked as a duplicate of this bug. ***
Comment 43 Boris Zbarsky [:bz] 2011-03-26 13:08:54 PDT
*** Bug 10637 has been marked as a duplicate of this bug. ***
Comment 44 Dão Gottwald [:dao] 2011-06-05 10:52:58 PDT
(In reply to comment #21)
> This shouldn't be removed without a suitable replacement.

FWIW, we now have that replacement with mozRequestAnimationFrame. At this point the lateness argument isn't doing any good and we should just remove it.
Comment 45 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-05 11:52:41 PDT
The lateness arg has been removed from worker timeouts too, in bug 649537.
Comment 46 Dave Garrett 2011-08-29 09:20:49 PDT
*** Bug 682841 has been marked as a duplicate of this bug. ***
Comment 47 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-31 15:30:07 PDT
*** Bug 299476 has been marked as a duplicate of this bug. ***
Comment 48 Robert Kieffer 2012-02-07 06:58:04 PST
When can we expect this fix to make it into FF?

'Just wrote "setTimeout(function() {foo()})" instead of "setTimeout(foo);" for, like, the five-hundredth time since reporting this and... well... it's code stank I'd love to do away with.  (That said, I do appreciate that this is [slowly] getting fixed!)
Comment 49 Boris Zbarsky [:bz] 2012-02-07 11:38:38 PST
Well, a good start is having a working patch.  The one in this bug doesn't apply, and when merged doesn't work right....
Comment 50 Boris Zbarsky [:bz] 2012-02-07 11:41:37 PST
Created attachment 595115 [details] [diff] [review]
Remove the additional lateness argument for timeouts and intervals.
Comment 51 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-07 11:56:52 PST
Comment on attachment 595115 [details] [diff] [review]
Remove the additional lateness argument for timeouts and intervals.

r=jst
Comment 52 Boris Zbarsky [:bz] 2012-02-07 12:30:58 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/9bbda25076bf

If all goes to plan, this should ship in a Firefox release on June 5.
Comment 53 Ed Morley [:emorley] 2012-02-08 09:38:38 PST
https://hg.mozilla.org/mozilla-central/rev/9bbda25076bf
Comment 55 Marek Stępień [:marcoos, inactive] 2012-02-24 12:25:54 PST
Also updated https://developer.mozilla.org/en/Firefox_13_for_developers#section_6
Comment 56 John P Baker 2012-04-27 07:38:56 PDT
*** Bug 749489 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.