[webkit-reviews] review denied: [Bug 4045] JavaScript call stack limit of 99 is too small for some applications; needs to be closer to 500 : [Attachment 13359] naive fix

Sat Feb 24 14:52:39 PST 2007

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 4045: JavaScript call stack limit of 99 is too small for some applications;
needs to be closer to 500
http://bugs.webkit.org/show_bug.cgi?id=4045

Attachment 13359: naive fix
http://bugs.webkit.org/attachment.cgi?id=13359&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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