Bug 99872 – JSC: Change stack recursion checks to be based on stack availability

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Some existing stack checks are based on  max reentry depth estimates.  Fix these checks to be based on actual available stack space.  Also fix the JSStack to reserve some space for error handling so that we won't end up throwing a StackOverflowError when processing (throwing and reporting) an exception.

[Mark Lam]

Created attachment 169764 [details]
Fix.

[WebKit Review Bot]

Attachment 169764 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:125:  PREFERRED_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:126:  MINIMUM_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:127:  MINIMUM_INTERPRETER_NATIVE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.h:250:  size_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.h:280:  UNINITIALIZED_NATIVE_STACK_AVAILABILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

(In reply to comment #2)
> Attachment 169764 [details] did not pass style-queue:
> 
> Source/JavaScriptCore/interpreter/Interpreter.cpp:125:  PREFERRED_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/interpreter/Interpreter.cpp:126:  MINIMUM_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/interpreter/Interpreter.cpp:127:  MINIMUM_INTERPRETER_NATIVE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> Source/JavaScriptCore/interpreter/Interpreter.h:280:  UNINITIALIZED_NATIVE_STACK_AVAILABILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

These are all constants, e.g. "const int PREFERRED_NATIVE_STACK_AVAILBILITY = 1024 * 1024;"  The style guide says that #define constants should be capitalized with underscores, but does not say anything about typed constants like these.  I think it is appropriate to use this same "capitalized with underscores" convention for typed constants as well.

> Source/JavaScriptCore/interpreter/Interpreter.h:250:  size_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

This is from "operator size_t()".  Bug against check-webkit-style filed: <https://bugs.webkit.org/show_bug.cgi?id=99911>.

[Build Bot]

Comment on attachment 169764 [details]
Fix.

Attachment 169764 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14462793

[Mark Lam]

Created attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

[WebKit Review Bot]

Attachment 169767 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:125:  PREFERRED_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:126:  MINIMUM_NATIVE_STACK_AVAILBILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:127:  MINIMUM_INTERPRETER_NATIVE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.h:250:  size_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.h:280:  UNINITIALIZED_NATIVE_STACK_AVAILABILITY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

All bots are green.  We're ready for a review now.

[Filip Pizlo]

Comment on attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

I buy his. It would be good if you got a second review for this, though; I don't have a great intuition for

[Filip Pizlo]

(In reply to comment #8)
> (From update of attachment 169767 [details])
> I buy his. It would be good if you got a second review for this, though; I don't have a great intuition for

... For the meaning of soundness in this code.

[Geoffrey Garen]

Comment on attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

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

Please make sure to address the interpreter caching issue before landing, as that is a real correctness problem.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:117
> +Interpreter::StackAvailability::StackAvailability(Interpreter& interpreter, const StackBounds& stack)

Since this class selects a stack usage policy, instead of reporting how much stack is available, maybe we should call it "StackPolicy".

> Source/JavaScriptCore/interpreter/Interpreter.cpp:123
> +    if (m_interpreter.m_nativeStackAvailability == UNINITIALIZED_NATIVE_STACK_AVAILABILITY
> +        || (!!m_interpreter.m_errorHandlingModeReentry)) {
> +        int size = stack.size();

Since VM exit and re-entry can restart on another thread, it's not valid to cache this value in the interpreter.

[Mark Lam]

Issues have been addressed.

Landed in r132143: <http://trac.webkit.org/changeset/132143>.

[Chris Dumez]

We hit an assertion in EFL API tests after this patch:

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

[Mark Lam]

(In reply to comment #12)
> We hit an assertion in EFL API tests after this patch:
> 
> 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&)

Can you please check what the value of requiredCapacity is in your case that is failing this assertion?  Thanks.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > We hit an assertion in EFL API tests after this patch:
> > 
> > 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&)
> 
> Can you please check what the value of requiredCapacity is in your case that is failing this assertion?  Thanks.

Sure, I'll report back soon.

[Chris Dumez]

Here are the results on Ubuntu 64 bit:

requiredCapacity: 0, size: -944271360
ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size)

[Mark Lam]

(In reply to comment #15)
> Here are the results on Ubuntu 64 bit:
> 
> requiredCapacity: 0, size: -944271360
> ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size)

Your size should not be negative.  It is likely that your port is not implementing WTF::StackBounds correctly.  Please look into that.

[Chris Dumez]

Comment on attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

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

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:123
>> +        int size = stack.size();
> 
> Since VM exit and re-entry can restart on another thread, it's not valid to cache this value in the interpreter.

Why is this stored in an int? stack.size() returns a size_t so it can overflow and becomes negative, right?

StackBounds does not seem to have any port-specific implementation as far as I could see.

[Chris Dumez]

Comment on attachment 169767 [details]
Fix 2: a partial edit slipped thru on the first patch.  Now fixed.

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

>>> Source/JavaScriptCore/interpreter/Interpreter.cpp:123
>>> +        int size = stack.size();
>> 
>> Since VM exit and re-entry can restart on another thread, it's not valid to cache this value in the interpreter.
> 
> Why is this stored in an int? stack.size() returns a size_t so it can overflow and becomes negative, right?
> 
> StackBounds does not seem to have any port-specific implementation as far as I could see.

By the way, I may be misunderstanding Geoffrey Garen's comment but it seems to me that the patch that landed still caches stack.size().

[Chris Dumez]

I can confirm that using size_t instead of int in the constructor solves crashing on EFL port.

Here is the patch I used for testing (for reference):
http://pastebin.com/MX0Px5S5

[Csaba Osztrogonác]

r132143 made fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html flakey on Qt - https://bugs.webkit.org/show_bug.cgi?id=100117

[Mark Lam]

(In reply to comment #18)
> By the way, I may be misunderstanding Geoffrey Garen's comment but it seems to me that the patch that landed still caches stack.size().

You misunderstood.  Geoff's comment was that the required stack capacity used for the stack checks should not be cached in the interpreter, and this patch is not in violation of that.