Bug 163446 – JSON.parse should not modify non-configurable properties.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

In response to a reviver function's result, JSON.parse modifies an object property using the CreateDataProperty spec function.  CreateDataProperty is specified to fail if the property is non-configurable.  Hence, JSON.parse should not modify non-configurable properties.

[Alexey Shvayka]

Created attachment 373115 [details]
Patch

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12603230

New failing tests:
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373116 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12603280

New failing tests:
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373119 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12603304

New failing tests:
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373120 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12603319

New failing tests:
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373121 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12603276

New failing tests:
ChakraCore.yaml/ChakraCore/test/JSON/jx2.js.default
apiTests

[jsc-armv7 EWS]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass jsc-armv7-ews (jsc-only):
Output: https://webkit-queues.webkit.org/results/12603273

New failing tests:
ChakraCore.yaml/ChakraCore/test/JSON/jx2.js.default
apiTests

[Yusuke Suzuki]

Comment on attachment 373115 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSONObject.cpp:716
> +                else {
> +                    PropertyDescriptor desc(filteredValue, static_cast<unsigned>(PropertyAttribute::None));
> +                    bool shouldThrow = false;
> +                    array->defineOwnIndexedProperty(m_exec, indexStack.last(), desc, shouldThrow);
> +                }

This looks very slow to me. Can we call this slow path only when the receiver callback is passed?

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12603481

New failing tests:
http/tests/css/filters-on-iframes-transform.html
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373128 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[EWS Watchlist]

Comment on attachment 373115 [details]
Patch

Attachment 373115 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12604786

New failing tests:
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 373136 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Alexey Shvayka]

Created attachment 373178 [details]
Patch

Fix test and clarify comments.

[Alexey Shvayka]

Created attachment 373180 [details]
Patch

Adjust expectations for ChakraCore test.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 373115 [details]
> Patch
> 
> This looks very slow to me. Can we call this slow path only when the
> receiver callback is passed?

This method (Walker::walk) is invoked only if receiver is a function.
Also, JSObject::defineOwnIndexedProperty has fast path to putDirectIndex, which will always be taken for regular arrays.

There is another patch for this method where I preserved fast path for regular arrays: https://bugs.webkit.org/show_bug.cgi?id=199292.
I would really appreaciate your feedback on it.

---

Tests were failing because [[DefineOwnProperty]] changes enumeration order for existing property if given descriptor is different:

```
let obj = {a: 1, b: 2}
Object.defineProperty(obj, "a", {value: 3})
Object.keys(obj) // => ["b", "a"]
```

Please see https://bugs.webkit.org/show_bug.cgi?id=142933.

[Darin Adler]

Comment on attachment 373180 [details]
Patch

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

I didn't fully review this, and someone with more JSC expertise will probably review, but I did have a few comments:

To me this looks like this might have a performance effect. Can someone help us check JSON parsing performance to make sure it’s either faster or not measurably slower?

> JSTests/ChangeLog:8
> +        * ChakraCore/test/JSON/jx2.baseline-jsc: Adjust expectations.

*Why* do the expectations change? It looks like orders of properties are changing? Why is that happening? Is it actively better or just OK? Why is it OK?

If this is an accident, then: Is there something the authors of the test can do to make it less dependent on engine internals? Should we be doing something in our engine so serialization writes in a predictable order instead of what seems like arbitrary order?

> Source/JavaScriptCore/runtime/JSONObject.cpp:713
> +                    PropertyDescriptor desc(filteredValue, static_cast<unsigned>(PropertyAttribute::None));

While we sometimes use abbreviations like "desc", generally WebKit project coding style encourages use of words like "description" instead.

> Source/JavaScriptCore/runtime/JSObject.h:213
> +    // This is pretty much like performing defineOwnProperty, but without extensibility check and property descriptor validation.

Why the words "pretty much"? Is there something not mentioned here? The comment would be better without the vagueness of "pretty much". Could the comment be:

// This is like defineOwnProperty without an extensibility check or property descriptor validation.

Or is that inaccurate in some way?

> Source/JavaScriptCore/runtime/JSObject.h:242
> +    // This is pretty much like performing defineOwnProperty(propertyName, {configurable:true, writable:true, enumerable:true, value:value}),
> +    // but without extensibility check and property descriptor validation.

Ditto.

[Alexey Shvayka]

Created attachment 373199 [details]
Patch

Rename variables and remove comments.

[Alexey Shvayka]

(In reply to Darin Adler from comment #20)
> Comment on attachment 373180 [details]
> Patch
> 
> To me this looks like this might have a performance effect.

For arrays, we are still on the fast path: this change won't cause any noticable regressions.
For objects though, this patch adds a few descriptor checks, nothing particularly slow.

We can skip calling [[DefineOwnProperty]] if returned value from reviver is the same and holder is not a Proxy.
V8 invokes [[DefineOwnProperty]] without extra checks though (for both arrays and objects).

> > JSTests/ChangeLog:8
> > +        * ChakraCore/test/JSON/jx2.baseline-jsc: Adjust expectations.
> 
> *Why* do the expectations change? It looks like orders of properties are
> changing? Why is that happening? Is it actively better or just OK? Why is it
> OK?
> 
> If this is an accident, then: Is there something the authors of the test can
> do to make it less dependent on engine internals? Should we be doing
> something in our engine so serialization writes in a predictable order
> instead of what seems like arbitrary order?

Serialization is perfect: property order is correct if `reviver` is not provided.
Order has changed because JSObject::defineOwnProperty removes old descriptor and inserts new one in new place (instead of overwriting and keeping the order).
It doesn't corrupt property order if `reviver` returns `value` (second argument), which is the most common case I've seen in the wild.
I've made js/dom/JSON-parse.html independent of this. ChakraCore test relies on `reviver` returning another value.
Please see comment #19.

> 
> > Source/JavaScriptCore/runtime/JSObject.h:213
> > +    // This is pretty much like performing defineOwnProperty, but without extensibility check and property descriptor validation.
> 
> Why the words "pretty much"? Is there something not mentioned here? 

`defineOwnProperty` is actually performed in full, with all the checks, but only if `canDoFastPutDirectIndex` returns `false`.
`canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I removed references to `defineOwnProperty` from comments.

[Darin Adler]

(In reply to Alexey Shvayka from comment #22)
> For arrays, we are still on the fast path: this change won't cause any
> noticable regressions.

Normally we like to prove that through some quick performance testing rather than just relying on logic.

> For objects though, this patch adds a few descriptor checks, nothing
> particularly slow.
> 
> We can skip calling [[DefineOwnProperty]] if returned value from reviver is
> the same and holder is not a Proxy.
> V8 invokes [[DefineOwnProperty]] without extra checks though (for both
> arrays and objects).

Again, I think we can define "fast enough" by running some performance tests and making sure it's not something measurable.

> Order has changed because JSObject::defineOwnProperty removes old descriptor
> and inserts new one in new place (instead of overwriting and keeping the
> order).

Thanks. That's good new.

I suggest the reason for the change in the test results be mentioned in the change log.

> `canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I
> removed references to `defineOwnProperty` from comments.

I’m a little sad to hear that. I like the idea of having a comment, but if we can’t find something that is simple and clear to say, I understand making the choice to remove the comment instead.

[Alexey Shvayka]

Created attachment 373595 [details]
Patch

Add microbenchmarks and note on changed test results.

[Alexey Shvayka]

(In reply to Darin Adler from comment #23)
> (In reply to Alexey Shvayka from comment #22)
> > For arrays, we are still on the fast path: this change won't cause any
> > noticable regressions.
> 
> Normally we like to prove that through some quick performance testing rather
> than just relying on logic.
> 

I've failed to find any usages of `reviver` in mainstream benchmarks, so I've added a few microbenchmarks. Results of 10 runs x 4 samples:
  json-parse-array-reviver: 235.92ms trunk, 240.70ms patch (+2%)
  json-parse-array-reviver-same-value: 164.82ms trunk, 170.60ms patch (+3.5%)
  json-parse-object-reviver: 256.48ms trunk, 539.20ms patch (+110%)
  json-parse-object-reviver-same-value: 227.29ms trunk, 240.44ms patch (+5.8%)

As it was expected, common cases (*-same-value) are fine, but result of `json-parse-object-reviver` is pretty disturbing.
I think we should fix https://bugs.webkit.org/show_bug.cgi?id=142933 first and measure performance again.

[Yusuke Suzuki]

(In reply to Alexey Shvayka from comment #25)
> (In reply to Darin Adler from comment #23)
> > (In reply to Alexey Shvayka from comment #22)
> > > For arrays, we are still on the fast path: this change won't cause any
> > > noticable regressions.
> > 
> > Normally we like to prove that through some quick performance testing rather
> > than just relying on logic.
> > 
> 
> I've failed to find any usages of `reviver` in mainstream benchmarks, so
> I've added a few microbenchmarks. Results of 10 runs x 4 samples:
>   json-parse-array-reviver: 235.92ms trunk, 240.70ms patch (+2%)
>   json-parse-array-reviver-same-value: 164.82ms trunk, 170.60ms patch (+3.5%)
>   json-parse-object-reviver: 256.48ms trunk, 539.20ms patch (+110%)
>   json-parse-object-reviver-same-value: 227.29ms trunk, 240.44ms patch
> (+5.8%)
> 
> As it was expected, common cases (*-same-value) are fine, but result of
> `json-parse-object-reviver` is pretty disturbing.
> I think we should fix https://bugs.webkit.org/show_bug.cgi?id=142933 first
> and measure performance again.

Can you measure the perf again because the reordering patch is landed :) ?

[Alexey Shvayka]

Created attachment 404651 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #27)
> Created attachment 404651 [details]
> Patch

Warmed-up runs, --outer 50:

                                               r264388                   patch

json-parse-object-reviver-same-value      241.8507+-4.5414     ^    213.8166+-4.2992        ^ definitely 1.1311x faster
json-parse-object-reviver                 270.6204+-4.2983     ^    239.5138+-4.4341        ^ definitely 1.1299x faster
json-parse-array-reviver-same-value       168.0164+-3.1485     ?    169.7259+-2.2783        ? might be 1.0102x slower
json-parse-array-reviver                  223.8018+-4.1355     ?    229.6355+-3.6125        ? might be 1.0261x slower

<geometric>                               222.3860+-2.0189     ^    211.0908+-1.9662        ^ definitely 1.0535x faster


(In reply to Yusuke Suzuki from comment #26)
> Can you measure the perf again because the reordering patch is landed :) ?

Without the fast path, it is 20% regression for json-parse-object-reviver.js.

[Alexey Shvayka]

(In reply to Darin Adler from comment #23)
> > `canDoFastPutDirectIndex` is a bit complex to describe it in comment, so I
> > removed references to `defineOwnProperty` from comments.
> 
> I’m a little sad to hear that. I like the idea of having a comment, but if
> we can’t find something that is simple and clear to say, I understand making
> the choice to remove the comment instead.

r258170 fixed putDirectIndex() to perform [[DefineOwnProperty]], so I've reverted the comment change.

[Darin Adler]

Should I be worried about the json-parse-array-reviver slow-down?

[EWS]

Committed r264833: <https://trac.webkit.org/changeset/264833>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404651 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66058601>

[Alexey Shvayka]

(In reply to Darin Adler from comment #30)
> Should I be worried about the json-parse-array-reviver slow-down?

I don't think so: slow path this patch adds isn't executed by these microbenchmarks.
Retested with the same params:

                                             r264603                    patch

json-parse-array-reviver-same-value      160.5339+-3.6430     ?    161.9704+-2.7285        ?
json-parse-array-reviver                 221.6459+-3.4088          217.0408+-4.2595          might be 1.0212x faster

<geometric>                              188.4357+-2.7034          187.2907+-2.3668          might be 1.0061x faster


Results are almost always within error margin.