Bug 40598 - [Qt] QtWebkit Crashes on loading CelticKane Standard tests
Summary: [Qt] QtWebkit Crashes on loading CelticKane Standard tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-06-14 16:16 PDT by Viatcheslav Ostapenko
Modified: 2010-10-05 03:02 PDT (History)
9 users (show)

See Also:


Attachments
Move big allocation from stack to heap. (1.32 KB, patch)
2010-06-14 16:55 PDT, Viatcheslav Ostapenko
ggaren: review-
Details | Formatted Diff | Diff
Benchmark (test_page.html) (707 bytes, text/html)
2010-06-15 11:10 PDT, Viatcheslav Ostapenko
no flags Details
Symbian only patch (1.35 KB, patch)
2010-07-22 07:45 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Added comment about symbian stack (1.72 KB, patch)
2010-09-16 12:47 PDT, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2010-06-14 16:16:31 PDT
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.
Comment 1 Viatcheslav Ostapenko 2010-06-14 16:55:32 PDT
Created attachment 58728 [details]
Move big allocation from stack to heap.
Comment 2 Alexey Proskuryakov 2010-06-15 10:53:23 PDT
I don't know if the inline capacity was added here based on actual measurements, but this change may negatively affect performance.
Comment 3 Viatcheslav Ostapenko 2010-06-15 11:09:34 PDT
(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.
Comment 4 Viatcheslav Ostapenko 2010-06-15 11:10:45 PDT
Created attachment 58798 [details]
Benchmark (test_page.html)
Comment 5 Oliver Hunt 2010-07-04 20:21:40 PDT
(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?
Comment 6 Geoffrey Garen 2010-07-06 10:08:38 PDT
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.
Comment 7 Viatcheslav Ostapenko 2010-07-22 07:45:40 PDT
Created attachment 62296 [details]
Symbian only patch
Comment 8 Viatcheslav Ostapenko 2010-07-22 07:48:00 PDT
> 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.
Comment 9 Geoffrey Garen 2010-07-22 14:25:00 PDT
> 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?
Comment 10 Viatcheslav Ostapenko 2010-07-22 14:53:33 PDT
(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.
Comment 11 Simon Hausmann 2010-08-04 12:54:49 PDT
What about increasing the stack size of the application, using EPOCSTACKSIZE?
Comment 12 Viatcheslav Ostapenko 2010-08-04 13:08:32 PDT
(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?
Comment 13 Janne Koskinen 2010-09-03 01:04:21 PDT
(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.
Comment 14 Andreas Kling 2010-09-16 04:11:42 PDT
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.
Comment 15 Alexey Proskuryakov 2010-09-16 08:17:40 PDT
Comment on attachment 62296 [details]
Symbian only patch

Please at least add a comment explaining why SYMBIAN is different here.
Comment 16 Viatcheslav Ostapenko 2010-09-16 12:47:59 PDT
Created attachment 67831 [details]
Added comment about symbian stack
Comment 17 Andreas Kling 2010-09-16 13:08:54 PDT
Comment on attachment 67831 [details]
Added comment about symbian stack

Good call, AP.
Comment 18 Eric Seidel 2010-09-18 03:19:48 PDT
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.
Comment 19 WebKit Commit Bot 2010-10-01 07:56:30 PDT
Comment on attachment 67831 [details]
Added comment about symbian stack

Clearing flags on attachment: 67831

Committed r68890: <http://trac.webkit.org/changeset/68890>
Comment 20 WebKit Commit Bot 2010-10-01 07:56:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Ademar Reis 2010-10-05 03:02:41 PDT
Revision r68890 cherry-picked into qtwebkit-2.1 with commit 1254672 <http://gitorious.org/webkit/qtwebkit/commit/1254672>