[webkit-reviews] review granted: [Bug 207423] Throw OutOfMemory exception instead of crashing if DirectArguments/ScopedArguments can't be created : [Attachment 390155] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 207423: Throw OutOfMemory exception instead of crashing if
DirectArguments/ScopedArguments can't be created
https://bugs.webkit.org/show_bug.cgi?id=207423

Attachment 390155: Patch

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




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 390155
  --> https://bugs.webkit.org/attachment.cgi?id=390155
Patch

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

r=me

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1107
> +	   LLINT_CHECK_EXCEPTION();

This is redundant because there's a LLINT_CHECK_EXCEPTION() immediately
following this if statement.  Please remove.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:984
> +	   CHECK_EXCEPTION();

I think you should put this after the if statement to match
LLINT_SLOW_PATH_DECL(slow_path_del_by_val).  Looks like currently, we're
missing an exception check after the call to deleteProperty().

> Source/JavaScriptCore/runtime/DirectArguments.cpp:125
> +    if (!backingStore) {

UNLIKELY()?

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:175
> +    scope.release();

Hmmm, use RELEASE_AND_RETURN instead of return in both cases below instead? 
Not a big difference right now, but I think that is less error prone if any of
the cases below expand to have more logic that might throw.

> Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:285
> +	   if (!backingStore) {

UNLIKELY()?