Bug 100109 - Regression(r132143): Assertion hit in JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&)
Summary: Regression(r132143): Assertion hit in JSC::Interpreter::StackPolicy::StackPol...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
Depends on:
Blocks: 99872
  Show dependency treegraph
Reported: 2012-10-23 04:28 PDT by Chris Dumez
Modified: 2012-10-23 22:03 PDT (History)
4 users (show)

See Also:

Patch (6.12 KB, patch)
2012-10-23 04:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.58 KB, patch)
2012-10-23 10:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-10-23 04:28:51 PDT
On EFL 64bit debug build bot, we hit the following assertion after r132143:
ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size)
/home/chris/Devel/WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp(194) : JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, const WTF::StackBounds&)
1   0x2b6781fbd5a6 JSC::Interpreter::StackPolicy::StackPolicy(JSC::Interpreter&, WTF::StackBounds const&)
2   0x2b6781fc095c JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
3   0x2b6782080af4 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
4   0x2b677e445e65 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
5   0x2b677e462bbc WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*)
6   0x2b677e462cbe WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&)
7   0x2b677d70490c WebCore::ScriptController::executeScript(WebCore::ScriptSourceCode const&)
8   0x2b677d704862 WebCore::ScriptController::executeScript(WTF::String const&, bool)
9   0x2b677aa55aa9 WebKit::WebPage::runJavaScriptInMainFrame(WTF::String const&, unsigned long)
10  0x2b677aae383e void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, unsigned long), WTF::String, unsigned long>(CoreIPC::Arguments2<WTF::String, unsigned long> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, unsigned long))
11  0x2b677aae159a void CoreIPC::handleMessage<Messages::WebPage::RunJavaScriptInMainFrame, WebKit::WebPage, void (WebKit::WebPage::*)(WTF::String const&, unsigned long)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WTF::String const&, unsigned long))
12  0x2b677aadf44e WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
13  0x2b677aa58d2e WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
14  0x2b677a988b88 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
15  0x2b677a98459f WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
16  0x2b677a84bb4c CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
17  0x2b677a84bc85 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
18  0x2b677a84be21 CoreIPC::Connection::dispatchOneMessage()
19  0x2b677a855f76 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
20  0x2b677a855d7c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
21  0x2b677aa3775c WTF::Function<void ()>::operator()() const
22  0x2b677deee132 WebCore::RunLoop::performWork()
23  0x2b677e8a88a7 WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
24  0x2b6780d28901
25  0x2b6780d27851
26  0x2b6780d27d97 ecore_main_loop_begin
27  0x2b677e8a8871 WebCore::RunLoop::run()
28  0x2b677aab4d78 WebProcessMainEfl
29  0x4007c4 main
30  0x2b677ae7e76d __libc_start_main
31  0x4006e9

When we hit this assertion, the size is negative which seems to indicate a possible integer overflow.
Comment 1 Chris Dumez 2012-10-23 04:40:57 PDT
Created attachment 170120 [details]

This patch fixes crashing on EFL port.
Comment 2 Chris Dumez 2012-10-23 06:49:10 PDT
On my machine I got before the crash:
stack.size(): 92967688560640
Max INT:          2147483647
Comment 3 Mark Lam 2012-10-23 08:20:32 PDT
(In reply to comment #2)
> On my machine I got before the crash:
> stack.size(): 92967688560640
> Max INT:          2147483647

Thanks for catching this issue.  I didn't think it was possible (because it seemed highly impractical) that someone would want to have a stack that is greater than 2G.  Your case proves otherwise, although I would be suspicious of that size value.  92967688560640 makes for 84 terabytes of memory.  That's more memory than most machines have just to be used for a single stack.

While it is programmatically legal for you to set the stack limit/size this high, I suspect that in practice, what this actually means is that on your platform, the stack check will be defeated, and you are effectively not doing any checks at all.  One side effect of this is that some of the layout tests will start to fail in a flaky manner.  The reason is because, by defeating the stack check with an artificially high stack size, you allow the code to run with a stack check.  As a result, the test will fail and crash wherever you physically run out of stack space, which is long before you reached the specified limit, and which may vary depending on system load and available memory at the time of the run.

And again, I point you to StackBounds.  StackBounds::initialize() is platform dependent.  I suggest that you add something for PLATFORM(EFL) there that bounds the stack to a more reasonable limit so that the stack recursion checks can be effective.

Regardless, you are right that I should have used size_t instead of int for the StackPolicy calculations.  I will comment on your patch shortly.  Thanks again for catching this issue.
Comment 4 Mark Lam 2012-10-23 08:55:29 PDT
Comment on attachment 170120 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=170120&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        happy.

See comment below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:123
> +    const size_t DefaultErrorModeRequiredStack = 32 * 1024;

I prefer that you leave these capitalized as in the original.  I understand that the style checker is not happy about it, but I consider this a bug in the style checker rather than in the code, and I made a comment about it in my original patch here: https://bugs.webkit.org/show_bug.cgi?id=99872#c3

> Source/JavaScriptCore/interpreter/Interpreter.cpp:128
> +    //    but require that we have at least DefaultRequiredStack capacity


> Source/JavaScriptCore/interpreter/Interpreter.cpp:133
> +    //    |         ... | <-- DefaultRequiredStack --> | ...


> Source/JavaScriptCore/interpreter/Interpreter.cpp:146
> +    //    DefaultErrorModeRequiredStack capacity.

Ditto, and similarly below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:180
> +    size_t useableStack = (requiredCapacity <= size) ? size - requiredCapacity : DefaultMinimumUseableStack;

Nit: (not in the coding style, just my personal pref but ...) can you break this line into 2 lines (as in example above) so as to keep the lengths < ~80 per line.  That was one of my goals when I wrote this function.  Would like to keep it consistent if that's ok with you.  Thanks.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:191
> +    ASSERT(useableStack >= DefaultMinimumUseableStack && useableStack <= size);

This assertion is not true.  If the stack size is less than DEFAULT_MINIMUM_USEABLE_STACK, then (useableStack >= DEFAULT_MINIMUM_USEABLE_STACK) will never be true.  Please fix the assertion or delete it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:195
>      // interpreter stack checks:

Please delete this line.  It was a half edited comment from my patch.  Thanks.

LGTM after you address the above issues.  Thanks.
Comment 5 Chris Dumez 2012-10-23 09:44:58 PDT
Ok, thanks. I'll fix and reupload now.
Comment 6 Chris Dumez 2012-10-23 10:00:18 PDT
Created attachment 170184 [details]

Take Mark Lam's feedback into consideration.

BTW, the extremely high stack size value is returned by pthread_attr_getstack(&sattr, &stackBase, &stackSize) (in #elif OS(UNIX) implementation of StackBounds::initialize()). I'll look further into it but this is anyway a separate issue.
Comment 7 WebKit Review Bot 2012-10-23 10:03:21 PDT
Attachment 170184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:121:  DEFAULT_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:122:  DEFAULT_MINIMUM_USEABLE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:123:  DEFAULT_ERROR_MODE_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 2 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 WebKit Review Bot 2012-10-23 10:58:36 PDT
Comment on attachment 170184 [details]

Clearing flags on attachment: 170184

Committed r132244: <http://trac.webkit.org/changeset/132244>
Comment 9 WebKit Review Bot 2012-10-23 10:58:40 PDT
All reviewed patches have been landed.  Closing bug.