Closed
Bug 391470
Opened 17 years ago
Closed 17 years ago
setTimeout("setTimeout(test, 1000);", 2000) is no longer working (and setInterval)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: moz, Assigned: mrbkap)
References
Details
(Keywords: regression, testcase)
Attachments
(5 files, 2 obsolete files)
239 bytes,
text/html
|
Details | |
459 bytes,
text/html
|
Details | |
517 bytes,
text/html
|
Details | |
2.11 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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
Comment 2•17 years ago
|
||
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]
Comment 3•17 years ago
|
||
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?
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.
Comment 6•17 years ago
|
||
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
Comment 7•17 years ago
|
||
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)
Attachment #275922 -
Attachment is obsolete: true
Comment 9•17 years ago
|
||
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?
Reporter | ||
Comment 10•17 years ago
|
||
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);
Reporter | ||
Comment 11•17 years ago
|
||
(same functionality, stupid markup error fixed)
Attachment #278338 -
Attachment is obsolete: true
Comment 12•17 years ago
|
||
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
Comment 13•17 years ago
|
||
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
Updated•17 years ago
|
Blocks: dom-agnostic
Comment 14•17 years ago
|
||
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+
Assignee | ||
Comment 15•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Attachment #279390 -
Attachment is patch: true
Attachment #279390 -
Attachment mime type: application/octet-stream → text/plain
Comment 16•17 years ago
|
||
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+
Comment 17•17 years ago
|
||
Sorry about that - the patch looks correct to me.
Assignee | ||
Comment 18•17 years ago
|
||
Assignee | ||
Comment 19•17 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•