[json parser] add position to error message.

[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N

Committed: https://crrev.com/35b6ca2528173896b26217c1067efc468c9ee75d
Cr-Commit-Position: refs/heads/master@{#33864}

[Yang, 2016-02-08 07:42:42]

This is the response to the feature request here:
https://groups.google.com/forum/#!topic/v8-users/-6xWd6JPBSU

I'm not 100% sure whether there are security concerns. The idea would be that
the error message could expose too much information.

JSON.parse when called from Javascript should be fine, since we already own the
string we pass to JSON.parse. Callers of the API (v8::JSON::Parse) do not seem
to propagate any error messages. So to me this seems safe enough. Firefox
exposes the position information similarly.

[Toon Verwaest, 2016-02-08 13:04:22]

lgtm

[alexk, 2016-02-08 15:38:11]

On 2016/02/08 13:04:22, Toon Verwaest wrote:
> lgtm

I think this is a huge improvement but...

The one nit I'd have is that it seems like it would be nice for the messages to
indicate the error is in JSON.parse -- one typically expects SyntaxErrror to be
associated with errors in the JS and it's helpful to make it very clear that the
problem is with JSON.parse (FireFox does). If it seems wasteful to have three
messages reserved for JSON.parse maybe it would make sense to eliminate the
distinction between number and character which is, in many cases arbitrary -- a
\x results in a "Unexpected token x" while a \6 results in a "Unexpected
number". Or even more logical would be for the lower level JSONParser methods to
provide an error indicator in the JSONParse object.

I'm probably over-analyzing a minor little corner of v8 but... as long as the
heart is on the table...?

[Yang, 2016-02-10 06:50:40]

Description was changed from

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
==========

to

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N
==========

[Yang, 2016-02-10 06:51:44]

yangguo@chromium.org changed reviewers:
+ littledan@chromium.org

[Yang, 2016-02-10 06:51:44]

Dan, could you review this as owner? Thanks.

[Toon Verwaest, 2016-02-10 10:07:41]

@Dan: can you please move json-parser.h out of parsing. It's entirely unrelated;
and I'm probably the only one who knows how it works...

[Yang, 2016-02-10 10:12:30]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-10 10:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1681513002/#ps40001
(title: "also move json-parser.h")

[commit-bot: I haz the power, 2016-02-10 10:12:39]

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

[Yang, 2016-02-10 10:12:42]

On 2016/02/10 10:07:41, Toon Verwaest wrote:
> @Dan: can you please move json-parser.h out of parsing. It's entirely
unrelated;
> and I'm probably the only one who knows how it works...

Also moved the file now.

[Dan Ehrenberg, 2016-02-10 10:13:00]

On 2016/02/10 at 10:07:41, verwaest wrote:
> @Dan: can you please move json-parser.h out of parsing. It's entirely
unrelated; and I'm probably the only one who knows how it works...

Where should it go?

[Dan Ehrenberg, 2016-02-10 10:13:56]

lgtm

Oh, I see, this patch does it. I'm happy with this move, so LGTM with respect to
the move (I'm trusting that others have reviewed the code).

[Yang, 2016-02-10 10:20:16]

On 2016/02/10 10:13:56, Dan Ehrenberg wrote:
> lgtm
> 
> Oh, I see, this patch does it. I'm happy with this move, so LGTM with respect
to
> the move (I'm trusting that others have reviewed the code).

Funny enough, moving the file removes the need for OWNER review :P

[Yang, 2016-02-10 10:26:56]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-10 10:26:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1681513002/#ps60001
(title: "fix BUILD.gn")

[commit-bot: I haz the power, 2016-02-10 10:27:06]

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

[commit-bot: I haz the power, 2016-02-10 11:05:09]

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

[commit-bot: I haz the power, 2016-02-10 11:05:09]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: No JSON object could be decoded

[Yang, 2016-02-10 11:26:59]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2016-02-10 11:27:08]

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

[commit-bot: I haz the power, 2016-02-10 11:28:16]

Description was changed from

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N
==========

to

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N
==========

[commit-bot: I haz the power, 2016-02-10 11:28:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-10 11:28:57]

Description was changed from

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N
==========

to

==========
[json parser] add position to error message.

R=jochen@chromium.org, verwaest@chromium.org
BUG=chromium:585724
LOG=N

Committed: https://crrev.com/35b6ca2528173896b26217c1067efc468c9ee75d
Cr-Commit-Position: refs/heads/master@{#33864}
==========

[commit-bot: I haz the power, 2016-02-10 11:28:58]

Patchset 4 (id:??) landed as
https://crrev.com/35b6ca2528173896b26217c1067efc468c9ee75d
Cr-Commit-Position: refs/heads/master@{#33864}