Bug 199292 – JSON.parse incorrectly handles array proxies

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Shvayka]

1. Arrays should be detected using `isArray`:

ECMA262: https://tc39.es/ecma262/#sec-internalizejsonproperty (step 2.a)
Test262: https://test262.report/browse/built-ins/JSON/parse/revived-proxy.js

2. "length" lookup should be observable by proxies:

ECMA262: https://tc39.es/ecma262/#sec-internalizejsonproperty (step 2.b.ii)
Test262:
  https://test262.report/browse/built-ins/JSON/parse/reviver-array-length-get-err.js
  https://test262.report/browse/built-ins/JSON/parse/reviver-array-length-coerce-err.js

[Alexey Shvayka]

Created attachment 373059 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 373059 [details]
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?

[Alexey Shvayka]

Created attachment 373546 [details]
Patch

Remove isArray from ASSERTs, add stress tests for 1, 2, and isArray order.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 373059 [details]
> Patch
> 
> 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?

Nice catch, thanks. I've replaced ASSERTs with comments and added error-handling for isArray call in StateUnknown (that one is per spec).

[Alexey Shvayka]

Created attachment 376429 [details]
Patch

Replace comments with real ASSERTs, use toLength.

[Saam Barati]

Comment on attachment 376429 [details]
Patch

nice. Can you verify this doesn't make JSON parsing slower?

[Alexey Shvayka]

Created attachment 377862 [details]
Patch

Add 4 microbenchmarks and set reviewer.

[Alexey Shvayka]

(In reply to Saam Barati from comment #6)
> Comment on attachment 376429 [details]
> Patch
> 
> nice. Can you verify this doesn't make JSON parsing slower?

Please note that changes in Walker::walk may affect only JSON.parse with custom reviver (2nd argument).
I've added two sets of microbenchmarks: reviver returning same or different value.
We will find them useful in https://bugs.webkit.org/show_bug.cgi?id=163446 as well.

Cold runs, --outer 16:

                                            rev. 249368              patch

json-parse-object-reviver-same-value      196.5163+-1.1350      198.7460+-1.4635
json-parse-array-reviver-same-value       143.3628+-1.1241      150.7418+-1.2160
json-parse-object-reviver                 217.9791+-1.2748      223.2554+-1.9196
json-parse-array-reviver                  197.3965+-3.8484      198.1227+-1.9369

<geometric>                               186.5654+-1.0157      190.7782+-0.8042

[Alexey Shvayka]

Created attachment 380451 [details]
Patch

Rebase patch.

[WebKit Commit Bot]

Comment on attachment 380451 [details]
Patch

Clearing flags on attachment: 380451

Committed r250860: <https://trac.webkit.org/changeset/250860>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56088749>

[Alexey Shvayka]

*** Bug 155267 has been marked as a duplicate of this bug. ***