[webkit-reviews] review denied: [Bug 124634] Introducing VMEntryScope to update the VM stack limit : [Attachment 217455] patch 3: another try at fixing EWS issues.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied  review:
Bug 124634: Introducing VMEntryScope to update the VM stack limit
https://bugs.webkit.org/show_bug.cgi?id=124634

Attachment 217455: patch 3: another try at fixing EWS issues.
https://bugs.webkit.org/attachment.cgi?id=217455&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.