Bug 4045 – JavaScript call stack limit of 99 is too small for some applications; needs to be closer to 500

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Vicki Murley]

<rdar://problem/3590522>

* SUMMARY: 
Complicated web application can push a lot of calls on the stack.  We differ greatly from other browsers 
on the same platform and windows.

* STEPS TO REPRODUCE: 
Open the attached in Safari. 

Onload handler calls a function which recursively calls itself a writes the loop count to a div on the 
page.

* RESULTS: 
My results for the config attached....
Safari - 99

Others
Mac IE 5.2.2 - 690
Mac Camino .7 - 1000
Mac Firefox .8 - 1000

Win - Config 256 MB RAM, Windows 2000 (5.00.2195)
Win IE 6.0.x  - 466
Win Mozilla 1.5 - 1000
Win Netscape 7.1 - 1000

[Ian 'Hixie' Hickson]

*** Bug 4044 has been marked as a duplicate of this bug. ***

[Ian 'Hixie' Hickson]

vicky: could you attach the testcase that was attached to the original report?

[Ian 'Hixie' Hickson]

Apparently there is no original testcase.

TESTCASE:
   http://www.hixie.ch/tests/adhoc/js/core/001.xml
   http://www.hixie.ch/tests/adhoc/js/core/001.html

Gecko does indeed have a maximum stack depth of 1000. Opera gives me a depth of
3341 but I get the feeling it depends on the available memory. WinIE6 gives me
462 but similarly, I'm guessing that's dependent on memory in some way.

[Sjoerd Mulder]

We at Backbase would really like to write support for the Safari Browser, 
because a lot of customers are asking for it. This bug is blocking our engine 
to startup on the Safari browser because of the tiny stack of 99. When we 
change the stack to 300 manually in source and compile it works. Is it 
possible to put more priority on this bug?

[Maciej Stachowiak]

We will be redoing the engine soon so the JS call stack does not depend on the machine stack. That should 
make it much easier to increase the limit.

[Sjoerd Mulder]

