[webkit-reviews] review granted: [Bug 215114] CheckpointSideState shoud play nicely with StackOverflowException unwinding. : [Attachment 405892] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 215114: CheckpointSideState shoud play nicely with StackOverflowException
unwinding.
https://bugs.webkit.org/show_bug.cgi?id=215114

Attachment 405892: Patch

https://bugs.webkit.org/attachment.cgi?id=405892&action=review




--- Comment #2 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 405892
  --> https://bugs.webkit.org/attachment.cgi?id=405892
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   automatically unwind into the first frame before calling into the

you should say why: we do this b/c stack overflow does it purposefully because
the top frame isn't fully materialized.

> Source/JavaScriptCore/ChangeLog:16
> +	   miggrated us from another thread below the current thread.

miggrated -> migrated

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:663
> +

revert

> Source/JavaScriptCore/runtime/VM.cpp:1058
> +#if ASSERT_ENABLED
> +    for (const auto& sideState : m_checkpointSideState)
> +	   ASSERT(sideState->associatedCallFrame != callFrame);
> +#endif

Let's also assert it's sorted after the append.

> Source/JavaScriptCore/runtime/VM.cpp:1066
> +    ASSERT_UNUSED(expectedCallFrame, sideState->associatedCallFrame ==
expectedCallFrame);

let's make this RELEASE_ASSERT

> Source/JavaScriptCore/runtime/VM.cpp:1074
> +    ASSERT(bounds.contains(target));

WDYT about a release assert?

> Source/WTF/wtf/StackBounds.h:107
> +    StackBounds withSoftOrigin(void* origin) const

nit: Why not call this "withOrigin"? Seems more in line with the name
"origin()" used elsewhere