Closed Bug 564621 Opened 10 years ago Closed 9 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: Ms2ger, Assigned: Waldo)



(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)


(2 files)

Attached file Testcase
{"a" : "b",} or ["foo",] should raise a SyntaxError. See ES5 (final final final final draft standard), printed page 202. (Or <>.)
Attached patch PatchSplinter Review
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.
Assignee: general → jwalden+bmo
Attachment #444298 - Flags: review?(sayrer)
Attachment #444298 - Flags: review?(sayrer) → review+
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...
Depends on: 505656
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.
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.
Blocks: 512442
Whiteboard: [awaiting bug 505656 branch landings, subsequent point releases, and percolation]
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:

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.
Depends on: 582077
No longer depends on: 505656
Closed: 9 years ago
Resolution: --- → FIXED
Summary: JSON.parse shouldn't allow {"a" : "b",} → JSON.parse shouldn't allow trailing commas, e.g. {"a" : "b",}
Not landed, just a little bit to ease some docs additions requested in another bug...
Resolution: FIXED → ---
blocking2.0: --- → betaN+

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.
Whiteboard: [awaiting bug 505656 branch landings, subsequent point releases, and percolation] → fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b5

Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.