Closed Bug 391470 Opened 18 years ago Closed 18 years ago

setTimeout("setTimeout(test, 1000);", 2000) is no longer working (and setInterval)

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: moz, Assigned: mrbkap)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 2 obsolete files)

testcase
239 bytes, text/html
Details
Test case -2 (Calling of settimeout is moved to onLoad script)
459 bytes, text/html
Details
testcase 4 (uses setInterval)
517 bytes, text/html
Details
So like this?
2.11 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
What I'm about to check in
2.12 KB, patch
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080705 Minefield/3.0a8pre Build Identifier: This code is stupid, but used to work. It's not working on trunk. setTimeout("setTimeout(test, 1000);", 2000) Reproducible: Always
Keywords: regression, testcase
Attached file testcase —
Version: unspecified → Trunk
When test case of comment #1, next error is issued. (Firefox trunk 2007/6/18 build & 2007/8/08 build, MS Win-2K) > Error: uncaught exception: [Exception... "Component returned failure code: > 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMJSWindow.setTimeout]" nsresult: > "0x8007000e (NS_ERROR_OUT_OF_MEMORY)" location: "JS frame :: :: <TOP_LEVEL> :: > line 0" data: no]
Calling of setTimeout is moved from script in <head>(no <head> & no <body> in original case, then script is placed in <head>) to onLoad script. No problem when onLoad script. Result of that setTimeout before onLoad is prohibited?
Attached file testcase 3 (obsolete) —
No, this test uses onLoad, but fails. this fails: setTimeout("setTimeout(test, 1000);", 2000) this works: setTimeout("setTimeout("test()", 1000);", 2000)
> this works: setTimeout("setTimeout("test()", 1000);", 2000) should read: setTimeout("setTimeout('test()', 1000);", 2000) (this code is used by test case 2, so it works) j.j.
Relates to issues being discussed in Bug 302389? (1) setTimeout("setTimeout(test, 1000);", 2000) 1. string of "setTimeout(test, 1000);" is evaluated after 2000msec 2. function object of test is searched and tried to execute after 1000msec (2) setTimeout("setTimeout('test()', 1000);", 2000) 1. string of "setTimeout('test()', 1000);" is evaluated after 2000msec 2. string of 'test()' is evaluated after 1000msec
Execution context is probably related. http://developer.mozilla.org/en/docs/DOM:window.setTimeout > Syntax > timeoutID = window.setTimeout(func, delay[, param1, param2, ...]); > timeoutID = window.setTimeout(code, delay); > The 'this' problem > Code executed by setTimeout() is run in a separate execution context to the > function from which it was called. As a consequence, the this keyword for the > called function will be set to the window (or global) object, it will not be > the same as the this value for the function that called setTimeout. > This issue is explained in more detail in the JavaScript reference.
Summary: setTimeout("setTimeout(test, 1000);", 2000) is no longer working → setTimeout("setTimeout(test, 1000);", 2000) is no longer working (and setInterval)
Attached file testcase 4 (uses setInterval) (obsolete) —
Attachment #275922 - Attachment is obsolete: true
I can confirm testcase 4. uncaught exception: [Exception... "Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMJSWindow.setTimeout]" nsresult: "0x8007000e (NS_ERROR_OUT_OF_MEMORY)" location: "JS frame :: :: <TOP_LEVEL> :: line 0" data: no]
Flags: blocking1.9?
Comment on attachment 278338 [details] testcase 4 (uses setInterval) Note This works: setInterval ("document.bgColor='gold'; setTimeout('test()', 2000);", 4000); This fails: setInterval ("document.bgColor='gold'; setTimeout(test, 2000);", 4000);
(same functionality, stupid markup error fixed)
Attachment #278338 - Attachment is obsolete: true
This regressed between 2006-06-11 and 2006-06-15: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-06-11+04&maxdate=2006-06-15+09&cvsroot=%2Fcvsroot
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: DOM: Level 0 → JavaScript Engine
Ever confirmed: true
QA Contact: general → general
Bug 255942 (DOM agnostic) landed in that range, it could be related... Given that this is a setTimeout bug I doubt it's a bug in the JS engine itself.
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
As far as I can tell the problem is that the mFileName/mLineNo code in nsJSTimeoutHandler is confused. To be more precise, those are only _set_ when we have mFunObj, but are only _used_ when we don't (see nsGlobalWindow::RunTimeout). And failure to set them is treated as out-of-memory. So in this case, the string timeout doesn't set those (because the relevant code is in the |if(!expr && funobj)| branch in nsJSScriptTimeoutHandler::Init), then we run that timeout, have a JSScript with an empty filename, try to set a function timeout from it, try to get the filename, get the empty string, and bail. I think the right fix is to move the filename/linenumber code to the |if(expr)| branch and probably to get rid of the error return, since it _is_ possible to have a JS script with an empty filename. Mark, is that what you meant to do? Or am I missing something? Oh, and all the relevant stuff (esp nsIScriptTimeoutHandler) needs to document that this getter does NOT allocate the string, in spite of appearances to the contrary.
Flags: blocking1.9? → blocking1.9+
Attached patch So like this? — — Splinter Review
I'd actually come to the same conclusions as bz, but when I came to the bug, I saw the he'd posted his comment (whose corresponding mail I'm still waiting for). This patch addresses the bug here and adds a comment about the lifetime of the filename returned from nsIScriptTimeoutHandler::GetLocation.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #279390 - Flags: superreview?(bzbarsky)
Attachment #279390 - Flags: review?(bzbarsky)
Attachment #279390 - Attachment is patch: true
Attachment #279390 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 279390 [details] [diff] [review] So like this? >+++ b/dom/public/nsIScriptTimeoutHandler.h >+ // Note: The memory pointed to by aFileName is owned by the JS engine >+ // and should not be freed by the caller. Except this is a language-agnostic interface. So perhaps "is owned by the nsIScriptTimeoutHandler" (which is more correct anyway, and makes it clearer that it should not be used after the nsIScriptTimeoutHandler goes away. r+sr=bzbarsky with that.
Attachment #279390 - Flags: superreview?(bzbarsky)
Attachment #279390 - Flags: superreview+
Attachment #279390 - Flags: review?(bzbarsky)
Attachment #279390 - Flags: review+
Sorry about that - the patch looks correct to me.
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 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: