JSON.parse: properly deal with reviver result

JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable when the reviver modifies its "this" object directly.  There are a few further issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/a5380fe9ed8eecdda79f709d5a9c4714d97fa623
Cr-Commit-Position: refs/heads/master@{#32750}

[Yang, 2015-12-08 14:21:54]

https://codereview.chromium.org/1506933003/diff/1/src/js/json.js
File src/js/json.js (right):

https://codereview.chromium.org/1506933003/diff/1/src/js/json.js#newcode34
src/js/json.js:34: if (IS_ARRAY(val)) {
I thought IS_ARRAY is not the same as IsArray from the spec?

https://codereview.chromium.org/1506933003/diff/1/src/js/json.js#newcode42
src/js/json.js:42: val[i] = newElement;
is this equivalent to CreateDataProperty? What if val has a getter defined for
i?

[Yang, 2015-12-09 08:18:47]

On 2015/12/08 14:21:54, Yang wrote:
> https://codereview.chromium.org/1506933003/diff/1/src/js/json.js
> File src/js/json.js (right):
> 
> https://codereview.chromium.org/1506933003/diff/1/src/js/json.js#newcode34
> src/js/json.js:34: if (IS_ARRAY(val)) {
> I thought IS_ARRAY is not the same as IsArray from the spec?
> 
> https://codereview.chromium.org/1506933003/diff/1/src/js/json.js#newcode42
> src/js/json.js:42: val[i] = newElement;
> is this equivalent to CreateDataProperty? What if val has a getter defined for
> i?

lgtm.

[neis, 2015-12-09 09:41:45]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2015-12-09 09:41:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506933003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506933003/1

[commit-bot: I haz the power, 2015-12-09 09:55:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 09:55:16]

Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11203)

[neis, 2015-12-09 12:12:58]

The CQ bit was checked by neis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 12:13:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506933003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506933003/20001

[commit-bot: I haz the power, 2015-12-09 12:27:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 12:27:17]

Dry run: Try jobs failed on following builders:
  v8_linux_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel/builds/12807)

[neis, 2015-12-09 12:52:57]

Please have another look.  I had to adapt a webkit test.  While doing so, I
realized that the enumeration of property keys wasn't quite correct either.

I had to blacklist the test for the interpreter because the interpreter doesn't
yet support for-of.

[Yang, 2015-12-09 20:19:09]

On 2015/12/09 12:52:57, neis wrote:
> Please have another look.  I had to adapt a webkit test.  While doing so, I
> realized that the enumeration of property keys wasn't quite correct either.
> 
> I had to blacklist the test for the interpreter because the interpreter
doesn't
> yet support for-of.

lgtm. please also run webkit layout tests.

[adamk, 2015-12-09 23:49:20]

https://codereview.chromium.org/1506933003/diff/1/src/js/json.js
File src/js/json.js (right):

https://codereview.chromium.org/1506933003/diff/1/src/js/json.js#newcode42
src/js/json.js:42: val[i] = newElement;
On 2015/12/08 14:21:54, Yang wrote:
> is this equivalent to CreateDataProperty? What if val has a getter defined for
> i?

What was the response to this comment? Perhaps this is related to
https://bugs.chromium.org/p/v8/issues/detail?id=3139?

[neis, 2015-12-10 08:35:44]

(It seems that hitting reply-all on the comment email neither includes the
commenter nor posts to the code review...)

Hi Adam,

my offline response to Yang was as follows:

Regarding IS_ARRAY, that should be fine because the Object created by
JSON.parse can never be a proxy.
Regarding CreateDataProperty, that should be fine because the Object
created by JSON.parse doesn't have any setters (I suppose Yang meant
setters, not getters).

It's now clear to me that both statements are wrong because I
underestimated the power of the reviver function.  Since the reviver is passed
the object as "this" value it can modify the values of yet-to-be-visited keys,
eg inserting a proxy.  I really should have seen this when touching the tests. 
Thanks!

I was thinking of extending this CL but right now I think it makes more sense to
address these issues in a separate CL (that will also close the bug report you
linked to).

As a heads up, I'm also preparing a CL concerning JSON.stringify,
mostly for correctly dealing with proxies as replacer.

[neis, 2015-12-10 12:00:58]

Description was changed from

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.


R=yangguo@chromium.org
BUG=
==========

to

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable
when the reviver modifies its "this" object directly.  There are a few further
issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=
==========

[neis, 2015-12-10 12:06:22]

The CQ bit was checked by neis@chromium.org

[neis, 2015-12-10 12:06:23]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1506933003/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-10 12:06:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1506933003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1506933003/80001

[commit-bot: I haz the power, 2015-12-10 12:48:42]

Description was changed from

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable
when the reviver modifies its "this" object directly.  There are a few further
issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=
==========

to

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable
when the reviver modifies its "this" object directly.  There are a few further
issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2015-12-10 12:48:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-10 12:49:13]

Description was changed from

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable
when the reviver modifies its "this" object directly.  There are a few further
issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=
==========

to

==========
JSON.parse: properly deal with reviver result

When the reviver returns undefined, the property in question must be deleted
even for arrays.  So far this only happened for non-array objects.

Also change the property enumeration to be spec-conformant, which is observable
when the reviver modifies its "this" object directly.  There are a few further
issues that need to be addressed in a separate CL.

R=yangguo@chromium.org
BUG=

Committed: https://crrev.com/a5380fe9ed8eecdda79f709d5a9c4714d97fa623
Cr-Commit-Position: refs/heads/master@{#32750}
==========

[commit-bot: I haz the power, 2015-12-10 12:49:14]

Patchset 5 (id:??) landed as
https://crrev.com/a5380fe9ed8eecdda79f709d5a9c4714d97fa623
Cr-Commit-Position: refs/heads/master@{#32750}