564621 - JSON.parse shouldn't allow trailing commas, e.g. {"a" : "b",}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Testcase

{"a" : "b",} or ["foo",] should raise a SyntaxError. See ES5 (final final final final draft standard), printed page 202. (Or <http://www.json.org>.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

I'm guessing the JSON_PARSE_STATE_OBJECT state was dead code from when es5 required JSON.parse to not accept 'true', 'false', and 'null', but I didn't bother checking.  MXR says it's dead, and it didn't seem worth investigating beyond that.

I also fully fixed an older bug noted in passing from a comment/code mismatch; the original reviewer was an illiterate idiot to r+ that.

This passes all shell tests, will give it a throw at tryserver when tinderbox infra isn't awol.

Comment 2

[Jeff Walden [:Waldo]]

Apparently we have at least some code that depends on the validity of trailing commas, in array literal syntax if nowhere else -- test_browserGlue_corrupt.js times out without trailing-comma support.  ψ.  So I guess I need to figure out where that's being generated, fix that, and see if there are other latent failures after that.  Try server's going to be getting a workout with this change, I bet...

Comment 3

[Mike Shaver (:shaver emeritus)]

Why do you need try server for this?  Why not run the tests on your machine, to iterate?  Running the full series of tests on all operating systems, including performance, seems like a waste of scarce and shared resources, if you're not chasing something that has platform-specific or performance-related effects.

Comment 4

[Jeff Walden [:Waldo]]

I could, and I do in small portions, once I get initial feedback on exactly what bits are failing.  But I don't have the time to do full runs of every single test suite (and likely forget some in the process) on my own system, so I try to optimize: run past try server to get feedback on which unknown bits are broken, fix those and retest those isolated bits locally, then iterate once or twice more depending on whether revealed failures mask further failures or not.  This almost always is unnecessary for SpiderMonkey patches (I'd estimate I've done this maybe three or so times in the last year), but for the occasional one likely to be especially exercised by browser or non-JS testsuite tests I'll put it through this.

As it turns out, here there seem to only be two problems: one xpcshell places test that has a non-JSON input file (fixed in the bug depending on this), and one places issue in code which generates files like it (fixed or nearly so in the bug depending on this).  I think that's it that I need to worry about for this bug, but it's hard to say since there are some latent TraceMonkey failures from bug 558754 that test runs are tripping over (I have other tasks to work through so haven't investigated them closely).  I'm pretty sure they're isolated from the problems this could/does cause, but I'm not sure enough to bet the farm on it.

Comment 5

[Jeff Walden [:Waldo]]

I split out the JSONParserState docs changes from the JSONParserState modifications, so that changes there right now don't have to either lack docs (for parity with the surrounding code) or anomalously include them:

http://hg.mozilla.org/tracemonkey/rev/db4607b00594

The meat of the patch, including the JSONParserState state additions, changes, and removals, remains blocked by bug 505656, so this bug should continue to remain open.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/db4607b00594

Comment 7

[Jeff Walden [:Waldo]]

Not landed, just a little bit to ease some docs additions requested in another bug...

Comment 8

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/77a30f0f6a17

The revision also includes the patch from bug 582077 -- would be simpler to separate them, but I was wary of someone somehow hitting the intermediate state with strict-JSON-but-bad-bookmarks, so I folded them into one.

Comment 9

[Brendan Eich [:brendan]]

http://hg.mozilla.org/mozilla-central/rev/77a30f0f6a17

/be