Bug 124634

*** Bug 56291 has been marked as a duplicate of this bug. ***

Created attachment 217386 [details] the patch.

Comment on attachment 217386 [details] the patch. Attachment 217386 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/29828069

Comment on attachment 217386 [details] the patch. Attachment 217386 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/29848105

Comment on attachment 217386 [details] the patch. Attachment 217386 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/29768081

Comment on attachment 217386 [details] the patch. Attachment 217386 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/29768076

Comment on attachment 217386 [details] the patch. Should have tested with a release build. Will fix.

Created attachment 217399 [details] patch 2: fixed release build.

Comment on attachment 217399 [details] patch 2: fixed release build. Attachment 217399 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/30058020

Created attachment 217455 [details] patch 3: another try at fixing EWS issues.

Comment on attachment 217455 [details] patch 3: another try at fixing EWS issues. View in context: https://bugs.webkit.org/attachment.cgi?id=217455&action=review > Source/JavaScriptCore/interpreter/JSStackInlines.h:158 > + m_vm.setStackLimit(reinterpret_cast<void*>(newEnd)); No need to reinterpret_cast to void*. > Source/JavaScriptCore/runtime/VMEntryScope.cpp:42 > +#if ENABLE(ASSEMBLE) Should be ASSEMBLER. I like to use copy-and-paste for tasks like this, to avoid typos like this. > Source/JavaScriptCore/runtime/VMEntryScope.h:43 > + bool hasEnoughStackToEnterVM() const { return m_stackBounds.isSafeToRecurse(); } You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code. > Source/JavaScriptCore/runtime/VMEntryScope.h:52 > + void* m_prevStackLimit; This needs a comment explaining that the previous limit might belong to another thread's stack. > Source/WebCore/bindings/js/JSCryptoAlgorithmBuilder.cpp:74 > + ASSERT(vm.entryScope); I don't think this ASSERT adds much. If it's false, we'll crash on the next line anyway.

(In reply to comment #11) > > Source/JavaScriptCore/runtime/VMEntryScope.h:43 > > + bool hasEnoughStackToEnterVM() const { return m_stackBounds.isSafeToRecurse(); } > > You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code. I can't do this. The BytecodeGenerator, Parser, and StringRecursionChecker still relies on VMStackBounds::isSafeToRecurse(). So, instead, I opted to keep a unified implementation of how we do stack checks in VMStackBounds::isSafeToRecurse() instead.

(In reply to comment #12) > (In reply to comment #11) > > You should remove VMStackBounds::isSafeToRecurse() and its uses, including this use. Plain comparison against m_stackBounds.stackLimit() needs to become the one true way to measure the extent of the stack. Otherwise, we allow subtle differences between the stack limit check in C++ and the same check in JIT code. > > I can't do this. The BytecodeGenerator, Parser, and StringRecursionChecker still relies on VMStackBounds::isSafeToRecurse(). So, instead, I opted to keep a unified implementation of how we do stack checks in VMStackBounds::isSafeToRecurse() instead. Per our offline conversation, I'll re-implement the C++ code native stack check to be a comparison against the VM's stackLimit pointer. The only added functionality that VMStackBounds add over the WTF::StackBounds is that it takes into consideration whether we're in the midst of handling a JS exception or not when determining how much stack capacity is required in the stack checks. Revisiting the current users of VMStackBounds, I see no reason why this required stack capacity value should change since the last time the stackLimit was set in the VM. Hence, these users should be able to do their stack checks against the VM's stackLimit pointer without any adjustments since the required stack capacity won't change in the period of their use. With that, I ended up adding a VM::isSafeToRecurse() function, and removed VMStackBounds altogether. VMEntryScope now only updates the VM stackLimit, but does not offer any mechanism to check it. The only stack check mechanism for the native stack is now VM::isSafeToRecurse() which implements the pointer comparison against VM::m_stackLimit. Updated patch coming soon.

Created attachment 217503 [details] patch 4: addresses Geoff's feedback.

Comment on attachment 217503 [details] patch 4: addresses Geoff's feedback. r=me

Thanks for the review. Landed in r159605: <http://trac.webkit.org/r159605>.