The default bug view has changed. See this FAQ.

setTimeout/setInterval "lateness" argument breaks expected behavior

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Robert Kieffer, Assigned: bz)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla13
addon-compat, dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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
So it seems like you're asking for the decision made in bug 10637 (WONTFIX) to be reconsidered?
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
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.

Updated

9 years ago
Duplicate of this bug: 430771

Updated

9 years ago
Duplicate of this bug: 443391

Comment 5

9 years ago
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? 

Updated

9 years ago
Keywords: dev-doc-needed

Updated

8 years ago
Duplicate of this bug: 512259
Confirming this bug as suggested by Brendan in bug 394769.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

8 years ago
Keywords: dev-doc-needed
Err, I mean in bug 512259.
Version: unspecified → Trunk

Comment 9

8 years ago
Created attachment 396879 [details]
Web Worker Testcase (HTML file)

This behavior appears to be happening in Web Worker timeouts as well.

Comment 10

8 years ago
Created attachment 396882 [details]
Web Worker Testcase (JavaScript file)

Comment 11

8 years ago
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
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #396896 - Flags: review?(jonas)
Comment on attachment 396896 [details] [diff] [review]
Patch v1

Over to Ben as he wrote this code.
Attachment #396896 - Flags: review?(jonas) → review?(bent.mozilla)
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.
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.
Attachment #396896 - Flags: review?(bent.mozilla)
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.
Seemed to me like that decision was made in bug 512259 comment 4.
Attachment #396896 - Flags: review?(jst)
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.
Attachment #396896 - Flags: superreview+
Attachment #396896 - Flags: review?(jst)
Attachment #396896 - Flags: review+
(Reporter)

Comment 18

8 years ago
@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.

Updated

8 years ago
Duplicate of this bug: 512838
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
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

8 years ago
Created attachment 398816 [details] [diff] [review]
Patch v2
Attachment #396896 - Attachment is obsolete: true
Attachment #398816 - Flags: superreview?(jst)
Attachment #398816 - Flags: review?(jst)

Comment 23

8 years ago
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.
Flags: in-testsuite?
(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

8 years ago
(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.
(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 on attachment 398816 [details] [diff] [review]
Patch v2

This regresses bug 430925.
Attachment #398816 - Flags: review-

Comment 28

8 years ago
(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?)
(Reporter)

Comment 29

8 years ago
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.)
(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

8 years ago
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

8 years ago
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" );
(Reporter)

Comment 33

8 years ago
(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?
The difference I've seen was about 10ms.
(Reporter)

Comment 35

8 years ago
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.
Attachment #398848 - Attachment is obsolete: true

Comment 36

8 years ago
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.
Attachment #398816 - Attachment is obsolete: true
Attachment #401720 - Flags: review?(jst)
Attachment #398816 - Flags: superreview?(jst)
Attachment #398816 - Flags: review?(jst)
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.
Attachment #401720 - Flags: review?(jst) → review-

Comment 38

8 years ago
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.

Updated

8 years ago
Assignee: wesongathedeveloper → nobody
Status: ASSIGNED → NEW

Comment 39

7 years ago
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.
Assignee: nobody → wesongathedeveloper
Attachment #401720 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #416301 - Flags: review?(jst)

Comment 40

7 years ago
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
Summary: setTimeout "lateness" argument breaks expected behavior → setTimeout/setInterval "lateness" argument breaks expected behavior

Updated

7 years ago
Duplicate of this bug: 557073
Duplicate of this bug: 645385
Duplicate of this bug: 10637

Updated

6 years ago
Attachment #416301 - Flags: review?(jst)

Updated

6 years ago
Assignee: wesongathedeveloper → nobody
(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.
Status: ASSIGNED → NEW

Updated

6 years ago
Attachment #398816 - Attachment is obsolete: false
Attachment #398816 - Flags: review-

Updated

6 years ago
Attachment #416301 - Attachment is obsolete: true
The lateness arg has been removed from worker timeouts too, in bug 649537.

Updated

6 years ago
Whiteboard: needs up-to-date patch

Updated

6 years ago
Duplicate of this bug: 682841
Duplicate of this bug: 299476
(Reporter)

Comment 48

5 years ago
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!)
Well, a good start is having a working patch.  The one in this bug doesn't apply, and when merged doesn't work right....
Created attachment 595115 [details] [diff] [review]
Remove the additional lateness argument for timeouts and intervals.
Attachment #595115 - Flags: review?(jst)
Assignee: nobody → bzbarsky
Whiteboard: needs up-to-date patch → [need review]
Attachment #398816 - Attachment is obsolete: true
Comment on attachment 595115 [details] [diff] [review]
Remove the additional lateness argument for timeouts and intervals.

r=jst
Attachment #595115 - Flags: review?(jst) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/9bbda25076bf

If all goes to plan, this should ship in a Firefox release on June 5.
Flags: in-testsuite? → in-testsuite+
Keywords: addon-compat, dev-doc-needed
Whiteboard: [need review]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/9bbda25076bf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 54

5 years ago
Updated:
https://developer.mozilla.org/en/DOM/window.setTimeout
https://developer.mozilla.org/en/DOM/window.setInterval
Also updated https://developer.mozilla.org/en/Firefox_13_for_developers#section_6
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Duplicate of this bug: 749489
You need to log in before you can comment on or make changes to this bug.