Bug 40598 – [Qt] QtWebkit Crashes on loading CelticKane Standard tests

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

Crash in JavaScriptCore on S60 running popular Javascript benchmark case. 
Caused by stack overflow.

The related call stack .
--------------------------------------------------
68 JSC::arrayProtoFuncToString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\ArrayPrototype.cpp:180 0x4987601a
67 JSC::call() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\CallData.cpp:36 0x4987c96d
66 JSC::callDefaultValueFunction() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:245 0x4989a110
65 JSC::JSObject::defaultValue() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:266 0x49899f30
64 JSC::JSObject::toPrimitive() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.h:590 0x497f32b7
63 JSC::JSObject::toString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:487 0x4989ad53
62 JSC::JSValue::toString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSString.h:275 0x49802c32
61 JSC::arrayProtoFuncToString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\ArrayPrototype.cpp:181 0x4987602f
60 JSC::call() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\CallData.cpp:36 0x4987c96d
59 JSC::callDefaultValueFunction() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:245 0x4989a110
58 JSC::JSObject::defaultValue() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:266 0x49899f30
57 JSC::JSObject::toPrimitive() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.h:590 0x497f32b7
56 JSC::JSObject::toString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSObject.cpp:487 0x4989ad53
55 JSC::JSValue::toString() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSString.h:275 0x49802c32
54 JSC::JSArray::sort() Y:\qt\src\3rdparty\webkit\JavaScriptCore\runtime\JSArray.cpp:701 0x498920bd
-------------------------------------------------------------

arrayProtoFuncToString() function allocates quite big object on stack.

[Viatcheslav Ostapenko]

Created attachment 58728 [details]
Move big allocation from stack to heap.

[Alexey Proskuryakov]

I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance.

[Viatcheslav Ostapenko]

(In reply to comment #2)
> I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance.

We've done some benchmarking (test_page.html):

linux
Vector on stack, buffer on stack: 43 - 44 ms
Vector on heap, buffer on heap: 44 - 45 ms
Vector on stack, buffer on heap: 43.5 - 44 ms

Windows XP
Vector on stack, buffer on stack: 83 - 84 ms
Vector on heap, buffer on heap: 94 - 95 ms
Vector on stack, buffer on heap: 88 - 89 ms

Yes, it affects performance, but it seems the difference is not big.
This code could be called recursively and 1k buffer on stack is not right.

[Viatcheslav Ostapenko]

Created attachment 58798 [details]
Benchmark (test_page.html)

[Oliver Hunt]

(In reply to comment #3)
> (In reply to comment #2)
> > I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance.
> 
> We've done some benchmarking (test_page.html):
> 
> linux
> Vector on stack, buffer on stack: 43 - 44 ms
> Vector on heap, buffer on heap: 44 - 45 ms
> Vector on stack, buffer on heap: 43.5 - 44 ms
> 
> Windows XP
> Vector on stack, buffer on stack: 83 - 84 ms
> Vector on heap, buffer on heap: 94 - 95 ms
> Vector on stack, buffer on heap: 88 - 89 ms
> 
> Yes, it affects performance, but it seems the difference is not big.
> This code could be called recursively and 1k buffer on stack is not right.

Can we get sunspider numbers?

[Geoffrey Garen]

Comment on attachment 58728 [details]
Move big allocation from stack to heap.

Even if this is the right approach (and, without any SunSpider numbers, I'm not convinced that it is), I don't think the inline capacity should go all the way to 0. How about 16?

How much stack does S60 have?

If S60's stack space is truly pathologically small, maybe the best solution is just to conditionally disable all Vector inline capacity on S60. Should be pretty easy to make that change in wtf/Vector.h.

[Viatcheslav Ostapenko]

Created attachment 62296 [details]
Symbian only patch

[Viatcheslav Ostapenko]

> If S60's stack space is truly pathologically small, maybe the best solution is just to conditionally disable all Vector inline capacity on S60. Should be pretty easy to make that change in wtf/Vector.h.

Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack.

[Geoffrey Garen]

> Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack.

Yes, there would be collateral damage from this change.

But if S60's stack is so small, what alternative do we have to ensure that the stack-allocated vectors we use don't cause stack overflow?

[Viatcheslav Ostapenko]

(In reply to comment #9)
> > Disabling inline i Vector.h is not right, because vectors could be inlined in structures that are not on stack.
> 
> Yes, there would be collateral damage from this change.
> 
> But if S60's stack is so small, what alternative do we have to ensure that the stack-allocated vectors we use don't cause stack overflow?

I've checked. There is not very much stack allocations of inlined vectors. And more important, those functions wouldn't be called recursively. The problem here in recursive allocation of 1k object on stack.

[Simon Hausmann]

What about increasing the stack size of the application, using EPOCSTACKSIZE?

[Viatcheslav Ostapenko]

(In reply to comment #11)
> What about increasing the stack size of the application, using EPOCSTACKSIZE?

How big should it be?
I've tried increasing stack size to 512kb and it still was crashing.
Should it be set bigger? Is it practical for mobile platform?

[Janne Koskinen]

(In reply to comment #12)
> (In reply to comment #11)
> > What about increasing the stack size of the application, using EPOCSTACKSIZE?
> 
> How big should it be?
> I've tried increasing stack size to 512kb and it still was crashing.
> Should it be set bigger? Is it practical for mobile platform?

Qt already sets the stack to maximum of 80kB for main thread (with EPOCSTACKSIZE). This value is hardcoded in kernel and cannot be increased any further.
If this is not run in the main thread then you can increase it up to 80kB from the default of 8kB.

BTW: I have succesfully ran Celtikane tests with QtWebkit before so this can be considered as regression if that makes any difference.

[Andreas Kling]

Comment on attachment 62296 [details]
Symbian only patch

Since we cannot increase the stack size beyond 80kB on Symbian, this looks like a reasonable solution to me.

[Alexey Proskuryakov]

Comment on attachment 62296 [details]
Symbian only patch

Please at least add a comment explaining why SYMBIAN is different here.

[Viatcheslav Ostapenko]

Created attachment 67831 [details]
Added comment about symbian stack

[Andreas Kling]

Comment on attachment 67831 [details]
Added comment about symbian stack

Good call, AP.

[Eric Seidel (no email)]

Comment on attachment 62296 [details]
Symbian only patch

Cleared Andreas Kling's review+ from obsolete attachment 62296 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 67831 [details]
Added comment about symbian stack

Clearing flags on attachment: 67831

Committed r68890: <http://trac.webkit.org/changeset/68890>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r68890 cherry-picked into qtwebkit-2.1 with commit 1254672 <http://gitorious.org/webkit/qtwebkit/commit/1254672>