[webkit-reviews] review denied: [Bug 199292] JSON.parse incorrectly handles array proxies : [Attachment 373059] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 199292: JSON.parse incorrectly handles array proxies
https://bugs.webkit.org/show_bug.cgi?id=199292

Attachment 373059: Patch

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 373059
  --> https://bugs.webkit.org/attachment.cgi?id=373059
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +	   1. Use isArray to correctly detect proxied arrays.
> +	   2. Make "length" lookup observable to array proxies and handle
exceptions.

Let's add each test in addition to test262 to ensure this behavior.

> Source/JavaScriptCore/runtime/JSONObject.cpp:675
> +		   ASSERT(isArray(m_exec, inValue));
>		   if (markedStack.size() > maximumFilterRecursion)
>		       return throwStackOverflowError(m_exec, scope);
>  
> -		   JSArray* array = asArray(inValue);
> +		   auto array = asObject(inValue);
>		   markedStack.appendWithCrashOnOverflow(array);
> -		   arrayLengthStack.append(array->length());
> +		   unsigned length = isJSArray(array)
> +		       ? asArray(array)->length()
> +		       : array->get(m_exec,
vm.propertyNames->length).toUInt32(m_exec);
> +		   RETURN_IF_EXCEPTION(scope, { });
> +		   arrayLengthStack.append(length);

`isArray` is user-observable, side-effect operations. When we encounter the
revoked Proxy, then we throw an error.
So,

1. When `isArray` is used, we need to do error-handling correctly.
2. Since this error is observable (like, throwing an error before/after the
other operations, which can be observable to users), when calling `isArray`
becomes important.

Is this `isArray()` call specified in the spec?