304033 - "Useless setTimeout" calls should warn, not throw

Status: RESOLVED FIXED

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

Description

[Gabriel Walt]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050809 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050809 Firefox/1.0+

DOM events don't get stopped with stopPropagation() when there is a setTimeout()
with a closure in the event handling function.

i've specially prepared this page to show the problem:
http://wh.ysagoon.com/diz/tmp/event3.php
and here's a page showing how it should work, the setTimeout has been replaced
by an alert to show the event propagation:
http://wh.ysagoon.com/diz/tmp/event4.php

Reproducible: Always

Comment 1

[Aiko]

Your show function is buggy:

function show(event) {
    function setBack(obj) {
        return setTimeout(function() { obj.style.backgroundColor = "rgb(255,
255, 255)"; }, 1000);
    }

    this.style.backgroundColor = "rgb(255, 0, 0)";
    setTimeout(setBack(this));
    event.stopPropagation();
}

setBack returns the result of setTimeout, which is a number. So your second call
of setTimeout is something like 'setTimeout(2)'. If you check the JavaScript
Console, Firefox complains about the "useless setTimeout call" and terminates
the script. The next line, event.stopPropagation() is never reached.

Comment 2

[Jesse Ruderman]

Safari doesn't terminate scripts that make useless setTimeout calls. Maybe
Firefox also shouldn't. I haven't tested IE6.

Example: javascript:setTimeout(2,2); alert(4);

Comment 3

[Aiko]

For reference: IE6 and Opera 8.02 accept setTimeout(2), Navigator 3.04 and 4.75
don't. I consider NN3 to be the DOM level 0 reference implementation but I'm
probably alone with that point of view.

Comment 4

[Gabriel Walt]

*shame* oO

right! sorry for having waisted your time!! the page works as expected if
replacing setTimeout(setBack(this)); by setBack(this);

the thing worked in konqueror (thus probably also in safari as Jesse says) so i
though it was a bug. and strangely the js error icon of the web developper
toolbar didnt turn red as it usually does to show errors (btw this is really not
normal)! now i know that when in doubt i should't thrust this icon and check the
console anyway...

for stopping scripts making useless calls to setTimeout, i think that a warning
would maybe be more suited since it shows the problem but is stilly compatible
with **** scripts since it doesn't stop the execution.

btw change bug status to INVALID. i hope this is ok since i'm not very used to
this bug tracker.

Comment 5

[Jesse Ruderman]

Reopening.  This bug was invalid as filed, but morphed into "'Useless
setTimeout' calls should warn, not throw", I think it's a useful bug report.

Comment 6

[Aiko]

For me, the real problem here is that this error cannot be catched.

    'try { setTimeout(2) } catch(ex) {}' 

still kills the script. setTimeout should be just like any other DOM method and
throw a catchable exception if it gets unacceptable input parameters, maybe
TYPE_MISMATCH_ERR. 

And can somebody please move this to the right component and Hardware/OS All/All?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make it a warning

I think this patch, along with the patch for bug 317476 fixes all of the problems this bug covers.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcase

From this testcase, it seems that IE throws a catchable error. jst doesn't think that we should shift our behavior away from IE. I'll mark this bug dependent on the JS_ReportError bug, since that's the reason we fail my simple testcase.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I'm marking this bug as FIXED since bug 317476 is now checked in and you can catch the error. The other part of this bug (making it a warning instead an exception) is INVALID.