33728 - setTimeout on Function.prototype.apply crashes Mozilla

Status: RESOLVED WORKSFORME

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

Description

[Martijn Pieters]

Calling a the apply method of a Function object via the setTimeout method will
cause Mozilla to crash.

In my application this happend directly, in the attached testcase you have to
run the test twice to cause it. Testcase, instructions and strace output follow.

Comment 1

[Martijn Pieters]

Attachment: Testcase

Comment 2

[Martijn Pieters]

Reproduce:
1/ save attached testcase as chrome/test/content/default/7058.xul
2/ run 'mozilla -chrome chrome://test/content/7058.xul
3/ In the resulting (mini) window, choose Test->Test
4/ The consule prints 'direct'.
5/ Choose Test->Test again
6/ The consule prints 'direct'.
7/ Mozilla crashes.

Expected result:
The console should print 'direct', then 'timeout' and not crash at all.

Attached trace is from strace attached to mozilla between points 2 and 3.

Comment 3

[Martijn Pieters]

Attachment: strace output

Comment 4

[Martijn Pieters]

Above report with recent tip build (yesterday, IIRC) on Linux, with gcc.
Confirmed on WinNT, build 2000032409.

Comment 5

[Mike Shaver (:shaver emeritus)]

Looks like something's getting corrupted in the timer list:



(gdb) frame 0

#0  0x4033ee5b in GlobalWindowImpl::RunTimeout (this=0x83cc830,

    aTimeout=0x8430070) at nsGlobalWindow.cpp:3055

