"Useless setTimeout" calls should warn, not throw

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Gabriel Walt, Assigned: mrbkap)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

12 years ago
Component: General → DOM: Events
Product: Firefox → Core
Version: unspecified → Trunk

Comment 1

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

12 years ago
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);
Summary: dom events don't get stopped with stopPropagation() when using setTimeout() with closure in event handling function → "Useless setTimeout" calls should warn, not throw

Comment 3

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

Comment 4

12 years ago
*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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID

Comment 5

12 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---

Comment 6

12 years ago
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?
Assignee: nobody → general
Component: DOM: Events → DOM
OS: Linux → All
QA Contact: general → ian
Hardware: PC → All
(Assignee)

Comment 7

12 years ago
Created attachment 203984 [details] [diff] [review]
Make it a warning

I think this patch, along with the patch for bug 317476 fixes all of the problems this bug covers.
Assignee: general → mrbkap
Status: UNCONFIRMED → ASSIGNED
Attachment #203984 - Flags: review?(jst)
(Assignee)

Comment 8

12 years ago
Created attachment 204000 [details]
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.
Attachment #203984 - Attachment is obsolete: true
Attachment #203984 - Flags: review?(jst)
(Assignee)

Updated

12 years ago
Depends on: 317476
(Assignee)

Comment 9

12 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.