[webkit-reviews] review denied: [Bug 129429] [Win32][LLINT] Crash when running JSC stress tests. : [Attachment 225791] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied peavo at outlook.com's request for
review:
Bug 129429: [Win32][LLINT] Crash when running JSC stress tests.
https://bugs.webkit.org/show_bug.cgi?id=129429

Attachment 225791: Patch
https://bugs.webkit.org/attachment.cgi?id=225791&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225791&action=review


Looking good, but I think this needs one more pass.

> Source/JavaScriptCore/runtime/VM.cpp:763
> +// when needed, see http://support.microsoft.com/kb/100775

"." at the end of the sentence, please.

> Source/JavaScriptCore/runtime/VM.cpp:778
> +#if PLATFORM(WIN) && ENABLE(LLINT)

I don't think we want to check ENABLE(LLINT) here. For example, if you disabled
LLInt and enabled JIT, you would still need preCommitStackMemory. So, let's
just check PLATFORM(WIN).

> Source/JavaScriptCore/runtime/VM.cpp:803
> +#if PLATFORM(WIN) && ENABLE(LLINT)
> +    if (lastStackLimit != m_stackLimit)
> +	   preCommitStackMemory(m_stackLimit);
> +#endif

Looks good.

> Source/WTF/wtf/WTFThreadData.h:99
>	   // We need to always get a fresh StackBounds from the OS due to how
fibers work.
>	   // See https://bugs.webkit.org/show_bug.cgi?id=102411
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) && !ENABLE(LLINT)
>	   m_stackBounds = StackBounds::currentThreadStackBounds();
>  #endif
>	   return m_stackBounds;

I don't think we want to check ENABLE(LLINT) here. A Windows client can use
fibers regardless of LLInt.