3055	       (0 == timeout->firingDepth)) {

(gdb) print timeout

$3 = (nsTimeoutImpl *) 0xfffff438

(gdb) list

3053	  for(timeout = mTimeouts; timeout; timeout = timeout->next) {

3054	    if(((timeout == aTimeout) || !LL_CMP(timeout->when, >, now)) &&

3055	       (0 == timeout->firingDepth)) {

(gdb) print *mTimeouts

$5 = {ref_count = 2, window = 0x83cc830, expr = 0x0, funobj = 0x8349e78,

timer = 0x842fff0, argv = 0x842fce8, argc = 2, spare = 0, public_id = 2,

interval = 0, when = 860689507, principal = 0x82081d0,

filename = 0x842ffc8 "chrome://test/content/7058.xul", lineno = 4,

version = 0x400e9fb8 "default", firingDepth = 1, next = 0x8430070}

(gdb) print *mTimeouts->next

$6 = {ref_count = 2, window = 0x83cc830, expr = 0x0, funobj = 0x8349e78,

timer = 0x83c1d78, argv = 0x842d698, argc = 2, spare = 0, public_id = 3,

interval = 0, when = 860707115, principal = 0x82081d0,

filename = 0x83c1d50 "chrome://test/content/7058.xul", lineno = 4,

version = 0x400e9fb8 "default", firingDepth = 1, next = 0xbffff718}

(gdb) print aTimeout

$7 = (nsTimeoutImpl *) 0x8430070

(gdb)



->next on the last timer there is pointing into stack space, it seems -- that

can't be right!



Over to you, jst: it doesn't look like a JS-engine bug.

Comment 6

[Martijn Pieters]

FYI: I see this bug also when indirectly calling Function.apply:

  setTimeout(doCall, 1, functionObj, args);

function doCall(f, a) { f.apply(null, a) }

However, full JS functions (non-native, like apply) do not crash Mozilla.

Comment 7

[Martijn Pieters]

Attachment: Updated testcase (including indirect call to function.apply)

Comment 8

[Martijn Pieters]

The new testcase adds setting a timeout for a simple function that uses
Function.apply. This causes a crash as well.

Expected output on console is now:
direct
timeout
timeout indirect

Comment 9

[Martijn Pieters]

Attachment: 3rd version of testcase

Comment 10

[Martijn Pieters]

With a patch from jst that only fixes the crashing part of the bug, I analised a
bit further.

With the 3rd testcase, clicking Test->test executes the following steps:
1/ Fire a timeout of 1 to dummy(). Dummy does nothing but print 'dummy timeout
called'.
2/ call fTest.apply so that fTest prints 'direct'.
3/ Fire a timeout that calls indirect(). Indirect prints 'indirect called', and
calls fTest.apply so that fTest prints 'timeout indirect'.
4/ Fire a timeout to fTest.apply so that fTest prints 'timeout'.

When executing Test->Test, the following output is printed:
direct
dummy timeout called
indirect called
timeout indirect
Moz then crashes.

With jst's patch no crash occurs, any subsequent tries print only 'direct'.

So, steps 1, 2, and 3 work, 4 crashes. With the patch, _no_ timeouts work
anymore after step 4. Something is fishy about the setTimeout and Function.apply
combo.

Comment 11

[Brendan Eich [:brendan]]

JS does not make a "bound method reference" when you pass a function-valued 
property m of an object o to another function for later invocation -- that is, 
the later invocation will not bind 'this' to o unless you pass o as well, and 
the invoker supplies it via Function.prototype.call or Function.prototype.apply:

var o = {m: function() {print("In m, this is " + this)};
function later(f) {f();}
later(o.m);

In the JS shell, you'll see "In m, this is [object global]" (XUL or HTML with 
dump or alert for print, you'd see [object Window]", IIRC).  JS *always* binds 
'this' dynamically, based on the caller's function-valued expression.  So 'this' 
binds to the global object when later() calls f() -- 'this' does not bind to o.

(JS also doesn't bind 'this' to the activation object that ECMA says is at the 
front of the scope chain when later() is active -- ECMA requires that object to 
be hidden, so the global object is used instead.)

If you want this to bind to o, pass it and use Function.prototype.call or 
Function.prototype.apply:

var o = {m: function() {print("In m, this is " + this)};
function later(f,o,a) {f.apply(o, a);}
later(o.m);

This prints "In m, this is [object Object]" -- setting o.toString = function 
(){return "[o]"} makes things even clearer.

Now, things are trickier when substituting setTimeout() for later(), because 
setTimeout always binds 'this' to the window object in which 'setTimeout' was 
found by scope chain search (the current window, typically).  You might rather 
say w.setTimeout(f, t, args...) where w is some other window or frame, and that 
would bind 'this' to w, not the current window.  But w is not f, the function 
to be applied.  Although you can call another window's setTimeout, you can't 
"borrow" the setTimeout function from Window.prototype by making your own 
object, o = {setTimeout: window.setTimeout}, and then calling o.setTimeout(f, t, 
args...), because the DOM native code implementing setTimeout requires its this 
('obj' in the JS API convention) parameter to be of Window class.

(It's a bug that this requirement is not enforced with a JS_InstanceOf test and 
soft error; currently you can crash the system by borrowing setTimeout using an 
object of a different DOM class!  See bug 33304.)

So, to use setTimeout and apply properly, you must write a helper function:

function helper() {
  var a = [].concat(arguments);
  var f = a.shift();
  var t = a.shift();
  f.apply(t, a);
}
setTimeout(helper, 1, fTest, null, 'time', 'out');

Note that I provide for a 'this' parameter (passing null, and shifted into the 
local variable t within helper) to be provided when fTest is invoked.  Also, I 
avoid making an array ['time', 'out'], passing those elements as extra actual 
parameters to setTimeout and leaving them in the twice-shifted copy of the 
arguments array.

Let me know if this isn't satisfactory.  Sorry to run on so long, I wanted to 
give the gory details.  And remember, in JS a function is not a "method bound to 
a particular object" -- 'this' binding depends on how one *calls* the function, 
not on how one declares it or uses its value without calling it.

/be

Comment 12

[Johnny Stenback (:jst)]

After I managed to fix the crash (in nsGlobalWindow.cpp) caused by the testcases
in this bug I scratched my head for some time trying to figure out why the
testcase didn't work. First I realized that the security manager didn't allow
the JS to execute, I hacked around that and then I realized that the code in the
testcases doesn't work (see brendans explanation above).

Once I had hacked around the security manager problem I realized that once the
security manager is fixed (assuming it needs fixing) to allow for the testcase
to execute, my crash fix is no longer needed (in the testcases in this bug) but
I think it's worth checking in anyway (I'll attach it to this bug).

Norris, brendan: Here's what I did to the security manager to get around the
problem:

Index: caps/src/nsScriptSecurityManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/caps/src/nsScriptSecurityManager.cpp,v
retrieving revision 1.61
diff -u -r1.61 nsScriptSecurityManager.cpp
--- nsScriptSecurityManager.cpp 2000/04/01 00:37:30     1.61
+++ nsScriptSecurityManager.cpp 2000/04/05 23:34:49
@@ -662,7 +662,7 @@
     // JavaScript is disabled, but we must still execute system JavaScript
     JSScript *script = JS_GetFunctionScript(nsnull, (JSFunction *) jsFunc);
     if (!script)
-        return NS_ERROR_FAILURE;
+        return NS_OK;
     JSPrincipals *jsprin = JS_GetScriptPrincipals(nsnull, script);
     if (!jsprin)
         return NS_ERROR_FAILURE;

(this is in nsScriptSecurityManager::CanExecuteFunction())

Now, the problem seems to be that since the testcases do

  var fText = new Function(...);
  setTimeout(fTest.apply, 1, ...)

there's no script for the function passed to CanExecuteFunction() (since
fTest.apply is a native method) the security manager returns an error and that's
when bad things start happening...

Norris (and/or brendan), could you look into the security manager problem?

Comment 13

[Johnny Stenback (:jst)]

Duh, forgot to Cc: norris and brendan, please have a look at my above comment.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Initial crash fix in nsGlobalWindow.cpp

Comment 15

[Johnny Stenback (:jst)]

This is not a crasher any more, but the patch (or some version of it) should be
checked in sometime, moving to M19.

Comment 16

[Johnny Stenback (:jst)]

Nominating for beta3, we should at least check in some version of the attached
patch for beta3.

Comment 17

[Dave]

On http://www.honda.com, an attempt to "customize" a car will cause mozilla to
exponentially increase its memory useage (goes from ~14 to above 30 according to
top). The only thing out of the ordinary that I can see on the Jscript on that
page is a call to setTimeout which is present in the offending JS functions:

E.g.

var timer = null;
...
...
timer = setTimeout("selectTrim('"+trimID+"')", 100);

To reproduce, go to http://www.honda2000.com/models/accord_coupe/customize.html.
Just loading the page will probably substantially increase mozilla's memory
useage. Clicking on the links that call selectTrim (the LX, EX, etc links) or
clicking on the "color" links, which calls the nextPage function will fail (you
will only see a message like "Document: Done (0.043 secs)" but won't get the
intended page) and will also substantially increase mem useage if clicked enough
times.

I'm assuming that this is directly related to this bug. If not, let me know and
I'll open a new one.

I'm using 2000080404/Linux

Comment 18

[Brendan Eich [:brendan]]

Setting URL, but we really need a reduced testcase.  I'm not convinced this has 
anything to do with setTimeout itself.  Ideally, there should be a new bug filed 
with a reduced testcase.

/be

Comment 19

[Johnny Stenback (:jst)]

Please split out the honda problem into a separate bug, thanks!

Comment 20

[Dave]

Opened bug #47872 with bug on honda.com

Comment 21

[Martijn Pieters]

I removed the (for this bug irrelevant) Honda URL.

Comment 22

[Nisheeth Ranjan]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration, but do not clear 
the nsbeta3- nomination.

Comment 23

[Brendan Eich [:brendan]]

jst, did you check your patch in?  If not, can you do it?  I'll give it r= love.

/be

Comment 24

[Rick Niles]

Attachment: Really short testcase.

Comment 25

[Rick Niles]

IMHO, if it's a crasher it should get highest priority.  This bug
has bit me on quite a few pages.  Assign it to someone else, but
this needs to be fixed.

Comment 26

[ekrock's old account (dead)]

Crasher. Nom. nsbeta1.

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Moving to 0.9.1 and adding nsbeta1+ for analysis: does it still crash? Is jst's
fix checked in?

Rob, could you see if we still crash?

nsbeta1+ also for fixing the crash if it still exists, otherwise nsbeta1-.

Comment 28

[Prashant Desale]

Already nominated for nsbeta1. Milestone is 0.9.1.
adding keyword mozilla0.9.1

Comment 29

[Johnny Stenback (:jst)]

Moving to mozilla0.9.2

Comment 30

[basic]

The xul based testcases no longer work in the current builds due to xul changes.
The html testcase (attachment 15953 [details]) doesn't crash mozilla.

Comment 31

[basic]

Attachment: modified version of 3rd version of testcase to use new xul syntax

Comment 32

[basic]

build 2001061304 win32 installer sea talkback trunk
when testing attachment 38586 [details] (locally)

I don't see a crash, but I do see this in the console (dos):
direct
dummy timeout called
indirect called
timeout indirect

and this in the javascript console:
Error: uncaught exception: [Exception... "Cannot convert WrappedNative to 
function"  nsresult: "0x8057000d (NS_ERROR_XPC_CANT_CONVERT_WN_TO_FUN)"  
location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: 
no]

Comment 33

[Johnny Stenback (:jst)]

Moving to mozilla0.9.3

Comment 34

[Johnny Stenback (:jst)]

Pushing to mozilla0.9.5

Comment 35

[Johnny Stenback (:jst)]

WORKSFPRME now, probably fixed by the other setTimeout() crash fixes that went
in not too long ago. Please re-open if this still crashes mozilla for someone.