Closed Bug 394769 Opened 17 years ago Closed 13 years ago

setTimeout/setInterval "lateness" argument breaks expected behavior

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: robert, Assigned: bzbarsky)

References

()

Details

(Keywords: addon-compat, dev-doc-complete)

Attachments

(5 files, 5 obsolete files)

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.
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?
Keywords: dev-doc-needed
Confirming this bug as suggested by Brendan in bug 394769.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Err, I mean in bug 512259.
Version: unspecified → Trunk
Attached file Web Worker Testcase (HTML file) (deleted) —
This behavior appears to be happening in Web Worker timeouts as well.
Attached file Web Worker Testcase (JavaScript file) (deleted) —
Attached patch Patch v1 (obsolete) (deleted) — Splinter Review
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.
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.
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+
@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.
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.
Attached patch Patch v2 (obsolete) (deleted) — Splinter Review
Attachment #396896 - Attachment is obsolete: true
Attachment #398816 - Flags: superreview?(jst)
Attachment #398816 - Flags: review?(jst)
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.
(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-
(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?)
> > > 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.
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 );
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" );
(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.
(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
Attached patch Patch (obsolete) (deleted) — Splinter Review
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-
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.
Assignee: wesongathedeveloper → nobody
Status: ASSIGNED → NEW
Attached patch Patch v3 (obsolete) (deleted) — Splinter Review
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)
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
Attachment #416301 - Flags: review?(jst)
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
Attachment #398816 - Attachment is obsolete: false
Attachment #398816 - Flags: review-
Attachment #416301 - Attachment is obsolete: true
The lateness arg has been removed from worker timeouts too, in bug 649537.
Whiteboard: needs up-to-date patch
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....
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+
Whiteboard: [need review]
Target Milestone: --- → mozilla13
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: