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)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: moz, Assigned: mrbkap)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files, 2 obsolete files)

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: 17 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.