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
Created attachment 373059 [details] Patch
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?
Created attachment 373546 [details] Patch Remove isArray from ASSERTs, add stress tests for 1, 2, and isArray order.
(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).
Created attachment 376429 [details] Patch Replace comments with real ASSERTs, use toLength.
Comment on attachment 376429 [details] Patch nice. Can you verify this doesn't make JSON parsing slower?
Created attachment 377862 [details] Patch Add 4 microbenchmarks and set reviewer.
(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
Created attachment 380451 [details] Patch Rebase patch.
Comment on attachment 380451 [details] Patch Clearing flags on attachment: 380451 Committed r250860: <https://trac.webkit.org/changeset/250860>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56088749>
*** Bug 155267 has been marked as a duplicate of this bug. ***