Closed
Bug 564621
Opened 15 years ago
Closed 14 years ago
JSON.parse shouldn't allow trailing commas, e.g. {"a" : "b",}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla2.0b5
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: Ms2ger, Assigned: Waldo)
References
Details
(Keywords: dev-doc-complete, Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
sayrer
:
review+
|
Details | Diff | Splinter Review |
{"a" : "b",} or ["foo",] should raise a SyntaxError. See ES5 (final final final final draft standard), printed page 202. (Or <http://www.json.org>.)
Assignee | ||
Comment 1•15 years ago
|
||
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.
Updated•15 years ago
|
Attachment #444298 -
Flags: review?(sayrer) → review+
Assignee | ||
Comment 2•14 years ago
|
||
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...
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.
Assignee | ||
Comment 4•14 years ago
|
||
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.
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•14 years ago
|
Whiteboard: [awaiting bug 505656 branch landings, subsequent point releases, and percolation]
Assignee | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Summary: JSON.parse shouldn't allow {"a" : "b",} → JSON.parse shouldn't allow trailing commas, e.g. {"a" : "b",}
Assignee | ||
Comment 7•14 years ago
|
||
Not landed, just a little bit to ease some docs additions requested in another bug...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•14 years ago
|
blocking2.0: --- → betaN+
Assignee | ||
Comment 8•14 years ago
|
||
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.
Status: REOPENED → ASSIGNED
Whiteboard: [awaiting bug 505656 branch landings, subsequent point releases, and percolation] → fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b5
Comment 9•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•