Bug 99872

Created attachment 169764 [details] Fix.

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.

(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>.

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

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

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.

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.

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

(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.

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.

Issues have been addressed. Landed in r132143: <http://trac.webkit.org/changeset/132143>.

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

(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.

(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.

Here are the results on Ubuntu 64 bit: requiredCapacity: 0, size: -944271360 ASSERTION FAILED: (requiredCapacity >= 0) && (requiredCapacity < size)

(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.

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.

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().

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

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

(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.