(In reply to comment #5)
> We will be redoing the engine soon so the JS call stack does not depend on the
> machine stack. That should 
> make it much easier to increase the limit.
> 

How soon is soon?

[Fabian Jakobs]

We have a similar problem as Sjoerd with qooxdoo (http://qooxdoo.org). The applications start up and are working mostly as expected, but at some points we run into this issue as well. This is an issue that blocks full Safari support for our Framework.

[Alexey Proskuryakov]

Created attachment 13359 [details]
naive fix

I guess this is more like a request for comments than a real proposed patch, but this bug blocks a HitList one, so I'll shoot anyway...

The stack size was decreased to 100 in r2184:

-------------------------------------------------------------
r2184 | darin | 2002-09-27 21:27:43 +0400 (Fri, 27 Sep 2002) | 5 lines

        - fixed 3033969 -- repro crash (infinite recursion in JavaScript)
        clicking on "screens" option at fsv.sf.net

        * kjs/object.h: Change recursion limit to 100 levels rather than 1000.
-------------------------------------------------------------

I have tried clicking on "screens" option at fsv.sf.net after raising the limit, and didn't get a crash on a PowerPC Mac. The included test doesn't crash either, of course (I ran it under GuardMalloc).

I should also mention that the recursion counter in a static variable doesn't look thread-safe to me.

[Geoffrey Garen]

> I should also mention that the recursion counter in a static variable doesn't
> look thread-safe to me.

A lot of JavaScriptCore is not thread-safe. The current solution is a global lock around the framework.

[Geoffrey Garen]

Darin's comments in Radar indicate that he saw JavaScriptCore crashing at "around 350" recursions. I checked the callstacks, and it looks like we've removed 1/7 calls per recursion in TOT. So that would enable us to get up around 400 recursions.

I tested with a debug build of TOT on my MacBook Pro, and didn't crash until > 11,000 recursions. 

I suspect that the real limit depends on how much RAM you have, if heap and stack grow toward each other. I have 2 GB. Unfortunately, there's no hardware info in the original bug report. The original bug report was also on Jaguar, and the OS may have improved since then.

I would recommend being conservative, and setting the limit to 500, since that's what WinIE does (at least on a system with 256 MB of RAM). I'd like Maciej or Darin to make the call on this, though.

[Geoffrey Garen]

...because enabling a potential stack overflow seems risky.

[Darin Adler]

Comment on attachment 13359 [details]
naive fix

The problem with the test cases here is that stack usage can be much-much greater per level that the simple function call case, due to the recursive descent structure of the interpreter and I beleve in some cases even worse due to calls in through the DOM and then back into the JavaScript interpreter. I think that even 100 may be too generous for some of those bad cases.

The original bug that motivated changing our recursion depth limit was this:

----------------

<rdar://problem/3033969> repro crash (infinite recursion in JavaScript) clicking on "screens" option at fsv.sf.net

Thread 0 Crashed:
#0  0x02b34404 in DOM::AttributeImpl::id() const ()
#1  0x02abf180 in DOM::NamedAttrMapImpl::getAttributeItem(unsigned) const (this=0xbff80130, id=26516752) at khtml/xml/dom_elementimpl.cpp:656
#2  0x02a4d324 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x1949d10, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:350
#3  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x19395b0, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#4  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x192abc0, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#5  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x192ab10, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#6  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x1a1d490, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#7  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x1a1d430, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#8  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x1a295a0, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#9  0x02a4d410 in DOM::HTMLCollectionImpl::getNamedItem(DOM::NodeImpl*, int, DOM::DOMString const&) const (this=0x1c07510, current=0x1a35130, attr_id=56, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:357
#10 0x02a4d4e8 in DOM::HTMLCollectionImpl::namedItem(DOM::DOMString const&) const (this=0x1c07510, name=@0xbff806b0) at khtml/html/html_miscimpl.cpp:377
#11 0x029d292c in DOM::HTMLCollection::namedItem(DOM::DOMString const&) const (this=0xbff80670, name=@0xbff806b0) at khtml/dom/html_misc.cpp:140
#12 0x029f9b14 in KJS::HTMLDocument::tryGet(KJS::ExecState*, KJS::UString const&) const (this=0x1a3bdf0, exec=0xbff80f80, propertyName=@0xbff80a54) at khtml/ecma/kjs_html.cpp:159
#13 0x029d95a4 in KJS::DOMObject::get(KJS::ExecState*, KJS::UString const&) const (this=0x1a3bdf0, exec=0xbff80f80, p=@0xbff80a54) at khtml/ecma/kjs_binding.cpp:45
#14 0x080586f8 in KJS::Reference::getValue(KJS::ExecState*) const (this=0xbff80a40, exec=0xbff80f80) at kjs/reference.cpp:123
#15 0x080228b8 in KJS::ResolveNode::evaluate(KJS::ExecState*) (this=0x1970db0, exec=0xbff80f80) at kjs/nodes.cpp:215
#16 0x08025b44 in KJS::ArgumentListNode::evaluateList(KJS::ExecState*) (this=0x1970dd0, exec=0xbff80f80) at kjs/nodes.cpp:584
#17 0x08025ebc in KJS::ArgumentsNode::evaluateList(KJS::ExecState*) (this=0x1970df0, exec=0xbff80f80) at kjs/nodes.cpp:624
#18 0x08026760 in KJS::FunctionCallNode::evaluate(KJS::ExecState*) (this=0x1970e10, exec=0xbff80f80) at kjs/nodes.cpp:700
#19 0x0802e150 in KJS::ExprStatementNode::execute(KJS::ExecState*) (this=0x1970e30, exec=0xbff80f80) at kjs/nodes.cpp:1764
#20 0x08037418 in KJS::SourceElementNode::execute(KJS::ExecState*) (this=0x1970e60, exec=0xbff80f80) at kjs/nodes.cpp:2841
#21 0x08037840 in KJS::SourceElementsNode::execute(KJS::ExecState*) (this=0x1970e90, exec=0xbff80f80) at kjs/nodes.cpp:2884
#22 0x08036324 in KJS::FunctionBodyNode::execute(KJS::ExecState*) (this=0x1970ec0, exec=0xbff80f80) at kjs/nodes.cpp:2709
#23 0x08010e24 in KJS::DeclaredFunctionImp::execute(KJS::ExecState*) (this=0x1970f10, exec=0xbff80f80) at kjs/function.cpp:270
#24 0x0800fe7c in KJS::FunctionImp::call(KJS::ExecState*, KJS::Object&, KJS::List const&) (this=0x1970f10, exec=0xbff81490, thisObj=@0xbff81100, args=@0xbff810c0) at kjs/function.cpp:124
#25 0x0806d3d8 in KJS::Object::call(KJS::ExecState*, KJS::Object&, KJS::List const&) ()
#26 0x08026bfc in KJS::FunctionCallNode::evaluate(KJS::ExecState*) (this=0x1970e10, exec=0xbff81490) at kjs/nodes.cpp:755
#27 0x0802e150 in KJS::ExprStatementNode::execute(KJS::ExecState*) (this=0x1970e30, exec=0xbff81490) at kjs/nodes.cpp:1764
#28 0x08037418 in KJS::SourceElementNode::execute(KJS::ExecState*) (this=0x1970e60, exec=0xbff81490) at kjs/nodes.cpp:2841
#29 0x08037840 in KJS::SourceElementsNode::execute(KJS::ExecState*) (this=0x1970e90, exec=0xbff81490) at kjs/nodes.cpp:2884
#30 0x08036324 in KJS::FunctionBodyNode::execute(KJS::ExecState*) (this=0x1970ec0, exec=0xbff81490) at kjs/nodes.cpp:2709
#31 0x08010e24 in KJS::DeclaredFunctionImp::execute(KJS::ExecState*) (this=0x1970f10, exec=0xbff81490) at kjs/function.cpp:270

... and so on

----------------

Here is a bit of the discussion from 2002:

8/26/02 5:15 PM:
hit http://fsv.sf.net and click on the "Screens" button on the left
crash

8/27/02 8:24 PM Darin Adler:
The problem seems to be caused by an onClick handler that calls a function named onclick:

<a href="screenshots/" onMouseOver="mouseover(screenshots_node)" onClick="onclick(screenshots_node)" onMouseOut="mouseout(screenshots_node)">
<img name="screenshots_node" src="img/screenshots-0.png" alt="[Screen shots]" border="0" width="140" height="92" /></a>

The onclick function is innocuous enough. So is this supposed to be case-sensitive? What makes it work on other web browsers?

8/27/02 10:18 PM Darin Adler:
Maciej, I'd like to fix this, but I'd like your input about where you think the problem lies.

9/6/02 8:39 PM Maciej Stachowiak:
A JavaScript infinite recursion isn't supposed to crash the browser, so it seems we have at least two bugs. I am pretty sure that it is supposed to be case sensitive, but the actual name of the window attribute is "onclick" in JavaScript, in all-lowercase. Perhaps it works in other browsers because the function declaration shadows the event handler property, but in kjs it assigns to it.

9/27/02 7:58 AM Darin Adler:
I tried this simplified test that doesn't even define a function:

    <a href="buttondiv.html" onclick="onclick()">test</a>

This test crashes with infinite recursion on both Safari and Firefox, but not on MacIE.

If I add a parameter to the handler call:

    <a href="buttondiv.html" onclick="onclick(1)">test</a>

Then the infinite recursion doesn't happen in Firefox. That's why the original page works fine in Firefox.

So the case sensitivity is a red herring. That's working fine. The issue here seems something to do with parameters.

9/27/02 9:09 AM Darin Adler:
The fact that the infinite recursion doesn't happen when there's a parameter seems to be a quirk in Firefox.

But this seems to be a legitimate case of infinite recursion. Testing in all three of the others browsers previously mentioned, I find that this:

    <script language=javascript>
    function onclick() { alert("called onclick function"); }
    </script>
    <a href="buttondiv.html" onclick="onclick(1)">test</a>

Does *not* put up an alert when you click. So it's clear that the function is not being called. But this does:

    <script language=javascript>
    function f() { alert("called f function"); }
    </script>
    <a href="buttondiv.html" onclick="f(1)">test</a>

So I think that our case sensitivity and scoping is working correctly here, and the fact that Firefox crashes when there is no parameter, and do when there is a parameter, remains a deep mystery.

9/27/02 9:10 AM Darin Adler:
Should have been "and don't when there is a parameter".

9/27/02 9:27 AM Darin Adler:
Turns out we have infinite recursion protection in KJS already. But the level is set to 1000, and the kjs stack use is great enough that we crash due to lack of stack long before we hit 1000 levels, around 350 levels. I set our limit to 100, which fixes this bug.

----------------

review- for now, at least until we make a more-challenging test for LayoutTests

[Mark Rowe (bdash)]

*** Bug 13284 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

*** Bug 13289 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

I think we could fix this by taking an actual measurement of the stack instead of the number of JS function call recursions. In most cases, that would allow us to go far past 99.

[Mark Malone]

Bring it!  Last time I talked to them, this was Cognos' only blocker to Safari certification.

[David Kilzer (:ddkilzer)]

Is there any way to revisit this for Leopard?  Interesting results from using the naive fix in Bug 13815 Comment #2.

[Maciej Stachowiak]

While the naiive fix works sometimes, in other cases it can lead to an overflow of the machine stack when executing complex JS, which crashes the whole browser. This is why we are hesitant to just change the constant.

[Luigi Giannini]

For example?
Is it possible to have some URL for test ?

Thank
Luigi GIannini

(In reply to comment #18)
> While the naiive fix works sometimes, in other cases it can lead to an overflow
> of the machine stack when executing complex JS, which crashes the whole
> browser. This is why we are hesitant to just change the constant.
> 

[Luigi Giannini]

Hello, I'm the sender of Bug 13815 Comment #2

With the only purpose to evaluate how much and  in witch way change KJS_MAX_STACK to a constant 1000 value can impact the daily web browsing, I have spent short time to test the latest WebKit source tree without and with the ultra easy fix mentioned above:

WITHOUT fix
run-webkit-tests: run 7822 test ; 7815 pass ; 7 not
run-javascriptcore-tests: find 2 regression (relatetive to date management)
run-iexploder-tests: fail 22 test on a 55543 total test

WITH fix
run-webkit-tests: run 7822 test ; 7816 pass ; 6 not
run-javascriptcore-tests: find 2 regression (relatetive to date management)
run-iexploder-tests: fail 22 test on a 55543 total test
Moreover I try to load the http://fsv.sf.net and click on the "Screens" button on the left and everything run without crash! This was the original cause that introduce the call stack limitation of 100.
And nothing bad in daily browsing fot 2 working days.

Why not fix?

[Luigi Giannini]

If WebKit crash with heavy and complex JS on mac and not on other platform is a good thing?
And is really a bug related to call stack?
In my previous comment I reported that the crash at rdar://problem/3033969 on 2002 (from which orginated the 100 limit call stack recursion) is not more occurring also with a limit equal to 1000: why?

[Luigi Giannini]

This occurs on PPC and Intel?

(In reply to comment #18)
> While the naiive fix works sometimes, in other cases it can lead to an overflow
> of the machine stack when executing complex JS, which crashes the whole
> browser. This is why we are hesitant to just change the constant.
> 

[David Kilzer (:ddkilzer)]

Fixed by kmccollo in r25161.

http://trac.webkit.org/projects/webkit/changeset/25161